* [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count @ 2020-05-18 17:11 Ard Biesheuvel 2020-05-18 17:18 ` Leif Lindholm 2020-05-19 10:00 ` [edk2-devel] " Laszlo Ersek 0 siblings, 2 replies; 6+ messages in thread From: Ard Biesheuvel @ 2020-05-18 17:11 UTC (permalink / raw) To: devel; +Cc: leif, graeme.gregory, tanmay.jagdale, Ard Biesheuvel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count 2020-05-18 17:11 [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count Ard Biesheuvel @ 2020-05-18 17:18 ` Leif Lindholm 2020-05-19 10:00 ` [edk2-devel] " Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: Leif Lindholm @ 2020-05-18 17:18 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: devel, graeme.gregory, tanmay.jagdale 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count 2020-05-18 17:11 [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count Ard Biesheuvel 2020-05-18 17:18 ` Leif Lindholm @ 2020-05-19 10:00 ` Laszlo Ersek 2020-05-19 11:15 ` Leif Lindholm 2020-05-19 12:12 ` Ard Biesheuvel 1 sibling, 2 replies; 6+ messages in thread From: Laszlo Ersek @ 2020-05-19 10:00 UTC (permalink / raw) To: devel, ard.biesheuvel; +Cc: leif, graeme.gregory, tanmay.jagdale 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count 2020-05-19 10:00 ` [edk2-devel] " Laszlo Ersek @ 2020-05-19 11:15 ` Leif Lindholm 2020-05-20 9:16 ` Sami Mujawar 2020-05-19 12:12 ` Ard Biesheuvel 1 sibling, 1 reply; 6+ messages in thread From: Leif Lindholm @ 2020-05-19 11:15 UTC (permalink / raw) To: devel, lersek Cc: ard.biesheuvel, graeme.gregory, tanmay.jagdale, Sami Mujawar +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 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count 2020-05-19 11:15 ` Leif Lindholm @ 2020-05-20 9:16 ` Sami Mujawar 0 siblings, 0 replies; 6+ messages in thread From: Sami Mujawar @ 2020-05-20 9:16 UTC (permalink / raw) To: Leif Lindholm, devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel, graeme.gregory@linaro.org, tanmay.jagdale@linaro.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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count 2020-05-19 10:00 ` [edk2-devel] " Laszlo Ersek 2020-05-19 11:15 ` Leif Lindholm @ 2020-05-19 12:12 ` Ard Biesheuvel 1 sibling, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2020-05-19 12:12 UTC (permalink / raw) To: Laszlo Ersek, devel; +Cc: leif, graeme.gregory, tanmay.jagdale 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-20 9:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-18 17:11 [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count Ard Biesheuvel 2020-05-18 17:18 ` Leif Lindholm 2020-05-19 10:00 ` [edk2-devel] " Laszlo Ersek 2020-05-19 11:15 ` Leif Lindholm 2020-05-20 9:16 ` Sami Mujawar 2020-05-19 12:12 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox