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

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

[Joey Gouly]

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/UefiShellAcpiViewCommandLib.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..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 947fb1f375667e12f8603e4264fef5e69cb98919..00e770d677ec1f2a23c1650fe2f4a94f2f86649f 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 15aa2392b60cee9e3843c7c560b0ab84e0be4174..9935537aaee28381fecec08d0057db83aaca1e1d 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.
   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..637c33adbcb2a84fb5bbfe09eafffda61e78cc8d 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")

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

[Joey Gouly]

> From: Joey Gouly <joey.gouly@arm.com>
> Subject: [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.

Ping.

Is anyone able to review this patch adding some more validation checks to Acpiview / MADT?

Thanks,
Joey

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

[Ni, Ray]

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")
> 
> 
> 
> 
> 

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

[Joey Gouly]

> From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ni, Ray via groups.io <ray.ni=intel.com@groups.io>
> Is this for ARM only?

Yes, the GIC interrupt controller is for Arm only.

Thanks,
Joey
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.