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

["Sean" <spbrogan@outlook.com>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-