[PATCH] ArmVirtPkg: remove status code support

[PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
from EmbeddedPkg with the well maintained generic alternative that lives
in MdeModulePkg.

However, as it turns out, the generic driver has a dependency on the
library class ReportStatusCodeLib, whose default resolution is an
implementation that is not safe for use at runtime, resulting in crashes
when trying to invoke it from the OS.

Since we have no use for status codes in any of the ArmVirtPkg
platforms, let's replace all resolutions with a common one to the NULL
implementation.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 0b9b457b5619..bfc40286d7ab 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -154,6 +154,8 @@ [LibraryClasses.common]
   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
   UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
 
+  ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
+
 [LibraryClasses.common.SEC]
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
@@ -174,7 +176,6 @@ [LibraryClasses.common.PEI_CORE]
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
   PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
   PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
-  ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
   UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
@@ -191,7 +192,6 @@ [LibraryClasses.common.PEIM]
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
   PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
   PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
-  ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
   PeiResourcePublicationLib|MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.inf
@@ -205,13 +205,11 @@ [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
   MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
-  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
   PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
 
 [LibraryClasses.common.DXE_DRIVER]
-  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
   PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -223,10 +221,8 @@ [LibraryClasses.common.UEFI_APPLICATION]
   HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
   ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
   FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
-  ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
 
 [LibraryClasses.common.UEFI_DRIVER]
-  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
   ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
@@ -234,7 +230,6 @@ [LibraryClasses.common.UEFI_DRIVER]
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
-  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
@@ -334,8 +329,6 @@ [PcdsFixedAtBuild.common]
   #  DEBUG_ERROR     0x80000000  // Error
   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|$(DEBUG_PRINT_ERROR_LEVEL)
 
-  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
-
   #
   # Optional feature to help prevent EFI memory map fragments
   # Turned on and off via: PcdPrePiProduceMemoryTypeInformationHob
-- 
2.9.3

Re: [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek]

On 07/05/17 15:04, Ard Biesheuvel wrote:
> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
> from EmbeddedPkg with the well maintained generic alternative that lives
> in MdeModulePkg.
> 
> However, as it turns out, the generic driver has a dependency on the
> library class ReportStatusCodeLib, whose default resolution is an
> implementation that is not safe for use at runtime, resulting in crashes
> when trying to invoke it from the OS.
> 
> Since we have no use for status codes in any of the ArmVirtPkg
> platforms, let's replace all resolutions with a common one to the NULL
> implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Alternative approach (if we wish to follow what OVMF does):

(1) Copy the library resolutions (as appropriate) from OvmfPkg:

- SEC, PEI_CORE, PEIM:
  MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf

- DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
  MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

- DXE_RUNTIME_DRIVER:
  MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf

(2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.

This should result in status code reporting and handling that is functional at runtime as well.

What do you prefer?

Thanks
Laszlo

> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 0b9b457b5619..bfc40286d7ab 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -154,6 +154,8 @@ [LibraryClasses.common]
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>    UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
>  
> +  ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
> +
>  [LibraryClasses.common.SEC]
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>    BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> @@ -174,7 +176,6 @@ [LibraryClasses.common.PEI_CORE]
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>    PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
>    PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
> -  ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>    UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
> @@ -191,7 +192,6 @@ [LibraryClasses.common.PEIM]
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>    PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
>    PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
> -  ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>    PeiResourcePublicationLib|MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.inf
> @@ -205,13 +205,11 @@ [LibraryClasses.common.DXE_CORE]
>    HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>    MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
>    DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>    ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>    UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
>    PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
>  
>  [LibraryClasses.common.DXE_DRIVER]
> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>    SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -223,10 +221,8 @@ [LibraryClasses.common.UEFI_APPLICATION]
>    HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
>    ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
>    FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> -  ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>  
>  [LibraryClasses.common.UEFI_DRIVER]
> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>    UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
>    ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
> @@ -234,7 +230,6 @@ [LibraryClasses.common.UEFI_DRIVER]
>  
>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>    CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
> @@ -334,8 +329,6 @@ [PcdsFixedAtBuild.common]
>    #  DEBUG_ERROR     0x80000000  // Error
>    gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|$(DEBUG_PRINT_ERROR_LEVEL)
>  
> -  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> -
>    #
>    # Optional feature to help prevent EFI memory map fragments
>    # Turned on and off via: PcdPrePiProduceMemoryTypeInformationHob
> 

Re: [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 14:43, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/05/17 15:04, Ard Biesheuvel wrote:
>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>> from EmbeddedPkg with the well maintained generic alternative that lives
>> in MdeModulePkg.
>>
>> However, as it turns out, the generic driver has a dependency on the
>> library class ReportStatusCodeLib, whose default resolution is an
>> implementation that is not safe for use at runtime, resulting in crashes
>> when trying to invoke it from the OS.
>>
>> Since we have no use for status codes in any of the ArmVirtPkg
>> platforms, let's replace all resolutions with a common one to the NULL
>> implementation.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> Alternative approach (if we wish to follow what OVMF does):
>
> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>
> - SEC, PEI_CORE, PEIM:
>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>
> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> - DXE_RUNTIME_DRIVER:
>   MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>
> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>
> This should result in status code reporting and handling that is functional at runtime as well.
>
> What do you prefer?
>
> Thanks
> Laszlo
>
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index 0b9b457b5619..bfc40286d7ab 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -154,6 +154,8 @@ [LibraryClasses.common]
>>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>>    UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
>>
>> +  ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
>> +
>>  [LibraryClasses.common.SEC]
>>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>>    BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>> @@ -174,7 +176,6 @@ [LibraryClasses.common.PEI_CORE]
>>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>>    PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
>>    PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
>> -  ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>>    PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>>    UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
>> @@ -191,7 +192,6 @@ [LibraryClasses.common.PEIM]
>>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>>    PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
>>    PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
>> -  ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>>    PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>>    PeiResourcePublicationLib|MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.inf
>> @@ -205,13 +205,11 @@ [LibraryClasses.common.DXE_CORE]
>>    HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>>    MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
>>    DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>>    ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>>    UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
>>    PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
>>
>>  [LibraryClasses.common.DXE_DRIVER]
>> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>>    SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
>>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
>>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>> @@ -223,10 +221,8 @@ [LibraryClasses.common.UEFI_APPLICATION]
>>    HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
>>    ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
>>    FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
>> -  ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>
>>  [LibraryClasses.common.UEFI_DRIVER]
>> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>>    UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
>>    ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
>> @@ -234,7 +230,6 @@ [LibraryClasses.common.UEFI_DRIVER]
>>
>>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>> -  ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>>    CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>>
>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>> @@ -334,8 +329,6 @@ [PcdsFixedAtBuild.common]
>>    #  DEBUG_ERROR     0x80000000  // Error
>>    gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|$(DEBUG_PRINT_ERROR_LEVEL)
>>
>> -  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>> -
>>    #
>>    # Optional feature to help prevent EFI memory map fragments
>>    # Turned on and off via: PcdPrePiProduceMemoryTypeInformationHob
>>
>

Re: [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 5 July 2017 at 14:43, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>> in MdeModulePkg.
>>>
>>> However, as it turns out, the generic driver has a dependency on the
>>> library class ReportStatusCodeLib, whose default resolution is an
>>> implementation that is not safe for use at runtime, resulting in crashes
>>> when trying to invoke it from the OS.
>>>
>>> Since we have no use for status codes in any of the ArmVirtPkg
>>> platforms, let's replace all resolutions with a common one to the NULL
>>> implementation.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> Alternative approach (if we wish to follow what OVMF does):
>>
>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>
>> - SEC, PEI_CORE, PEIM:
>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>
>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>
>> - DXE_RUNTIME_DRIVER:
>>   MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>
>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>
>> This should result in status code reporting and handling that is functional at runtime as well.
>>
>> What do you prefer?
>>


Depends on what status codes actually buy us. I'm clueless

Re: [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek]

On 07/05/17 15:46, Ard Biesheuvel wrote:
> On 5 July 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 5 July 2017 at 14:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>>> in MdeModulePkg.
>>>>
>>>> However, as it turns out, the generic driver has a dependency on the
>>>> library class ReportStatusCodeLib, whose default resolution is an
>>>> implementation that is not safe for use at runtime, resulting in crashes
>>>> when trying to invoke it from the OS.
>>>>
>>>> Since we have no use for status codes in any of the ArmVirtPkg
>>>> platforms, let's replace all resolutions with a common one to the NULL
>>>> implementation.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>>
>>> Alternative approach (if we wish to follow what OVMF does):
>>>
>>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>>
>>> - SEC, PEI_CORE, PEIM:
>>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>>
>>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>
>>> - DXE_RUNTIME_DRIVER:
>>>   MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>>
>>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>>
>>> This should result in status code reporting and handling that is functional at runtime as well.
>>>
>>> What do you prefer?
>>>
> 
> 
> Depends on what status codes actually buy us. I'm clueless
> 

I was similarly clueless :) which is why I asked Cinnamon back then [*]
to write a detailed commit message for what would become commit
a6d594c5fabd.

[*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html

So, if you read that message, you'll see that roughly, it buys us the
following in practice: when a module (boot time or runtime) calls one of
the REPORT_STATUS_CODE*() macros, a status code message will be printed
to the serial port.

That's it :)

I don't think it's extremely useful in practice for in-tree modules,
given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
modules with high DEBUG levels, add DEBUGs liberally to our own platform
code, and always capture the DEBUG output (serial port or QEMU debug
port) during boot.

For working with out-of-tree modules that rely heavily on status code
reporting, or else just to showcase/test the related PPIs and protocols
(which is arguably one of the goals of the in-tree virt platforms),
enabling the status code libs and drivers can be considered useful.

Given that OvmfPkg is already serving this showcase/test purpose, I'm
fine with disabling status code stuff in ArmVirtPkg for good; I just
wanted you to consider the alternative. So, what say you? :)

Thanks
Laszlo

Re: [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 15:25, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/05/17 15:46, Ard Biesheuvel wrote:
>> On 5 July 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 5 July 2017 at 14:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>>>> in MdeModulePkg.
>>>>>
>>>>> However, as it turns out, the generic driver has a dependency on the
>>>>> library class ReportStatusCodeLib, whose default resolution is an
>>>>> implementation that is not safe for use at runtime, resulting in crashes
>>>>> when trying to invoke it from the OS.
>>>>>
>>>>> Since we have no use for status codes in any of the ArmVirtPkg
>>>>> platforms, let's replace all resolutions with a common one to the NULL
>>>>> implementation.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>>>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>>>
>>>> Alternative approach (if we wish to follow what OVMF does):
>>>>
>>>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>>>
>>>> - SEC, PEI_CORE, PEIM:
>>>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>>>
>>>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>
>>>> - DXE_RUNTIME_DRIVER:
>>>>   MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>>>
>>>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>>>
>>>> This should result in status code reporting and handling that is functional at runtime as well.
>>>>
>>>> What do you prefer?
>>>>
>>
>>
>> Depends on what status codes actually buy us. I'm clueless
>>
>
> I was similarly clueless :) which is why I asked Cinnamon back then [*]
> to write a detailed commit message for what would become commit
> a6d594c5fabd.
>
> [*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html
>
> So, if you read that message, you'll see that roughly, it buys us the
> following in practice: when a module (boot time or runtime) calls one of
> the REPORT_STATUS_CODE*() macros, a status code message will be printed
> to the serial port.
>
> That's it :)
>
> I don't think it's extremely useful in practice for in-tree modules,
> given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
> modules with high DEBUG levels, add DEBUGs liberally to our own platform
> code, and always capture the DEBUG output (serial port or QEMU debug
> port) during boot.
>
> For working with out-of-tree modules that rely heavily on status code
> reporting, or else just to showcase/test the related PPIs and protocols
> (which is arguably one of the goals of the in-tree virt platforms),
> enabling the status code libs and drivers can be considered useful.
>
> Given that OvmfPkg is already serving this showcase/test purpose, I'm
> fine with disabling status code stuff in ArmVirtPkg for good; I just
> wanted you to consider the alternative. So, what say you? :)
>

Thanks for the explanation. I think one showcase is sufficient, indeed.

Re: [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek]

On 07/05/17 17:01, Ard Biesheuvel wrote:
> On 5 July 2017 at 15:25, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 07/05/17 15:46, Ard Biesheuvel wrote:
>>> On 5 July 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 5 July 2017 at 14:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>>>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>>>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>>>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>>>>> in MdeModulePkg.
>>>>>>
>>>>>> However, as it turns out, the generic driver has a dependency on the
>>>>>> library class ReportStatusCodeLib, whose default resolution is an
>>>>>> implementation that is not safe for use at runtime, resulting in crashes
>>>>>> when trying to invoke it from the OS.
>>>>>>
>>>>>> Since we have no use for status codes in any of the ArmVirtPkg
>>>>>> platforms, let's replace all resolutions with a common one to the NULL
>>>>>> implementation.
>>>>>>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>> ---
>>>>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>>>>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>>>>
>>>>> Alternative approach (if we wish to follow what OVMF does):
>>>>>
>>>>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>>>>
>>>>> - SEC, PEI_CORE, PEIM:
>>>>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>>>>
>>>>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>>>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>>
>>>>> - DXE_RUNTIME_DRIVER:
>>>>>   MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>>>>
>>>>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>>>>
>>>>> This should result in status code reporting and handling that is functional at runtime as well.
>>>>>
>>>>> What do you prefer?
>>>>>
>>>
>>>
>>> Depends on what status codes actually buy us. I'm clueless
>>>
>>
>> I was similarly clueless :) which is why I asked Cinnamon back then [*]
>> to write a detailed commit message for what would become commit
>> a6d594c5fabd.
>>
>> [*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html
>>
>> So, if you read that message, you'll see that roughly, it buys us the
>> following in practice: when a module (boot time or runtime) calls one of
>> the REPORT_STATUS_CODE*() macros, a status code message will be printed
>> to the serial port.
>>
>> That's it :)
>>
>> I don't think it's extremely useful in practice for in-tree modules,
>> given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
>> modules with high DEBUG levels, add DEBUGs liberally to our own platform
>> code, and always capture the DEBUG output (serial port or QEMU debug
>> port) during boot.
>>
>> For working with out-of-tree modules that rely heavily on status code
>> reporting, or else just to showcase/test the related PPIs and protocols
>> (which is arguably one of the goals of the in-tree virt platforms),
>> enabling the status code libs and drivers can be considered useful.
>>
>> Given that OvmfPkg is already serving this showcase/test purpose, I'm
>> fine with disabling status code stuff in ArmVirtPkg for good; I just
>> wanted you to consider the alternative. So, what say you? :)
>>
> 
> Thanks for the explanation. I think one showcase is sufficient, indeed.
> 

In that case :)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 16:07, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/05/17 17:01, Ard Biesheuvel wrote:
>> On 5 July 2017 at 15:25, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 07/05/17 15:46, Ard Biesheuvel wrote:
>>>> On 5 July 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>> On 5 July 2017 at 14:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>>>>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>>>>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>>>>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>>>>>> in MdeModulePkg.
>>>>>>>
>>>>>>> However, as it turns out, the generic driver has a dependency on the
>>>>>>> library class ReportStatusCodeLib, whose default resolution is an
>>>>>>> implementation that is not safe for use at runtime, resulting in crashes
>>>>>>> when trying to invoke it from the OS.
>>>>>>>
>>>>>>> Since we have no use for status codes in any of the ArmVirtPkg
>>>>>>> platforms, let's replace all resolutions with a common one to the NULL
>>>>>>> implementation.
>>>>>>>
>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>>> ---
>>>>>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++---------
>>>>>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> Alternative approach (if we wish to follow what OVMF does):
>>>>>>
>>>>>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>>>>>
>>>>>> - SEC, PEI_CORE, PEIM:
>>>>>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>>>>>
>>>>>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>>>>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>>>
>>>>>> - DXE_RUNTIME_DRIVER:
>>>>>>   MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>>>>>
>>>>>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>>>>>
>>>>>> This should result in status code reporting and handling that is functional at runtime as well.
>>>>>>
>>>>>> What do you prefer?
>>>>>>
>>>>
>>>>
>>>> Depends on what status codes actually buy us. I'm clueless
>>>>
>>>
>>> I was similarly clueless :) which is why I asked Cinnamon back then [*]
>>> to write a detailed commit message for what would become commit
>>> a6d594c5fabd.
>>>
>>> [*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html
>>>
>>> So, if you read that message, you'll see that roughly, it buys us the
>>> following in practice: when a module (boot time or runtime) calls one of
>>> the REPORT_STATUS_CODE*() macros, a status code message will be printed
>>> to the serial port.
>>>
>>> That's it :)
>>>
>>> I don't think it's extremely useful in practice for in-tree modules,
>>> given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
>>> modules with high DEBUG levels, add DEBUGs liberally to our own platform
>>> code, and always capture the DEBUG output (serial port or QEMU debug
>>> port) during boot.
>>>
>>> For working with out-of-tree modules that rely heavily on status code
>>> reporting, or else just to showcase/test the related PPIs and protocols
>>> (which is arguably one of the goals of the in-tree virt platforms),
>>> enabling the status code libs and drivers can be considered useful.
>>>
>>> Given that OvmfPkg is already serving this showcase/test purpose, I'm
>>> fine with disabling status code stuff in ArmVirtPkg for good; I just
>>> wanted you to consider the alternative. So, what say you? :)
>>>
>>
>> Thanks for the explanation. I think one showcase is sufficient, indeed.
>>
>
> In that case :)
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks!

Pushed as 59541d41633cf56e9b7c3ac0de112ab65d9331ca