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.w...@intel.com>
Cc: Hao Wu <hao.a...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Julien Grall <julien.gr...@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
.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
