[PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[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

Re: [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[Leif Lindholm]

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
> 

Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[Leif Lindholm]

+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
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[Ard Biesheuvel]

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.

Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

[Sami Mujawar]

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.