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

["Leif Lindholm" <leif@nuviainc.com>]

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