search

tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.52k stars 2.44k forks source link

Arm virt psci on armmonitorlib #5933

Closed ardbiesheuvel closed 1 month ago

ardbiesheuvel commented 1 month ago

Related to #5922

The first patch was taken from #5922 but should not be merged as part of this PR.

os-d commented 1 month ago

This looks reasonable to me, but is it possible to have a single ArmResetSystemLib, since this new version just depends on a feature PCD to determine whether to make SMC or HVC calls, we could do away with the SMC only version?

ardbiesheuvel commented 1 month ago

This looks reasonable to me, but is it possible to have a single ArmResetSystemLib, since this new version just depends on a feature PCD to determine whether to make SMC or HVC calls, we could do away with the SMC only version?

Yes, we could remove ArmSmcPsciResetSystemLib after this change, but we'd have to update the users in edk2-platforms first.

os-d commented 1 month ago

Sounds good. I can queue that up after this change, I think it is worthwhile to only have one lib to maintain.

ardbiesheuvel commented 1 month ago

@makubacki @mdkinney This PR failswas failing the uncrustify checks but the attachments are missing from the test results.

Please advise how to determine why these checks failed from the CI web UI.

makubacki commented 1 month ago

@makubacki @mdkinney This PR ~fails~was failing the uncrustify checks but the attachments are missing from the test results.

Please advise how to determine why these checks failed from the CI web UI.

I'm not 100% sure why the attachment is not in the test results, it might be related to audit mode being enabled for uncrustify in ArmPkg. Which means the test was skipped and therefore it may not be shown. I see it in test results UI for other packages that do not have Uncrustify in audit mode. Audit mode also means Uncrustify didn't contribute to the CI failure, it was EccCheck from the ECC errors.

