* [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly
@ 2023-09-30 21:23 Laszlo Ersek
2023-10-02 14:47 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2023-09-30 21:23 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Oliver Smith-Denny,
Oliver Steffen, Sami Mujawar, Sean Brogan
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.
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 (#109236): https://edk2.groups.io/g/devel/message/109236
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly
2023-09-30 21:23 [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly Laszlo Ersek
@ 2023-10-02 14:47 ` Laszlo Ersek
2023-10-06 17:08 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2023-10-02 14:47 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Oliver Smith-Denny,
Oliver Steffen, Sami Mujawar, Sean Brogan
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly
2023-10-02 14:47 ` Laszlo Ersek
@ 2023-10-06 17:08 ` Ard Biesheuvel
2023-10-08 9:16 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2023-10-06 17:08 UTC (permalink / raw)
To: devel, lersek
Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Oliver Smith-Denny,
Oliver Steffen, Sami Mujawar, Sean Brogan
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly
2023-10-06 17:08 ` Ard Biesheuvel
@ 2023-10-08 9:16 ` Laszlo Ersek
0 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2023-10-08 9:16 UTC (permalink / raw)
To: devel, ardb
Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Oliver Smith-Denny,
Oliver Steffen, Sami Mujawar, Sean Brogan
On 10/6/23 19:08, Ard Biesheuvel wrote:
> 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>
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-08 9:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-30 21:23 [edk2-devel] [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: initialize implicitly Laszlo Ersek
2023-10-02 14:47 ` Laszlo Ersek
2023-10-06 17:08 ` Ard Biesheuvel
2023-10-08 9:16 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox