[PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

[PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

[Ard Biesheuvel]

Add build time checks for serial terminal settings, so we don't end up
with a non-functional serial console if the PCDs are set incorrectly

v1 was just the second patch. v2 adds the first patch switching to
STATIC_ASSERT first, and then uses STATIC_ASSERT in the second one
as well.

Ard Biesheuvel (2):
  ArmPkg/PlatformBootManagerLib: use static assertion for console type
  ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit
    count

 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v2 1/2] ArmPkg/PlatformBootManagerLib: use static assertion for console type

[Ard Biesheuvel]

Replace the runtime ASSERT with the build time STATIC_ASSERT on the
check that ensures that the terminal type we use for the serial
console matches the one we explicitly add to the ConIn/ConOut/StdErr
variables.

This helps catch serial console issues early, even in RELEASE builds,
reducing the risk of ending up with no console at all, which can be
tricky to debug on bare metal.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index e6e788e0f107..f713605c02b2 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -583,7 +583,9 @@ PlatformBootManagerBeforeConsole (
   //
   // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
   //
-  ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4);
+  STATIC_ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4,
+    "PcdDefaultTerminalType must be TTYTERM");
+
   CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid);
 
   EfiBootManagerUpdateConsoleVariable (ConIn,
-- 
2.17.1

[PATCH v2 2/2] 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 STATIC_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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index f713605c02b2..3411219fbfdb 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -585,6 +585,10 @@ PlatformBootManagerBeforeConsole (
   //
   STATIC_ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4,
     "PcdDefaultTerminalType must be TTYTERM");
+  STATIC_ASSERT (FixedPcdGet8 (PcdUartDefaultParity) != 0,
+    "PcdUartDefaultParity must be set to an actual value, not 'default'");
+  STATIC_ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) != 0,
+    "PcdUartDefaultStopBits must be set to an actual value, not 'default'");
 
   CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid);
 
-- 
2.17.1

Re: [PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

[Leif Lindholm]

On Tue, May 19, 2020 at 14:23:49 +0200, Ard Biesheuvel wrote:
> Add build time checks for serial terminal settings, so we don't end up
> with a non-functional serial console if the PCDs are set incorrectly
> 
> v1 was just the second patch. v2 adds the first patch switching to
> STATIC_ASSERT first, and then uses STATIC_ASSERT in the second one
> as well.

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Thanks Ard, and Laszlo!

> Ard Biesheuvel (2):
>   ArmPkg/PlatformBootManagerLib: use static assertion for console type
>   ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit
>     count
> 
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

[Sami Mujawar]

Reviewed-by: Sami Mujawar <Sami.Mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@arm.com>
Sent: 19 May 2020 01:24 PM
To: devel@edk2.groups.io
Cc: leif@nuviainc.com; graeme.gregory@linaro.org; tanmay.jagdale@linaro.org; lersek@redhat.com; Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Subject: [PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

Add build time checks for serial terminal settings, so we don't end up with a non-functional serial console if the PCDs are set incorrectly

v1 was just the second patch. v2 adds the first patch switching to STATIC_ASSERT first, and then uses STATIC_ASSERT in the second one as well.

Ard Biesheuvel (2):
  ArmPkg/PlatformBootManagerLib: use static assertion for console type
  ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit
    count

 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--
2.17.1

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.

Re: [PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

[Laszlo Ersek]

On 05/19/20 14:23, Ard Biesheuvel wrote:
> Add build time checks for serial terminal settings, so we don't end up
> with a non-functional serial console if the PCDs are set incorrectly
> 
> v1 was just the second patch. v2 adds the first patch switching to
> STATIC_ASSERT first, and then uses STATIC_ASSERT in the second one
> as well.
> 
> Ard Biesheuvel (2):
>   ArmPkg/PlatformBootManagerLib: use static assertion for console type
>   ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit
>     count
> 
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

Re: [PATCH v2 0/2] ArmPkg/PlatformBootManagerLib: add build time checks for serial terminal settings

[Ard Biesheuvel]

On 5/19/20 2:23 PM, Ard Biesheuvel wrote:
> Add build time checks for serial terminal settings, so we don't end up
> with a non-functional serial console if the PCDs are set incorrectly
> 
> v1 was just the second patch. v2 adds the first patch switching to
> STATIC_ASSERT first, and then uses STATIC_ASSERT in the second one
> as well.
> 
> Ard Biesheuvel (2):
>    ArmPkg/PlatformBootManagerLib: use static assertion for console type
>    ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit
>      count
> 
>   ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 

Pushed as #655

Thanks all.