On 10/6/23 19:08, Ard Biesheuvel wrote: > On Mon, 2 Oct 2023 at 16:47, Laszlo Ersek <ler...@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+tianoc...@kernel.org> >>> Cc: Gerd Hoffmann <kra...@redhat.com> >>> Cc: Leif Lindholm <quic_llind...@quicinc.com> >>> Cc: Oliver Smith-Denny <o...@linux.microsoft.com> >>> Cc: Oliver Steffen <ostef...@redhat.com> >>> Cc: Sami Mujawar <sami.muja...@arm.com> >>> Cc: Sean Brogan <sean.bro...@microsoft.com> >>> Reported-by: Oliver Smith-Denny <o...@linux.microsoft.com> >>> Signed-off-by: Laszlo Ersek <ler...@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 <a...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
