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

["Laszlo Ersek" <lersek@redhat.com>]

On 9/8/23 01:58, Sean Brogan wrote:
>
> 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)
> <https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SerialPortLib.h> 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.

I've given this more thought. I think the problem cannot be solved 100%
generally in BaseDebugLibSerialPort.

        Lib instance constructors   Lib instance constructors
        called manually             called automatically

No RAM  SEC                         PEIM
        PEI_CORE

RAM     DXE_CORE                    DXE_DRIVER
        SMM_CORE                    DXE_SMM_DRIVER
        MM_CORE_STANDALONE          DXE_RUNTIME_DRIVER
                                    UEFI_DRIVER
                                    UEFI_APPLICATION
                                    SMM_DRIVER
                                    MM_STANDALONE

(The SEC, PEI_CORE and PEIM types may also run from RAM, dependent on
platform, but in their strictest definition, they may execute from
flash; i.e., with no writable global variables.)

The lower right corner (RAM + ctors) is no problem; the DebugLib
instance constructor calls SerialPortInitialize(); done.

The lower left corner (RAM + no ctors) can be solved in the DebugLib
instance. Just before we call SerialPortWrite() every time, check a
global boolean. If the variable is FALSE, call SerialPortInitialize(),
set the variable to TRUE, and then call SerialPortWrite(). Otherwise,
only call SerialPortWrite().

The upper right corner (No RAM + ctors) is where it starts getting
messy. In the DebugLib instance, we have no way to track whether
SerialPortInitialize() was called. Thankfully, we don't have to: we can
call it from the constructor, guaranteeing exactly 1 call to it, before
the other APIs are used. The underlying SerialPortLib can then
initialize the hardware if necessary. If the SerialPortLib instance
would like to cache various things in RAM, then it can't (no RAM!), but
that's OK, the SerialPortLib instance has to deal with the problem
internally, after all it declared itself PEIM-ready. This gets hackish,
because we need to starting thinking about SerialPortLib internals in
DebugLib, but ultimately it's OK.

The upper left corner is the wreck. No RAM and no constructors. So in
DebugLib, we cannot track SerialPortInitialize() *at all*. Either we
don't call it ever (which is a library interface violation), or we call
it before every SerialPortWrite() call (which is *also* a library
interface violation). A SEC-compatible SerialPortLib instance may very
well exist that performs such actions in SerialPortInitialize() that
must be performed *exactly once* during boot. And there's no way to be
compatible with such a SerialPortLib instance from a DebugLib instance
that runs from flash and cannot rely on its constructor being called.

So the upper left corner is unsolvable if we respect library class
(responsibility) boundaries. In that corner, the DebugLib instance would
have to assume either that (a) the SerialPortLib instance needed zero
calls to SerialPortInitialize(), or that (b) the SerialPortLib instance
tolerated infinite calls to SerialPortInitialize().

Currently, the right side column has no problem in edk2. The left side
column is buggy. I'd like to propose patches for fixing the lower left
corner, but the upper left corner is unfixable (in MdePkg anyway).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109234): https://edk2.groups.io/g/devel/message/109234
Mute This Topic: https://groups.io/mt/101203427/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-