[edk2-devel] [PATCH v1 1/1] ShellPkg: Validate that the Boot CPU is present in MADT
Leif Lindholm
leif at nuviainc.com
Wed Dec 2 12:33:01 UTC 2020
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 at 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")
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68178): https://edk2.groups.io/g/devel/message/68178
Mute This Topic: https://groups.io/mt/78636882/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list