Re: [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek <lersek@redhat.com>]

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>