On 9/7/2023 1:54 PM, Laszlo Ersek wrote:

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.

Where do you see this? Looking at the file here: edk2/MdePkg/Include/Library/SerialPortLib.h at master · tianocore/edk2 (github.com) I don't see that. I don't necessarily disagree with you but I am just trying to find out where this is documented.

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

Agree


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

Agree


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

Agree



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

Just for discussions sake you could also imagine a solution where the "base" instance does init tracking and then a new library instance is used only for XIP PEI that executes from RO memory (flash or otherwise). Also note that this isn't just a DXE_CORE problem. SEC, PEI_CORE, MM_CORE_STANDALONE and SMM_CORE types may have these similar restrictions.

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.

_._,_._,_