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

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

[Oliver Smith-Denny]

Currently, ArmVirtPkg does not provide a serial library for DxeCore,
so any early prints are missed. These prints are extremely valuable
for debugging.

The early serial port lib used by PeiCore and PEIMs is also
applicable to DxeCore and in testing works to print debug prints
from DxeCore throughout its lifecycle.

This patchset adds the indicated support for DXE_CORE to
EarlyFdtPL011SerialPortLib and adds this as the serial port
instance for DxeCore in ArmVirtPkg.

Github PR: https://github.com/tianocore/edk2/pull/4793

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 ArmVirtPkg/ArmVirt.dsc.inc                                              | 1 +
 ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 2443e8351c99..cf352619fd6e 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -225,6 +225,7 @@ [LibraryClasses.common.DXE_CORE]
   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
   ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
+  SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
 
 [LibraryClasses.common.DXE_DRIVER]
   SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
index 32b2d337d412..2c22ab088033 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
@@ -14,7 +14,7 @@ [Defines]
   FILE_GUID                      = 0983616A-49BC-4732-B531-4AF98D2056F0
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = SerialPortLib|SEC PEI_CORE PEIM
+  LIBRARY_CLASS                  = SerialPortLib|SEC PEI_CORE PEIM DXE_CORE
 
 [Sources.common]
   EarlyFdtPL011SerialPortLib.c
-- 
2.40.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108342): https://edk2.groups.io/g/devel/message/108342
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

On Wed, 6 Sept 2023 at 23:49, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Currently, ArmVirtPkg does not provide a serial library for DxeCore,
> so any early prints are missed. These prints are extremely valuable
> for debugging.
>
> The early serial port lib used by PeiCore and PEIMs is also
> applicable to DxeCore and in testing works to print debug prints
> from DxeCore throughout its lifecycle.
>
> This patchset adds the indicated support for DXE_CORE to
> EarlyFdtPL011SerialPortLib and adds this as the serial port
> instance for DxeCore in ArmVirtPkg.
>
> Github PR: https://github.com/tianocore/edk2/pull/4793
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
>
> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>

Thanks for the patch. I agree that omitting early DXE core DEBUG
output is not great.

However, I'd like to understand a bit better why this happens.

We have the PlatformPeim that discovers the UART base address and
records it in a GUIDed HOB 'gEarlyPL011BaseAddressGuid'. So when DXE
core launches, we already have the information we need stored
somewhere, and using the 'early' flavor of the PL011 serialportlib
that parses the DT for every line (or character?) printed seems
unnecessary. Maybe it is a matter of tweaking the logic in
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c (for
DEBUG builds only) to take the HOB into account even before the
constructor has been called?



> ---
>  ArmVirtPkg/ArmVirt.dsc.inc                                              | 1 +
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 2443e8351c99..cf352619fd6e 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -225,6 +225,7 @@ [LibraryClasses.common.DXE_CORE]
>    DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>    ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>    PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
> +  SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
>
>  [LibraryClasses.common.DXE_DRIVER]
>    SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
> index 32b2d337d412..2c22ab088033 100644
> --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
> @@ -14,7 +14,7 @@ [Defines]
>    FILE_GUID                      = 0983616A-49BC-4732-B531-4AF98D2056F0
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = SerialPortLib|SEC PEI_CORE PEIM
> +  LIBRARY_CLASS                  = SerialPortLib|SEC PEI_CORE PEIM DXE_CORE
>
>  [Sources.common]
>    EarlyFdtPL011SerialPortLib.c
> --
> 2.40.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108372): https://edk2.groups.io/g/devel/message/108372
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

(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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108386): https://edk2.groups.io/g/devel/message/108386
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 2713 bytes --]

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

[Oliver Smith-Denny]

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 (#108402): https://edk2.groups.io/g/devel/message/108402
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sean]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

[Sean]

[-- Attachment #1: Type: text/plain, Size: 7128 bytes --]


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.


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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 9155 bytes --]

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

[Laszlo Ersek]

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. 

I consider this an implicit requirement, from the first two sentences
(and from the fact that the function is declared at the top of the
header) -- "Initialize the serial device hardware. If no initialization
is required, then return RETURN_SUCCESS."

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108475): https://edk2.groups.io/g/devel/message/108475
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

On Mon, 11 Sept 2023 at 07:30, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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.
>
> I consider this an implicit requirement, from the first two sentences
> (and from the fact that the function is declared at the top of the
> header) -- "Initialize the serial device hardware. If no initialization
> is required, then return RETURN_SUCCESS."
>

Agreed. Even if the header file does not spell it out literally, it is
implied: SerialPortInitialize() needs to be implemented even if no
initialization is required, in which case the routine does nothing
before returning SUCCESS. So the caller cannot distinguish a library
class implementation that requires initialization from one that does
not, and it can only assume that calling SerialPortInitialize() is
needed before using any other part of the API.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108481): https://edk2.groups.io/g/devel/message/108481
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 9/30/23 16:43, Laszlo Ersek wrote:
> 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().

Unfortunately, even this (i.e., the DXE_CORE case) proves intractable in
practice. There are several SerialPortLib instances that, regardless of
whether they do anything in SerialPortInitialize(), have constructors,
adn claim themselves DXE_CORE-ready.

Fixing these SerialPortLib instances is just not practical. One thing is
to move or copy the initialization logic from the constructor to
SerialPortInitialize(). The larger problem is that these SerialPortLib
instances have *further* dependencies, which *also* have constructors.
So if I remove the constructor from SerialPortLib totally, that breaks
the topological sorting of constructors even in other types of modules.
If I keep the logic in both SerialPortInitialize() and the constructor,
then the constructors of the further library dependencies are still not
called from SerialPortInitialize().

So all in all this is a train wreck, it's so pervasive and recursive
that it can't be fixed centrally; it has such a ripple effect.

What we can do is copy BaseDebugLibSerialPort to ArmVirtPkg, and fix it
up there (call SerialPortInitialize() as needed), in addition to
exposing the initialization logic of FdtPL011SerialPortLibInitialize()
in SerialPortInitialize() as well.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109235): https://edk2.groups.io/g/devel/message/109235
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]
-=-=-=-=-=-=-=-=-=-=-=-