[edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly
Laszlo Ersek
lersek at redhat.com
Sun Oct 8 09:16:57 UTC 2023
On 10/6/23 19:08, Ard Biesheuvel wrote:
> On Mon, 2 Oct 2023 at 16:47, Laszlo Ersek <lersek at redhat.com> wrote:
>>
>> On 9/30/23 23:23, Laszlo Ersek wrote:
>>> FdtPL011SerialPortLib claims that it's usable from the DXE_CORE. That's
>>> not correct: the DXE_CORE calls DEBUG() and ASSERT() before it calls
>>> ProcessLibraryConstructorList(). Via the BaseDebugLibSerialPort instance,
>>> those DEBUG() and ASSERT() calls result in SerialPortWrite() calls, before
>>> ProcessLibraryConstructorList() called either our constructor
>>> FdtPL011SerialPortLibInitialize(), or BaseDebugLibSerialPortConstructor().
>>>
>>> (And even if the DXE_CORE called the latter function early enough, it
>>> would just invoke our SerialPortInitialize() function -- which does
>>> nothing.)
>>>
>>> This means that the earliest DXE_CORE debug messages are lost.
>>>
>>> Rename FdtPL011SerialPortLibInitialize() to SerialPortInitialize(), so
>>> that the same initialization occur through the constructor and the public
>>> SerialPortInitialize() library API.
>>>
>>> Turn SerialPortInitialize() calls after the first one into no-ops.
>>>
>>> Our SerialPortLib APIs already use (mSerialBaseAddress != 0) to track
>>> initialization. Rework those checks to actually initialize the library if
>>> that hasn't happened yet.
>>>
>>> The following new lines appear in the log:
>>>
>>>> CoreInitializeMemoryServices:
>>>> BaseAddress - 0x48000000 Length - 0xF8000000 MinimalMemorySizeNeeded - 0x38C8000
>>>> InstallProtocolInterface: [EfiLoadedImageProtocol] 46EFC3E0
>>>> ProtectUefiImageCommon - 0x46EFC3E0
>>>> - 0x0000000046EB2000 - 0x0000000000068000
>>>
>>> (0x46EB2000 is the load address of the DXE Core.)
>>>
>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>>> Cc: Leif Lindholm <quic_llindhol at quicinc.com>
>>> Cc: Oliver Smith-Denny <osde at linux.microsoft.com>
>>> Cc: Oliver Steffen <osteffen at redhat.com>
>>> Cc: Sami Mujawar <sami.mujawar at arm.com>
>>> Cc: Sean Brogan <sean.brogan at microsoft.com>
>>> Reported-by: Oliver Smith-Denny <osde at linux.microsoft.com>
>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>> ---
>>>
>>> Notes:
>>> In my opinion this is the wrong approach to fix the bug, but the right
>>> approach would require so much work all over edk2 (and outside of it)
>>> that it's just not practical.
>>
>> NB I'd still like this patch to be merged; "wrong approach" is too
>> strongly worded. I'd now refine that as "theoretically not 100% pure".
>> Merging this one is a compromise, but an easy one -- I had worked
>> several hours on the "pure approach", and it absolutely spiralled out of
>> control. So this is the only one practical thing to do.
>>
>
> Reviewed-by: Ard Biesheuvel <ardb at kernel.org>
Thanks!
>
> I'll queue this up.
... for that, too! (<https://github.com/tianocore/edk2/pull/4893>,
commit 5087a0773645.)
Laszlo
>
>
>>>
>>> ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf | 2 +-
>>> ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 84 ++++++++++++--------
>>> 2 files changed, 52 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
>>> index 2b9a34aa30d1..c417514e3ed5 100644
>>> --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
>>> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
>>> @@ -15,7 +15,7 @@ [Defines]
>>> MODULE_TYPE = BASE
>>> VERSION_STRING = 1.0
>>> LIBRARY_CLASS = SerialPortLib|DXE_CORE DXE_DRIVER UEFI_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION
>>> - CONSTRUCTOR = FdtPL011SerialPortLibInitialize
>>> + CONSTRUCTOR = SerialPortInitialize
>>>
>>> [Sources.common]
>>> FdtPL011SerialPortLib.c
>>> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
>>> index 614ea5a860b9..9d71f369570f 100644
>>> --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
>>> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
>>> @@ -23,49 +23,57 @@
>>> #include <Library/HobLib.h>
>>> #include <Guid/EarlyPL011BaseAddress.h>
>>>
>>> -STATIC UINTN mSerialBaseAddress;
>>> -
>>> -RETURN_STATUS
>>> -EFIAPI
>>> -SerialPortInitialize (
>>> - VOID
>>> - )
>>> -{
>>> - return RETURN_SUCCESS;
>>> -}
>>> +STATIC UINTN mSerialBaseAddress;
>>> +STATIC RETURN_STATUS mPermanentStatus = RETURN_SUCCESS;
>>>
>>> /**
>>> -
>>> Program hardware of Serial port
>>>
>>> - @return RETURN_NOT_FOUND if no PL011 base address could be found
>>> - Otherwise, result of PL011UartInitializePort () is returned
>>> + @retval RETURN_SUCCESS If the serial port was initialized successfully by
>>> + this call, or an earlier call, to
>>> + SerialPortInitialize().
>>>
>>> + @retval RETURN_NOT_FOUND If no PL011 base address could be found.
>>> +
>>> + @return Error codes forwarded from
>>> + PL011UartInitializePort().
>>> **/
>>> RETURN_STATUS
>>> EFIAPI
>>> -FdtPL011SerialPortLibInitialize (
>>> +SerialPortInitialize (
>>> VOID
>>> )
>>> {
>>> VOID *Hob;
>>> + RETURN_STATUS Status;
>>> CONST UINT64 *UartBase;
>>> + UINTN SerialBaseAddress;
>>> UINT64 BaudRate;
>>> UINT32 ReceiveFifoDepth;
>>> EFI_PARITY_TYPE Parity;
>>> UINT8 DataBits;
>>> EFI_STOP_BITS_TYPE StopBits;
>>>
>>> + if (mSerialBaseAddress != 0) {
>>> + return RETURN_SUCCESS;
>>> + }
>>> +
>>> + if (RETURN_ERROR (mPermanentStatus)) {
>>> + return mPermanentStatus;
>>> + }
>>> +
>>> Hob = GetFirstGuidHob (&gEarlyPL011BaseAddressGuid);
>>> if ((Hob == NULL) || (GET_GUID_HOB_DATA_SIZE (Hob) != sizeof *UartBase)) {
>>> - return RETURN_NOT_FOUND;
>>> + Status = RETURN_NOT_FOUND;
>>> + goto Failed;
>>> }
>>>
>>> UartBase = GET_GUID_HOB_DATA (Hob);
>>>
>>> - mSerialBaseAddress = (UINTN)*UartBase;
>>> - if (mSerialBaseAddress == 0) {
>>> - return RETURN_NOT_FOUND;
>>> + SerialBaseAddress = (UINTN)*UartBase;
>>> + if (SerialBaseAddress == 0) {
>>> + Status = RETURN_NOT_FOUND;
>>> + goto Failed;
>>> }
>>>
>>> BaudRate = (UINTN)PcdGet64 (PcdUartDefaultBaudRate);
>>> @@ -74,15 +82,25 @@ FdtPL011SerialPortLibInitialize (
>>> DataBits = PcdGet8 (PcdUartDefaultDataBits);
>>> StopBits = (EFI_STOP_BITS_TYPE)PcdGet8 (PcdUartDefaultStopBits);
>>>
>>> - return PL011UartInitializePort (
>>> - mSerialBaseAddress,
>>> - FixedPcdGet32 (PL011UartClkInHz),
>>> - &BaudRate,
>>> - &ReceiveFifoDepth,
>>> - &Parity,
>>> - &DataBits,
>>> - &StopBits
>>> - );
>>> + Status = PL011UartInitializePort (
>>> + SerialBaseAddress,
>>> + FixedPcdGet32 (PL011UartClkInHz),
>>> + &BaudRate,
>>> + &ReceiveFifoDepth,
>>> + &Parity,
>>> + &DataBits,
>>> + &StopBits
>>> + );
>>> + if (RETURN_ERROR (Status)) {
>>> + goto Failed;
>>> + }
>>> +
>>> + mSerialBaseAddress = SerialBaseAddress;
>>> + return RETURN_SUCCESS;
>>> +
>>> +Failed:
>>> + mPermanentStatus = Status;
>>> + return Status;
>>> }
>>>
>>> /**
>>> @@ -102,7 +120,7 @@ SerialPortWrite (
>>> IN UINTN NumberOfBytes
>>> )
>>> {
>>> - if (mSerialBaseAddress != 0) {
>>> + if (!RETURN_ERROR (SerialPortInitialize ())) {
>>> return PL011UartWrite (mSerialBaseAddress, Buffer, NumberOfBytes);
>>> }
>>>
>>> @@ -126,7 +144,7 @@ SerialPortRead (
>>> IN UINTN NumberOfBytes
>>> )
>>> {
>>> - if (mSerialBaseAddress != 0) {
>>> + if (!RETURN_ERROR (SerialPortInitialize ())) {
>>> return PL011UartRead (mSerialBaseAddress, Buffer, NumberOfBytes);
>>> }
>>>
>>> @@ -146,7 +164,7 @@ SerialPortPoll (
>>> VOID
>>> )
>>> {
>>> - if (mSerialBaseAddress != 0) {
>>> + if (!RETURN_ERROR (SerialPortInitialize ())) {
>>> return PL011UartPoll (mSerialBaseAddress);
>>> }
>>>
>>> @@ -199,7 +217,7 @@ SerialPortSetAttributes (
>>> {
>>> RETURN_STATUS Status;
>>>
>>> - if (mSerialBaseAddress == 0) {
>>> + if (RETURN_ERROR (SerialPortInitialize ())) {
>>> Status = RETURN_UNSUPPORTED;
>>> } else {
>>> Status = PL011UartInitializePort (
>>> @@ -234,7 +252,7 @@ SerialPortSetControl (
>>> {
>>> RETURN_STATUS Status;
>>>
>>> - if (mSerialBaseAddress == 0) {
>>> + if (RETURN_ERROR (SerialPortInitialize ())) {
>>> Status = RETURN_UNSUPPORTED;
>>> } else {
>>> Status = PL011UartSetControl (mSerialBaseAddress, Control);
>>> @@ -261,7 +279,7 @@ SerialPortGetControl (
>>> {
>>> RETURN_STATUS Status;
>>>
>>> - if (mSerialBaseAddress == 0) {
>>> + if (RETURN_ERROR (SerialPortInitialize ())) {
>>> Status = RETURN_UNSUPPORTED;
>>> } else {
>>> Status = PL011UartGetControl (mSerialBaseAddress, Control);
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109388): https://edk2.groups.io/g/devel/message/109388
Mute This Topic: https://groups.io/mt/101682671/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list