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

[PATCH v1 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>
---
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.<BR>
+# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
 #
 #  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.
   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 BootMpidr;
+STATIC BOOLEAN HasBootCpuGicc = 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 == 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"
         ],
         # For host based unit tests
         "AcceptableDependencies-HOST_APPLICATION":[],
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

[Leif Lindholm]

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 <joey.gouly@arm.com>
> ---
> 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.<BR>
> +# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>

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

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

[Joey Gouly]

> > +/**
> > +  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?

I think that could be done if/when another architecture wants to add some
specific support to this file. Otherwise, in my opinion it just adds more
files / layers without any real benefit.

Thanks for the other comments, I have fixed them, and will wait for your reply
to the above comment.

Joey