Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: Validate that the Boot CPU is present in MADT

["Ni, Ray" <ray.ni@intel.com>]

Is this for ARM only?

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Joey
> Gouly
> Sent: Friday, January 15, 2021 10:44 PM
> To: devel@edk2.groups.io
> Cc: joey.gouly@arm.com; ardb+tianocore@kernel.org; leif@nuviainc.com;
> sami.mujawar@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v2 1/1] ShellPkg: Validate that the Boot CPU is
> present in MADT
> 
> 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
> 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 <joey.gouly@arm.com>
> ---
> 
> Changes since v1:
>   - Added 'm' prefix for global variables
>   - Reordered ci yaml file
> 
> The changes can be seen at
> https://github.com/jgouly/edk2/tree/1474_validate_boot_cpu_mpidr_v2
> 
>  ShellPkg/ShellPkg.dsc                                                        |  3 ++
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComman
> dLib.inf |  5 ++
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c       | 54 +++++++++++++++++++-
>  ShellPkg/ShellPkg.ci.yaml                                                    |  2 +
>  4 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
> index
> c42bc9464a0f7be111ee3086a664506c8288928c..661cc8b02b0971280c3649e0f2
> 9e109305bcc776 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/UefiShellAcpiViewComm
> andLib.inf
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> andLib.inf
> index
> 947fb1f375667e12f8603e4264fef5e69cb98919..00e770d677ec1f2a23c1650fe2f
> 4a94f2f86649f 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> andLib.inf
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> andLib.inf
> @@ -60,6 +60,9 @@ [Packages]
>    MdePkg/MdePkg.dec
>    ShellPkg/ShellPkg.dec
> 
> +[Packages.ARM, Packages.AARCH64]
> +  ArmPkg/ArmPkg.dec
> +
>  [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
> @@ -75,6 +78,8 @@ [LibraryClasses]
>    UefiLib
>    UefiRuntimeServicesTableLib
> 
> +[LibraryClasses.ARM, LibraryClasses.AARCH64]
> +  ArmLib
> 
>  [FixedPcd]
>    gEfiShellPkgTokenSpaceGuid.PcdShellProfileMask ## CONSUMES
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.c
> index
> 15aa2392b60cee9e3843c7c560b0ab84e0be4174..9935537aaee28381fecec08d0
> 057db83aaca1e1d 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.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.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>    @par Reference(s):
> @@ -13,6 +13,9 @@
> 
>  #include <IndustryStandard/Acpi.h>
>  #include <Library/UefiLib.h>
> +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64)
> +#include <Library/ArmLib.h>
> +#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 mBootMpidr;
> +STATIC BOOLEAN mHasBootMpidrGicc = FALSE;
> +#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)
> +  UINT64 CurrentMpidr;
> +
> +  CurrentMpidr = *(UINT64*)Ptr;
> +
> +  if (CurrentMpidr == mBootMpidr) {
> +    mHasBootMpidrGicc = 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)
> +  mBootMpidr = 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 (!mHasBootMpidrGicc) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: No GICC present for Boot CPU (MPIDR: 0x%lx)",
> +      mBootMpidr
> +      );
> +  }
> +#endif
>  }
> diff --git a/ShellPkg/ShellPkg.ci.yaml b/ShellPkg/ShellPkg.ci.yaml
> index
> 30894d44bc3ae9a2a9796146c5bcdc62d4ce9801..637c33adbcb2a84fb5bbfe09e
> afffda61e78cc8d 100644
> --- a/ShellPkg/ShellPkg.ci.yaml
> +++ b/ShellPkg/ShellPkg.ci.yaml
> @@ -3,6 +3,7 @@
>  #
>  # Copyright (c) Microsoft Corporation
>  # Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2020, Arm Limited. All rights reserved.
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  ##
>  {
> @@ -28,6 +29,7 @@
>      },
>      "DependencyCheck": {
>          "AcceptableDependencies": [
> +            "ArmPkg/ArmPkg.dec",
>              "MdePkg/MdePkg.dec",
>              "MdeModulePkg/MdeModulePkg.dec",
>              "ShellPkg/ShellPkg.dec",
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 
> 
> 
> 
>