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. View/Reply Online (#108426): https://edk2.groups.io/g/devel/message/108426 Mute This Topic: https://groups.io/mt/101203427/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-