| Message ID | 20170705130434.4525-1-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 59541d41633cf56e9b7c3ac0de112ab65d9331ca |
| Headers | show |
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
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(-) -- 2.9.3 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel