ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++ 1 file changed, 2 insertions(+)
In the ArmPkg version of PlatformBootManagerLib, we construct a
serial device path based on the default settings for baud rate,
parity and the number of stop bits, to ensure that a serial console
is available even on the very first boot.
This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are
not set to '0', meaning 'the default', as there is no default for
these when constructing a device path.
So add a couple of ASSERT()s to make sure that we catch this condition,
since it otherwise ignores the bogus device path silently, which is
rather tedious to debug,.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index e6e788e0f107..a030d510aa62 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole (
//
// Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
//
+ ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0);
+ ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) > 0);
ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4);
CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid);
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59753): https://edk2.groups.io/g/devel/message/59753
Mute This Topic: https://groups.io/mt/74297223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Mon, May 18, 2020 at 19:11:48 +0200, Ard Biesheuvel wrote: > In the ArmPkg version of PlatformBootManagerLib, we construct a > serial device path based on the default settings for baud rate, > parity and the number of stop bits, to ensure that a serial console > is available even on the very first boot. > > This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are > not set to '0', meaning 'the default', as there is no default for > these when constructing a device path. > > So add a couple of ASSERT()s to make sure that we catch this condition, > since it otherwise ignores the bogus device path silently, which is > rather tedious to debug,. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> Reviewed-by: Leif Lindholm <leif@nuviainc.com> > --- > ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > index e6e788e0f107..a030d510aa62 100644 > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > @@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole ( > // > // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut. > // > + ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0); > + ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) > 0); > ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4); > CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid); > > -- > 2.17.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59755): https://edk2.groups.io/g/devel/message/59755 Mute This Topic: https://groups.io/mt/74297223/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 05/18/20 19:11, Ard Biesheuvel wrote: > In the ArmPkg version of PlatformBootManagerLib, we construct a > serial device path based on the default settings for baud rate, > parity and the number of stop bits, to ensure that a serial console > is available even on the very first boot. > > This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are > not set to '0', meaning 'the default', as there is no default for > these when constructing a device path. > > So add a couple of ASSERT()s to make sure that we catch this condition, > since it otherwise ignores the bogus device path silently, which is > rather tedious to debug,. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > --- > ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > index e6e788e0f107..a030d510aa62 100644 > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > @@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole ( > // > // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut. > // > + ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0); > + ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) > 0); > ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4); > CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid); > > Given that these are fixed PCDs, I'd recommend STATIC_ASSERT(). We've recently put STATIC_ASSERT() to use (in "OvmfPkg/MptScsiDxe/MptScsi.c") in combination with a fixed PCD: STATIC_ASSERT ( FixedPcdGet8 (PcdMptScsiMaxTargetLimit) < 255, "Req supports 255 targets only (max target is 254)" ); Just an idea of course; if that's out of scope for now, I have nothing against this patch. Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59798): https://edk2.groups.io/g/devel/message/59798 Mute This Topic: https://groups.io/mt/74297223/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 5/19/20 12:00 PM, Laszlo Ersek wrote: > On 05/18/20 19:11, Ard Biesheuvel wrote: >> In the ArmPkg version of PlatformBootManagerLib, we construct a >> serial device path based on the default settings for baud rate, >> parity and the number of stop bits, to ensure that a serial console >> is available even on the very first boot. >> >> This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are >> not set to '0', meaning 'the default', as there is no default for >> these when constructing a device path. >> >> So add a couple of ASSERT()s to make sure that we catch this condition, >> since it otherwise ignores the bogus device path silently, which is >> rather tedious to debug,. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> >> --- >> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c >> index e6e788e0f107..a030d510aa62 100644 >> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c >> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c >> @@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole ( >> // >> // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut. >> // >> + ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0); >> + ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) > 0); >> ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4); >> CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid); >> >> > > Given that these are fixed PCDs, I'd recommend STATIC_ASSERT(). We've > recently put STATIC_ASSERT() to use (in "OvmfPkg/MptScsiDxe/MptScsi.c") > in combination with a fixed PCD: > > STATIC_ASSERT ( > FixedPcdGet8 (PcdMptScsiMaxTargetLimit) < 255, > "Req supports 255 targets only (max target is 254)" > ); > > Just an idea of course; if that's out of scope for now, I have nothing > against this patch. > Thanks. This is actually much better, as it diagnoses potential issues with the serial console in the RELEASE build as well, which might be tricky to catch otherwise. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59812): https://edk2.groups.io/g/devel/message/59812 Mute This Topic: https://groups.io/mt/74297223/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
+Sami On Tue, May 19, 2020 at 12:00:02 +0200, Laszlo Ersek wrote: > On 05/18/20 19:11, Ard Biesheuvel wrote: > > In the ArmPkg version of PlatformBootManagerLib, we construct a > > serial device path based on the default settings for baud rate, > > parity and the number of stop bits, to ensure that a serial console > > is available even on the very first boot. > > > > This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are > > not set to '0', meaning 'the default', as there is no default for > > these when constructing a device path. > > > > So add a couple of ASSERT()s to make sure that we catch this condition, > > since it otherwise ignores the bogus device path silently, which is > > rather tedious to debug,. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > > --- > > ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > index e6e788e0f107..a030d510aa62 100644 > > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > @@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole ( > > // > > // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut. > > // > > + ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0); > > + ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) > 0); > > ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4); > > CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid); > > > > > > Given that these are fixed PCDs, I'd recommend STATIC_ASSERT(). We've > recently put STATIC_ASSERT() to use (in "OvmfPkg/MptScsiDxe/MptScsi.c") > in combination with a fixed PCD: > > STATIC_ASSERT ( > FixedPcdGet8 (PcdMptScsiMaxTargetLimit) < 255, > "Req supports 255 targets only (max target is 254)" > ); > > Just an idea of course; if that's out of scope for now, I have nothing > against this patch. That sounds useful, but should then probably get applied to the TerminalType as well, so could come in as a separate patch on top. Sami: could this be an alternative resolution for https://edk2.groups.io/g/devel/message/59740 ? Regards, Leif > Thanks, > Laszlo > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59805): https://edk2.groups.io/g/devel/message/59805 Mute This Topic: https://groups.io/mt/74297223/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Leif, Please find my response marked inline as [SAMI] Regards, Sami Mujawar -----Original Message----- From: Leif Lindholm <leif@nuviainc.com> Sent: 19 May 2020 12:16 PM To: devel@edk2.groups.io; lersek@redhat.com Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; graeme.gregory@linaro.org; tanmay.jagdale@linaro.org; Sami Mujawar <Sami.Mujawar@arm.com> Subject: Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count +Sami On Tue, May 19, 2020 at 12:00:02 +0200, Laszlo Ersek wrote: > On 05/18/20 19:11, Ard Biesheuvel wrote: > > In the ArmPkg version of PlatformBootManagerLib, we construct a > > serial device path based on the default settings for baud rate, > > parity and the number of stop bits, to ensure that a serial console > > is available even on the very first boot. > > > > This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are > > not set to '0', meaning 'the default', as there is no default for > > these when constructing a device path. > > > > So add a couple of ASSERT()s to make sure that we catch this > > condition, since it otherwise ignores the bogus device path > > silently, which is rather tedious to debug,. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > > --- > > ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > index e6e788e0f107..a030d510aa62 100644 > > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > > @@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole ( > > // > > // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut. > > // > > + ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0); ASSERT > > + (FixedPcdGet8 (PcdUartDefaultStopBits) > 0); > > ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4); > > CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid); > > > > > > Given that these are fixed PCDs, I'd recommend STATIC_ASSERT(). We've > recently put STATIC_ASSERT() to use (in > "OvmfPkg/MptScsiDxe/MptScsi.c") in combination with a fixed PCD: > > STATIC_ASSERT ( > FixedPcdGet8 (PcdMptScsiMaxTargetLimit) < 255, > "Req supports 255 targets only (max target is 254)" > ); > > Just an idea of course; if that's out of scope for now, I have nothing > against this patch. That sounds useful, but should then probably get applied to the TerminalType as well, so could come in as a separate patch on top. Sami: could this be an alternative resolution for https://edk2.groups.io/g/devel/message/59740 ? [SAMI] For https://edk2.groups.io/g/devel/message/59740 we need a conditional check, as we need to decide between using the PL011UartInteger value or computing it using the UART clock and baud rate. I tried the following and it fixes the VS2017 warning without the need of a pragma statement. - if (FixedPcdGet32 (PL011UartInteger) != 0) { - Integer = FixedPcdGet32 (PL011UartInteger); + Integer = FixedPcdGet32 (PL011UartInteger); + if (Integer != 0) { Fractional = FixedPcdGet32 (PL011UartFractional); } else { // If BAUD rate is zero then replace it with the system default value The disassembly output (using GCC Objdump) for the above code, with or without this change appears to be the same. Please let me know if this is more suitable, I can submit a patch with this change. [/SAMI] Regards, Leif > Thanks, > Laszlo > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59948): https://edk2.groups.io/g/devel/message/59948 Mute This Topic: https://groups.io/mt/74297223/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.