From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120])
 by mx.groups.io with SMTP id smtpd.web10.8768.1589882411606531979
 for <devel@edk2.groups.io>;
 Tue, 19 May 2020 03:00:11 -0700
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LOT3Zni4;
 spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
	s=mimecast20190719; t=1589882410;
	h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
	 to:to:cc:cc:mime-version:mime-version:content-type:content-type:
	 content-transfer-encoding:content-transfer-encoding:
	 in-reply-to:in-reply-to:references:references;
	bh=54jQVD9gal9jMMvpr7LWbzqQuBwQEnHv/NghZ588qtI=;
	b=LOT3Zni4VB0YF3r2/F8552Gu04HBrSakawir/qSfGQiD3d/LWN+vyfyHab5KBmkqJnPcsa
	bThcVAJ2xtxtq4nLyDFpvfn2J+PqQn7kYQWDne0WCSXwLw8JtKhrQtt7Qo1T1k7VcPgmFJ
	t2jR7NiLeb3s645nWJCt0s47QzaL4dE=
Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com
 [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id
 us-mta-11-rAIMSsnsMW-27YueHqVIDQ-1; Tue, 19 May 2020 06:00:09 -0400
X-MC-Unique: rAIMSsnsMW-27YueHqVIDQ-1
Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BAF63835B42;
	Tue, 19 May 2020 10:00:06 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-149.ams2.redhat.com [10.36.114.149])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 8384B649D6;
	Tue, 19 May 2020 10:00:03 +0000 (UTC)
Subject: Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count
To: devel@edk2.groups.io, ard.biesheuvel@arm.com
Cc: leif@nuviainc.com, graeme.gregory@linaro.org, tanmay.jagdale@linaro.org
References: <20200518171148.6113-1-ard.biesheuvel@arm.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <6f410b08-f868-8b38-60fd-2b863a89658e@redhat.com>
Date: Tue, 19 May 2020 12:00:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20200518171148.6113-1-ard.biesheuvel@arm.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Language: en-US
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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