From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
Gerd Hoffmann <kraxel@redhat.com>,
Leif Lindholm <quic_llindhol@quicinc.com>,
Oliver Smith-Denny <osde@linux.microsoft.com>,
Oliver Steffen <osteffen@redhat.com>,
Sami Mujawar <sami.mujawar@arm.com>,
Sean Brogan <sean.brogan@microsoft.com>
Subject: Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly
Date: Mon, 2 Oct 2023 16:47:00 +0200 [thread overview]
Message-ID: <a28cb7be-6875-9897-eceb-57f19a122778@redhat.com> (raw)
In-Reply-To: <20230930212343.130015-1-lersek@redhat.com>
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.
Thanks
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 (#109255): https://edk2.groups.io/g/devel/message/109255
Mute This Topic: https://groups.io/mt/101682671/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-10-02 14:47 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-30 21:23 [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly Laszlo Ersek
2023-10-02 14:47 ` Laszlo Ersek [this message]
2023-10-06 17:08 ` Ard Biesheuvel
2023-10-08 9:16 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a28cb7be-6875-9897-eceb-57f19a122778@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox