Re: [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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