To find it in this case, in the published artifacts, if you download the TARGET_ARM_ARMPLATFORM zip file, you can open TestSuites.xml and see the result that would be in the test UI:

 Formatting errors in Library\ArmMonitorLib\ArmMonitorLib.c
 --- D:\a\1\s\ArmPkg\Library\ArmMonitorLib\ArmMonitorLib.c
 +++ D:\a\1\s\ArmPkg\Library\ArmMonitorLib\ArmMonitorLib.c.uncrustify_plugin
 @@ -26,7 +26,7 @@
 IN OUT ARM_MONITOR_ARGS  *Args
 )
 {
 -  if (FeaturePcdGet  (PcdMonitorConduitHvc)) {
 +  if (FeaturePcdGet (PcdMonitorConduitHvc)) {
 ArmCallHvc ((ARM_HVC_ARGS *)Args);
 } else {
 ArmCallSmc ((ARM_SMC_ARGS *)Args);

 Formatting errors in Library\DebugPeCoffExtraActionLib\DebugPeCoffExtraActionLib.c
 --- D:\a\1\s\ArmPkg\Library\DebugPeCoffExtraActionLib\DebugPeCoffExtraActionLib.c
 +++ D:\a\1\s\ArmPkg\Library\DebugPeCoffExtraActionLib\DebugPeCoffExtraActionLib.c.uncrustify_plugin
 @@ -32,7 +32,7 @@
 IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
 )
 {
 -#ifdef __GNUC__
 + #ifdef __GNUC__
 if (ImageContext->PdbPointer) {
 DEBUG ((
 DEBUG_LOAD | DEBUG_INFO,
 @@ -42,7 +42,8 @@
 ));
 return;
 }
 -#endif
 +
 + #endif

 DEBUG ((
 DEBUG_LOAD | DEBUG_INFO,
 @@ -68,7 +69,7 @@
 IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
 )
 {
 -#ifdef __GNUC__
 + #ifdef __GNUC__
 if (ImageContext->PdbPointer) {
 DEBUG ((
 DEBUG_LOAD | DEBUG_INFO,
 @@ -78,7 +79,8 @@
 ));
 return;
 }
 -#endif
 +
 + #endif

 DEBUG ((
 DEBUG_LOAD | DEBUG_INFO,

The build log (from the zip file) will also list the files that need formatting:

INFO - Formatting errors in Library\ArmMonitorLib\ArmMonitorLib.c
INFO - Formatting errors in Library\DebugPeCoffExtraActionLib\DebugPeCoffExtraActionLib.c
INFO - Setting test as skipped since AuditOnly is enabled
WARNING - --->Test Skipped: in plugin! Uncrustify Coding Standard Test NO-TARGET

Running Uncrustify on those files will make the changes needed.

ardbiesheuvel commented 1 month ago

@makubacki @mdkinney This PR ~fails~was failing the uncrustify checks but the attachments are missing from the test results. Please advise how to determine why these checks failed from the CI web UI.

I'm not 100% sure why the attachment is not in the test results, it might be related to audit mode being enabled for uncrustify in ArmPkg. Which means the test was skipped and therefore it may not be shown. I see it in test results UI for other packages that do not have Uncrustify in audit mode. Audit mode also means Uncrustify didn't contribute to the CI failure, it was EccCheck from the ECC errors.

No, this is not the case. Both EccCheck and Uncrustify failed, but not on a file in ArmPkg so the AuditMode has not bearing on this.

To find it in this case, in the published artifacts, if you download the TARGET_ARM_ARMPLATFORM zip file, you can open TestSuites.xml and see the result that would be in the test UI:

Thanks, this is useful. But could we please fix the CI so the attachments are accessible again? If needed, I can create a dummy PR against ArmVirtPkg (which has AuditMode disabled for Uncrustify) to illustrate the issue.

makubacki commented 1 month ago

@makubacki @mdkinney This PR ~fails~was failing the uncrustify checks but the attachments are missing from the test results. Please advise how to determine why these checks failed from the CI web UI.

I'm not 100% sure why the attachment is not in the test results, it might be related to audit mode being enabled for uncrustify in ArmPkg. Which means the test was skipped and therefore it may not be shown. I see it in test results UI for other packages that do not have Uncrustify in audit mode. Audit mode also means Uncrustify didn't contribute to the CI failure, it was EccCheck from the ECC errors.

No, this is not the case. Both EccCheck and Uncrustify failed, but not on a file in ArmPkg so the AuditMode has not bearing on this.

Can you please clarify? Are you saying you don't think AuditMode impacted the test results being shown in the UI? I will look at that in more detail, I just can't do so today.

EccCheck failed with two errors in ArmPkg:

ERROR - EFI coding style error
ERROR - *Error code: 9001
ERROR - *The file headers should follow Doxygen special documentation blocks in section 2.3.5
ERROR - *file: D:\a\1\s\Build\.pytool\Plugin\EccCheck\ArmPkg\Library\ArmPsciResetSystemLib\ArmPsciResetSystemLib.inf
ERROR - *Line number: 1
ERROR - *Header comment section must have copyright information
ERROR - 
ERROR - EFI coding style error
ERROR - *Error code: 9001
ERROR - *The file headers should follow Doxygen special documentation blocks in section 2.3.5
ERROR - *file: D:\a\1\s\Build\.pytool\Plugin\EccCheck\ArmPkg\Library\ArmPsciResetSystemLib\ArmPsciResetSystemLib.inf
ERROR - *Line number: 1
ERROR - *Header comment section must have Abstract information.
ERROR - --->Test Failed: EccCheck Test NO-TARGET returned 1
PROGRESS - --Running ArmPkg: Guid Check Test NO-TARGET --

Uncrustify was skipped for two files in ArmPkg:

INFO - Formatting errors in Library\ArmMonitorLib\ArmMonitorLib.c
INFO - Formatting errors in Library\DebugPeCoffExtraActionLib\DebugPeCoffExtraActionLib.c
INFO - Setting test as skipped since AuditOnly is enabled
WARNING - --->Test Skipped: in plugin! Uncrustify Coding Standard Test NO-TARGET

The total run had 1 failure (the one from ECC, which was a failure and not a skip):

ERROR - Overall Build Status: Error
PROGRESS - There were 1 failures out of 13 attempts
ardbiesheuvel commented 1 month ago

@makubacki @mdkinney This PR ~fails~was failing the uncrustify checks but the attachments are missing from the test results. Please advise how to determine why these checks failed from the CI web UI.

I'm not 100% sure why the attachment is not in the test results, it might be related to audit mode being enabled for uncrustify in ArmPkg. Which means the test was skipped and therefore it may not be shown. I see it in test results UI for other packages that do not have Uncrustify in audit mode. Audit mode also means Uncrustify didn't contribute to the CI failure, it was EccCheck from the ECC errors.

No, this is not the case. Both EccCheck and Uncrustify failed, but not on a file in ArmPkg so the AuditMode has not bearing on this.

Can you please clarify? Are you saying you don't think AuditMode impacted the test results being shown in the UI?

Yes, because the example I am referring to was about a file in ArmVirtPkg not ArmPkg. If you look at the conversation log, you will notice that after the comment about the missing attachment I rebased the branch but also updated it.

I will look at that in more detail, I just can't do so today.

Sure, no rush as far as I am concerned. I figured out in the mean time what the issue was with those particular files. But it would be nice to have that functionality restored.

I created a test PR here: https://github.com/tianocore/edk2/pull/5956

ardbiesheuvel commented 1 month ago

The below looks like a separate anomaly:

EccCheck failed with two errors in ArmPkg:

ERROR - EFI coding style error
ERROR - *Error code: 9001
ERROR - *The file headers should follow Doxygen special documentation blocks in section 2.3.5
ERROR - *file: D:\a\1\s\Build\.pytool\Plugin\EccCheck\ArmPkg\Library\ArmPsciResetSystemLib\ArmPsciResetSystemLib.inf
ERROR - *Line number: 1
ERROR - *Header comment section must have copyright information
ERROR - 
ERROR - EFI coding style error
ERROR - *Error code: 9001
ERROR - *The file headers should follow Doxygen special documentation blocks in section 2.3.5
ERROR - *file: D:\a\1\s\Build\.pytool\Plugin\EccCheck\ArmPkg\Library\ArmPsciResetSystemLib\ArmPsciResetSystemLib.inf
ERROR - *Line number: 1
ERROR - *Header comment section must have Abstract information.
ERROR - --->Test Failed: EccCheck Test NO-TARGET returned 1
PROGRESS - --Running ArmPkg: Guid Check Test NO-TARGET --

The file has this

## @file
#  Arm Monitor Library that chooses the conduit based on the PSCI node in the
#  device tree provided by QEMU.
#
#  Copyright (c) 2022, Arm Limited. All rights reserved.<BR>
#  Copyright (c) 2024, Google LLC. All rights reserved.<BR>
#
#  SPDX-License-Identifier: BSD-2-Clause-Patent
##

and the same PR was tested multiple times, and this is the first time this bit is flagged as an error. The patch in question is identical, but I did rebase it in the mean time.

Any clue what is going on here? I created another test PR #5958 which has the patch in question in isolation. The tests are still running but I wonder if EccCheck will produce the same error, as the patch is identical to the one in this series.

mergify[bot] commented 1 month ago

PR can not be merged due to conflict. Please rebase and resubmit