[edk2-devel] [PATCH v1 1/1] ArmVirtPkg: Enable Early Serial For DxeCore

[Laszlo Ersek]

On 9/7/23 19:50, Sean Brogan wrote:
> 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.

Good point, but then I still claim that the "init check in each API"
should be done in a dedicated "DxeCoreDebugLibSerialPort" instance, and
not in a SerialPortLib instance. Here's why:

(1) The SerialPortLib *class* requires SerialPortInitialize() to be
called before the other APIs. The FdtPL011SerialPortLib instance does
nothing in its implementation of that function, because it relies on the
constructor doing the same work. Therefore I agree that
FdtPL011SerialPortLib is not suitable for DXE_CORE, and I would suggest
removing DXE_CORE from LIBRARY_CLASS in the INF file, after the pipe
sign ("|").

(2) A new SerialPortLib instance should be added, very similar to
FdtPL011SerialPortLib -- the difference should be that it should have no
constructor, and the same job should be done in SerialPortInitialize().
This library instance sould be suitable for *direct use* in DXE_CORE
(and should likely be restricted to DXE_CORE exclusively).

The reason for that is the following. The DXE Core is entitled to
consume a lib instance without calling its constructor, in case the lib
instance declares itself DXE_CORE-capable (this is your argument). (In
fact such a lib instance is not supposed to have a constructor at all --
it might not be called anyway.) However, the DXE Core is *not* entitled
to ignore library *class* restrictions, and an explicit call to
SerialPortInitialize() is required by the SerialPortLib *class*. IOW, if
the DXE Core ever wanted to use SerialPortLib *directly*, it would have
to call SerialPortInitialize() before calling the other SerialPortLib
APIs, regardless of where and when the DXE Core ran the library
constructor list.

So that's why such a new FdtPL011SerialPortLib variant would be proper
for DXE_CORE.

(3) In turn, the new DxeCoreDebugLibSerialPort instance -- which would
have no constructor -- would be responsible for tracking in each API
implementation whether SerialPortInitialize() had been called before.

(4) This also means that the current BaseDebugLibSerialPort in MdePkg is
unsuitable for DXE_CORE usage, and so its LIBRARY_CLASS module type list
should be made explicit -- it should *exclude* the DXE_CORE. Even though
BaseDebugLibSerialPort has a BASE type entry point, this lib instance
relies on having a constructor (where it calls SerialPortInitialize()!),
and that rules it out for DXE_CORE usage.


IOW, I agree with you; my point is only that the serial init tracking
belongs in a new DebugLib instance (because, at the *class* level,
DebugLib permits the DXE_CORE to call its APIs in any order; whereas
SerialPortLib requires SerialPortInitialize() to be called first, also
at the *class* level).

Laszlo


> 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 (#108418): https://edk2.groups.io/g/devel/message/108418
Mute This Topic: https://groups.io/mt/101203427/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-