Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo <guoheyi@huawei.com>]

On 2019/2/28 21:39, Laszlo Ersek wrote:
> Hello Heyi,
>
> On 02/28/19 09:05, Heyi Guo wrote:
>> Serial port output is useful when debugging UEFI runtime services in OS runtime.
>> The patches are trying to provide a handy method to enable runtime serial port
>> debug for ArmVirtQemu.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>>
>> Heyi Guo (3):
>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>    ArmVirtQemu: enable runtime debug by build flag
>>
>>   ArmVirtPkg/ArmVirt.dsc.inc                                                          |   6 ++
>>   ArmVirtPkg/ArmVirtQemu.dsc                                                          |  13 +++
>>   ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                |   5 +
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c                    |  29 ++++--
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h                    |  27 +++++
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf                  |   3 +
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c              |  27 +++++
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c             | 104 ++++++++++++++++++++
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf           |  58 +++++++++++
>>   MdeModulePkg/MdeModulePkg.dec                                                       |   6 ++
>>   MdeModulePkg/MdeModulePkg.uni                                                       |   6 ++
>>   MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c   |   2 +-
>>   MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf |   7 +-
>>   13 files changed, 279 insertions(+), 14 deletions(-)
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
>>
> I'm quite swamped, so only a few general comments for now.
>
> I'll let the MdeModulePkg maintainers comment on the first patch.
>
> (1) The general idea to utilize
>
> - MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode
> - MdePkg/Library/DxeRuntimeDebugLibSerialPort
>
> seems mostly OK to me.
>
> Although, I think I would actually prefer the reverse implementation (=
> don't send debug messages to the status code infrastructure; instead,
> process status codes by logging them with DEBUG). I don't think there is
> a status code handler already present in edk2 for that however.
>
>
> (2) The use case is too generic. Until very recently, we hadn't enabled
> status code routing and reporting in ArmVirtPkg at all. We only did that
> recently, in commit 5c574b222ec2 ("ArmVirtPkg/ArmVirtQemu*: enable
> minimal Status Code Routing in DXE", 2019-02-25), because there was no
> other way to implement the boot progress reporting that I had been
> after. (See commit 1797f32e0a19 ("ArmVirtPkg/PlatformBootManagerLib:
> display boot option loading/starting", 2019-02-25).)
>
> So, in the proposal, the specific use should be described. What runtime
> module do you want status codes from?
>
> And why do you prefer status codes to actual (runtime) debug messages?
>
> ... Are you perhaps using this approach only because it lets you print
> DEBUGs at runtime? I.e., do you use status codes only as a "runtime
> carrier" for DEBUGs?
That's true:) StatusCode is only a carrier. We can see lots of X86 platforms use StatusCode driver for DEBUG serial print.

>
>
> (3) Patch #2 ("ArmVirtPkg: add runtime instance of
> FdtPL011SerialPortLib") looks confusing. It introduces a file called
> "FdtPL011SerialPortLibCommon.c", which doesn't seem common to multiple
> INF files. It is added only to one file.
Yes, the file name is not good. It only contains a null hook to build a non-runtime instance. I didn't get a proper name for that. Maybe RuntimeDummy or NoRuntime?
>
> Also, the patch changes the behavior of the current lib instance with
> regard to the SerialPortInitialize() function and the library
> constructor. I don't see why that is necessary just for adding a runtime
> instance. The runtime instance should not affect the behavior of the
> existent instance.
>
> Sorry if I misunderstood; a more detailed commit message would be nice.
Ah, this is a mistake. In current implementation we can still enable the constructor.

The long story is: at first I proposed to still use BaseSerialPortDebugLib, so I added RuntimePrepare() function into the constructor directly, but the builder complained about looped constructors, for RuntimePrepare() invokes gBS and some RunTime libraries. Then I tried disabling the constructor and moved RuntimePrepare() into SerialPortInitialize() which could complete the build, but still couldn't guarantee the calling order of these constructors, for BaseSerialPortDebugLib has a constructor to call SerialPortInitialize(). Finally I switched to the current implementation, but forgot to revert previous changes...

>
>
> (4) What's most worrying is that this change would lead to an unexpected
> sharing of the PL011 device between the OS and the firmware. I don't
> think that's a great idea, especially if QEMU's ACPI payload explicitly
> advertises the port to the OS.
That's true, so I propose to disable the feature by default. It is only used for UEFI runtime code debug. It is always painful when we don't a handy debug tool for runtime services code.
>
> (On x86/OVMF, the above problems don't surface. There, DEBUG messages
> are written to the QEMU debug IO port. The debug IO port is neither used
> by the OS kernel (so no race), nor affected by page tables (the IO port
> space is absolute). Hence no runtime specific processing.)
We have not real port IO on ARM, so I think address conversion is always needed. Also I think this can be a feasible template for physical ARM platforms, for it does not require additional hardware components.

Thanks,
Heyi

>
> Thanks
> Laszlo
>
> .
>