Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly

["Ard Biesheuvel" <ardb@kernel.org>]

On Mon, 2 Oct 2023 at 16:47, Laszlo Ersek <lersek@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@kernel.org>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
> > Cc: Oliver Steffen <osteffen@redhat.com>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Reported-by: Oliver Smith-Denny <osde@linux.microsoft.com>
> > Signed-off-by: Laszlo Ersek <lersek@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@kernel.org>

I'll queue this up.


> >
> >  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 (#109366): https://edk2.groups.io/g/devel/message/109366
Mute This Topic: https://groups.io/mt/101682671/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-