From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by mx.groups.io with SMTP id smtpd.web10.3992.1606912385777424579 for ; Wed, 02 Dec 2020 04:33:06 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=H3iaYwxz; spf=pass (domain: nuviainc.com, ip: 209.85.128.50, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f50.google.com with SMTP id x22so6630028wmc.5 for ; Wed, 02 Dec 2020 04:33:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=pvBu7d8LXRzSs61zJklkeWUdMm6t6y2D4YsTgfgCPL8=; b=H3iaYwxz83b1zrWp5m2YRuPbVfJtFvdZcDfihYYobYSHI4ULFLZYapprxEhD7pJ6y7 V4eE231e1FQblRfNk06POHtGWkKYJJgWvZyufdDm0krDE+0NhtUSu3T1hpYMBMuEXaz5 S3M9zJXSPCaSUZvKOzPQMSYY2ZpRcxVXpOTQpmhdve6Icvzi9IXZvr6AfbkMCoaHzO/R VFbqPIEqo0y/x5B/og1K5vy1wP9Jbj3CAf/8DGpG4FUJCpeepZ46otp4LszOFtNyZ4xL Rs74Vi1SQRqUHqfGGzHHaLwzMiIlM9AQGvGwOXOAgBIbTuZ4K8JwGBuJNfiakiyAhg7N sVLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=pvBu7d8LXRzSs61zJklkeWUdMm6t6y2D4YsTgfgCPL8=; b=pYFPGyas06Msjae685JcyHB6xlMXNFf+3nPCClN1wo85DzywGDJ00LBPXk8PsfucuR S0/QZY9QskqUfUdeB2A3YHWEd7tAgQrvhq01jwwMWrEU6CfWHw+MZ9q+qFl5XFvZeOWs FejIttUgSHtb4vBaVXcG5gMB3oiL2zBowNE/EIKTl/j6C5vlYAAI35TJvHq5kxzv+dyH oPEUeN3y9DwnvRmmND53bXCxmmSAW07YQQvB6ExDqpabu9AmqFBFcVOomcRfW+RtCLLk wiZ7pLWWMVpyN+ZdXVDQNlQGwpv+f7iMFzoC9JzqL4Wa5jTePPS87BvrwSarevDlJMW7 q1sA== X-Gm-Message-State: AOAM532g5shzjY6bCOqTX/HNQJNxKAf0S4ulTWHt6b3DKxO7rbwq0CEw lFcoSYzjqjza/3ZnY5ZAJBRZGA== X-Google-Smtp-Source: ABdhPJz6rHU/q1T17yXHnb/AQrmLdADk6wZFA/Cn5qyr+IQ3koWeHySs9YWxrJJqljGeMdWfazre/g== X-Received: by 2002:a1c:6008:: with SMTP id u8mr2816987wmb.173.1606912383939; Wed, 02 Dec 2020 04:33:03 -0800 (PST) Return-Path: Received: from vanye (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id o13sm1829556wmc.44.2020.12.02.04.33.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Dec 2020 04:33:03 -0800 (PST) Date: Wed, 2 Dec 2020 12:33:01 +0000 From: "Leif Lindholm" To: Joey Gouly Cc: devel@edk2.groups.io, ard.biesheuvel@arm.com, thomas.abraham@arm.com, nd@arm.com Subject: Re: [PATCH v1 1/1] ShellPkg: Validate that the Boot CPU is present in MADT Message-ID: <20201202123301.GG1664@vanye> References: <20201201160536.16903-1-joey.gouly@arm.com> MIME-Version: 1.0 In-Reply-To: <20201201160536.16903-1-joey.gouly@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Tue, Dec 01, 2020 at 16:05:36 +0000, Joey Gouly wrote: > The ACPI 6.3 Specification, January 2019, section 5.2.12.14 states that > the firmware must convey each processor’s GIC information to the OS using > the GICC structure. > > If a GICC structure for the boot CPU is missing some standards-based Sorry for nitpick, but that sentence really needs a comma before "some". > operating system may crash with an error code. It may be difficult to > diagnose the reason for the crash as the error code may be too generic > and mean firmware error. > > Therefore add validation to the MADT table parser to check that a GICC is > present for the boot CPU. > > Signed-off-by: Joey Gouly > --- > The changes can be seen at https://github.com/jgouly/edk2/tree/1474_validate_boot_cpu_mpidr_v1 > > ShellPkg/ShellPkg.dsc | 3 ++ > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf | 7 ++- > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 54 +++++++++++++++++++- > ShellPkg/ShellPkg.ci.yaml | 3 +- > 4 files changed, 63 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc > index c42bc9464a0f7be111ee3086a664506c8288928c..661cc8b02b0971280c3649e0f29e109305bcc776 100644 > --- a/ShellPkg/ShellPkg.dsc > +++ b/ShellPkg/ShellPkg.dsc > @@ -71,6 +71,9 @@ [LibraryClasses.ARM,LibraryClasses.AARCH64] > # Add support for GCC stack protector > NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf > > + # Add support for reading MPIDR > + ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf > + > [PcdsFixedAtBuild] > gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xFF > gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|16000 > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf > index 91459f9ec632635ee453c5ef46f67445cd9eee0c..6f9e77eed8f8c5f12ecf6b44c494da2e6aacda27 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf > @@ -1,7 +1,7 @@ > ## @file > # Provides Shell 'acpiview' command functions > # > -# Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
> +# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
Urgh. Do you have some internal CI job that bugs you if the current marketing-approved capitalisation is not used? Otherwise, I'd request to leave this alone until the date actually needs changing. > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -57,6 +57,9 @@ [Packages] > MdePkg/MdePkg.dec > ShellPkg/ShellPkg.dec > > +[Packages.ARM, Packages.AARCH64] > + ArmPkg/ArmPkg.dec > + > [LibraryClasses] > BaseLib > BaseMemoryLib > @@ -72,6 +75,8 @@ [LibraryClasses] > UefiLib > UefiRuntimeServicesTableLib > > +[LibraryClasses.ARM, LibraryClasses.AARCH64] > + ArmLib > > [FixedPcd] > gEfiShellPkgTokenSpaceGuid.PcdShellProfileMask ## CONSUMES > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c > index 15aa2392b60cee9e3843c7c560b0ab84e0be4174..be013ce5c06541d12dcd594eb0f5c1820708923e 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c > @@ -1,7 +1,7 @@ > /** @file > MADT table parser > > - Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. > + Copyright (c) 2016 - 2020, Arm Limited. All rights reserved. Likewise. > SPDX-License-Identifier: BSD-2-Clause-Patent > > @par Reference(s): > @@ -13,6 +13,9 @@ > > #include > #include > +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64) > +#include > +#endif > #include "AcpiParser.h" > #include "AcpiTableParser.h" > #include "AcpiViewConfig.h" > @@ -23,6 +26,11 @@ STATIC CONST UINT8* MadtInterruptControllerType; > STATIC CONST UINT8* MadtInterruptControllerLength; > STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; > > +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64) > +STATIC UINT64 BootMpidr; > +STATIC BOOLEAN HasBootCpuGicc = FALSE; If these are global variables, they should have m or g prefix, as appropriate. Preceding definitions do not follow coding style. > +#endif > + > /** > This function validates the System Vector Base in the GICD. > > @@ -95,6 +103,33 @@ ValidateSpeOverflowInterrupt ( > } > } > > +/** > + This function validates that the GICC structure contains an entry for > + the Boot CPU. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information e.g. this > + could be a pointer to the ACPI table header. > +**/ > +STATIC > +VOID > +EFIAPI > +ValidateBootMpidr ( > + IN UINT8* Ptr, > + IN VOID* Context > + ) > +{ > +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64) Surely all of the struct that should be called mGicCParser is only for ARM/AARCH64 and could be moved into a source file just included for those, and this function with it? With the only filtering on architectures done in ParseAcpiMadt? > + UINT64 CurrentMpidr; > + > + CurrentMpidr = *(UINT64*)Ptr; > + > + if (CurrentMpidr == BootMpidr) { > + HasBootCpuGicc = TRUE; > + } > +#endif > +} > + > /** > An ACPI_PARSER array describing the GICC Interrupt Controller Structure. > **/ > @@ -115,7 +150,7 @@ STATIC CONST ACPI_PARSER GicCParser[] = { > {L"GICH", 8, 48, L"0x%lx", NULL, NULL, NULL, NULL}, > {L"VGIC Maintenance interrupt", 4, 56, L"0x%x", NULL, NULL, NULL, NULL}, > {L"GICR Base Address", 8, 60, L"0x%lx", NULL, NULL, NULL, NULL}, > - {L"MPIDR", 8, 68, L"0x%lx", NULL, NULL, NULL, NULL}, > + {L"MPIDR", 8, 68, L"0x%lx", NULL, NULL, ValidateBootMpidr, NULL}, > {L"Processor Power Efficiency Class", 1, 76, L"0x%x", NULL, NULL, NULL, > NULL}, > {L"Reserved", 1, 77, L"0x%x", NULL, NULL, NULL, NULL}, > @@ -234,6 +269,11 @@ ParseAcpiMadt ( > UINT8* InterruptContollerPtr; > UINT32 GICDCount; > > +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64) > + BootMpidr = ArmReadMpidr () & > + (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3); > +#endif > + > GICDCount = 0; > > if (!Trace) { > @@ -371,4 +411,14 @@ ParseAcpiMadt ( > InterruptContollerPtr += *MadtInterruptControllerLength; > Offset += *MadtInterruptControllerLength; > } // while > + > +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64) > + if (!HasBootCpuGicc) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: No GICC present for Boot CPU (MPIDR: 0x%lx)", > + BootMpidr > + ); > + } > +#endif > } > diff --git a/ShellPkg/ShellPkg.ci.yaml b/ShellPkg/ShellPkg.ci.yaml > index 30894d44bc3ae9a2a9796146c5bcdc62d4ce9801..2833febb296f3d3b1fa0c48268955bb574d1dbee 100644 > --- a/ShellPkg/ShellPkg.ci.yaml > +++ b/ShellPkg/ShellPkg.ci.yaml > @@ -31,7 +31,8 @@ > "MdePkg/MdePkg.dec", > "MdeModulePkg/MdeModulePkg.dec", > "ShellPkg/ShellPkg.dec", > - "NetworkPkg/NetworkPkg.dec" > + "NetworkPkg/NetworkPkg.dec", > + "ArmPkg/ArmPkg.dec" NetworkPkg addition alredy screwed the sorting up, but still, please insert this one first. / Leif > ], > # For host based unit tests > "AcceptableDependencies-HOST_APPLICATION":[], > -- > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >