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

[Laszlo Ersek <lersek@redhat.com>]

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?


(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.

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.


(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.


(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.)

Thanks
Laszlo