I would argue that by declaring that your library class supports type
DXE_CORE (or any core type) that you have declared you understand the
uniqueness of the environment and have accounted for it.
For instances that support DXE_CORE or MM_CORE module types we often use
a global variable (to the library) to determine if the init routine has
been completed. This does require a single byte check on each serial
message write (hot path) but given all the code on that path this is not
noticeable in performance measurements. In the case below this pattern
could be used by the FdtPL011SerialPortLib to detect if it's init
routine has been called.
Thanks
Sean
On 9/7/2023 8:24 AM, Oliver Smith-Denny wrote:
On 9/7/2023 6:10 AM, Laszlo Ersek wrote:
(replying on the webui... sorry!)
The problem is actually embedded in MdePkg and MdeModulePkg.
- In DxeMain() (and in functions called by DxeMain()), we call
DebugLib APIs *before* reaching ProcessLibraryConstructorList().
- In ArmVirtQemu, we resolve the DXE Core's DebugLib dependency to
BaseDebugLibSerialPort (from MdePkg).
- BaseDebugLibSerialPort has a constructor function
(BaseDebugLibSerialPortConstructor()).
These already suffice for broken DebugLib behavior. APIs of a library
class should not be called because the library instance has a chance
to initialize.
The rest is circumstantial. Like, BaseDebugLibSerialPortConstructor
calls SerialPortInitialize, but our SerialPortInitialize (in
FdtPL011SerialPortLib) does nothing. Well, the latter doesn't need to
do anything, because FdtPL011SerialPortLib has its own constructor
(FdtPL011SerialPortLibInitialize), thus, if constructors were called
properly, then BaseDebugLibSerialPort + FdtPL011SerialPortLib would
work properly together, regardless of SerialPortInitialize being
empty in the latter.
Basically the DXE Core has a hidden requirement -- it can only use
such DebugLib instances that need no explicit initialization.
The proposed patch works around the problem by satisfying that hidden
requirement one level lower down: in the SerialPortLib instance. The
initialization of BaseDebugLibSerialPort is still busted (its
constructor is not called, so it cannot call SerialPortInitialize
either), but now it is masked, because EarlyFdtPL011SerialPortLib
works withouth *both* SerialPortInitialize and construction.
The real fix would be to make the DXE Core requirement explicit, by
introducing separate (dedicated) DebugLib and SerialPortLib *classes*
(whose APIs are guaranteed to work without initialization).
Laszlo
Thanks for the comprehensive breakdown! :). I completely agree that
fixing this at the upper level (and ideally documenting this
requirement) is the better move.
I can drop this patch and take a crack at that. I'm in the last few
weeks leading up to an extended parental leave, so we'll see if I can
squeeze it in prior to then :).
Oliver
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108408): https://edk2.groups.io/g/devel/message/108408
Mute This Topic: https://groups.io/mt/101203427/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
