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

["Leif Lindholm" <leif@nuviainc.com>]

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