[edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
Gao, Zhichao
zhichao.gao at intel.com
Thu May 30 08:54:48 UTC 2019
Sorry for late update.
Some minor comments below.
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Friday, May 17, 2019 12:06 AM
> To: devel at edk2.groups.io
> Cc: Sami.Mujawar at arm.com; Carsey, Jaben <jaben.carsey at intel.com>; Ni,
> Ray <ray.ni at intel.com>; Matteo.Carlini at arm.com; Stephanie.Hughes-
> Fitt at arm.com; nd at arm.com
> Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for
> MADT parser
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1820
>
> The ACPI 6.3 specification introduces a 'SPE overflow Interrupt' field as part
> of the GICC structure.
>
> Update the MADT parser to decode this field and validate the interrupt ID
> used.
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch at arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v1
>
> Notes:
> v1:
> - Decode and validate SPE Overflow Interrupt field [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 86 ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> h | 35 ++++++++
> 2 files changed, 113 insertions(+), 8 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.c
> index
> a1bf86ade5313f954a77b325c13394cfce133939..59c3df0cc8a080497b517baf36f
> c63f1e4ab866f 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> +++ er.c
> @@ -1,17 +1,21 @@
> /** @file
> MADT table parser
>
> - Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> + Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @par Reference(s):
> - - ACPI 6.2 Specification - Errata A, September 2017
> + - ACPI 6.3 Specification - January 2019
> + - Arm Generic Interrupt Controller Architecture Specification,
> + GIC architecture version 3 and version 4, issue E
> + - Arm Server Base System Architecture 5.0
> **/
>
> #include <IndustryStandard/Acpi.h>
> #include <Library/UefiLib.h>
> #include "AcpiParser.h"
> #include "AcpiTableParser.h"
> +#include "MadtParser.h"
>
> // Local Variables
> STATIC CONST UINT8* MadtInterruptControllerType; @@ -33,6 +37,21 @@
> ValidateGICDSystemVectorBase (
> IN VOID* Context
> );
>
> +/**
> + This function validates the SPE Overflow Interrupt in the GICC.
> +
> + @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
> +ValidateSpeOverflowInterrupt (
> + IN UINT8* Ptr,
> + IN VOID* Context
> + );
> +
>From ACPI 6.3 Table 5-60:
Statistical Profiling Extension buffer overflow GSIV. This interrupt is a level triggered PPI. Zero if SPE is not supported by this processor.
Seems it did descript which value is invalid. If it is mentioned in other spec, please help to figure out in the commit message.
Maybe I miss something. If so, please help to point.
> /**
> An ACPI_PARSER array describing the GICC Interrupt Controller Structure.
> **/
> @@ -56,7 +75,9 @@ STATIC CONST ACPI_PARSER GicCParser[] = {
> {L"MPIDR", 8, 68, L"0x%lx", NULL, NULL, NULL, NULL},
> {L"Processor Power Efficiency Class", 1, 76, L"0x%x", NULL, NULL, NULL,
> NULL},
> - {L"Reserved", 3, 77, L"%x %x %x", Dump3Chars, NULL, NULL, NULL}
> + {L"Reserved", 1, 77, L"0x%x", NULL, NULL, NULL, NULL}, {L"SPE
> + overflow Interrupt", 2, 78, L"0x%x", NULL, NULL,
> + ValidateSpeOverflowInterrupt, NULL}
> };
>
> /**
> @@ -160,6 +181,55 @@ ValidateGICDSystemVectorBase (
> }
> }
>
> +/**
> + This function validates the SPE Overflow Interrupt in the GICC.
> +
> + @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
> +ValidateSpeOverflowInterrupt (
> + IN UINT8* Ptr,
> + IN VOID* Context
> + )
> +{
> + UINT16 SpeOverflowInterrupt;
> +
> + SpeOverflowInterrupt = *(UINT16*)Ptr;
> +
> + // SPE not supported by this processor if (SpeOverflowInterrupt ==
> + 0) {
> + return;
> + }
> +
> + if ((SpeOverflowInterrupt < ARM_PPI_ID_MIN) ||
> + ((SpeOverflowInterrupt > ARM_PPI_ID_MAX) &&
> + (SpeOverflowInterrupt < ARM_PPI_ID_EXTENDED_MIN)) ||
> + (SpeOverflowInterrupt > ARM_PPI_ID_EXTENDED_MAX)) {
> + IncrementErrorCount ();
> + Print (
> + L"\nERROR: SPE Overflow Interrupt ID of %d is not in the allowed PPI ID "
> + L"ranges of %d-%d or %d-%d (for GICv3.1 or later).",
> + SpeOverflowInterrupt,
> + ARM_PPI_ID_MIN,
> + ARM_PPI_ID_MAX,
> + ARM_PPI_ID_EXTENDED_MIN,
> + ARM_PPI_ID_EXTENDED_MAX
> + );
> + } else if (SpeOverflowInterrupt != ARM_PPI_ID_PMBIRQ) {
> + IncrementWarningCount();
> + Print (
> + L"\nWARNING: SPE Overflow Interrupt ID of %d is not compliant with
> SBSA "
> + L"Level 3 PPI ID assignment: %d.",
> + SpeOverflowInterrupt,
> + ARM_PPI_ID_PMBIRQ
> + );
> + }
> +}
> +
Seems the valid function is working with ARM arch. I am not sure it works fine with other ARCH. If you are not sure, making this function work only with ARM arch is better.
> /**
> This function parses the ACPI MADT table.
> When trace is enabled this function parses the MADT table and @@ -233,7
> +303,7 @@ ParseAcpiMadt (
> }
>
> switch (*MadtInterruptControllerType) {
> - case EFI_ACPI_6_2_GIC: {
> + case EFI_ACPI_6_3_GIC: {
> ParseAcpi (
> TRUE,
> 2,
> @@ -245,7 +315,7 @@ ParseAcpiMadt (
> break;
> }
>
> - case EFI_ACPI_6_2_GICD: {
> + case EFI_ACPI_6_3_GICD: {
> if (++GICDCount > 1) {
> IncrementErrorCount ();
> Print (
> @@ -265,7 +335,7 @@ ParseAcpiMadt (
> break;
> }
>
> - case EFI_ACPI_6_2_GIC_MSI_FRAME: {
> + case EFI_ACPI_6_3_GIC_MSI_FRAME: {
> ParseAcpi (
> TRUE,
> 2,
> @@ -277,7 +347,7 @@ ParseAcpiMadt (
> break;
> }
>
> - case EFI_ACPI_6_2_GICR: {
> + case EFI_ACPI_6_3_GICR: {
> ParseAcpi (
> TRUE,
> 2,
> @@ -289,7 +359,7 @@ ParseAcpiMadt (
> break;
> }
>
> - case EFI_ACPI_6_2_GIC_ITS: {
> + case EFI_ACPI_6_3_GIC_ITS: {
> ParseAcpi (
> TRUE,
> 2,
Before using the new definition, you should add the new Acpi63.h first. Or the patch should pending until finish adding the new spec header file. If the adding file would be done by yourself, make these changes as a series patches.
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..462243647e77a53694ccdd887
> 23ed48fbe3c7cef
> --- /dev/null
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> +++ er.h
> @@ -0,0 +1,35 @@
> +/** @file
> + Header file for MADT table parser
> +
> + Copyright (c) 2019, ARM Limited. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> + @par Reference(s):
> + - Arm Generic Interrupt Controller Architecture Specification,
> + GIC architecture version 3 and version 4, issue E
> + - Arm Server Base System Architecture 5.0 **/
> +
> +///
> +/// Level 3 base server system Private Peripheral Inerrupt (PPI) ID
> +assignments ///
> +#define ARM_PPI_ID_OVERFLOW_INTERRUPT_FROM_CNTP 30
> +#define ARM_PPI_ID_OVERFLOW_INTERRUPT_FROM_CNTPS 29
> +#define ARM_PPI_ID_OVERFLOW_INTERRUPT_FROM_CNTHV 28
> +#define ARM_PPI_ID_OVERFLOW_INTERRUPT_FROM_CNTV 27
> +#define ARM_PPI_ID_OVERFLOW_INTERRUPT_FROM_CNTHP 26
> +#define ARM_PPI_ID_GIC_MAINTENANCE_INTERRUPT 25
> +#define ARM_PPI_ID_CTIIRQ 24
> +#define ARM_PPI_ID_PERFORMANCE_MONITORS_INTERRUPT 23
> +#define ARM_PPI_ID_COMMIRQ 22
> +#define ARM_PPI_ID_PMBIRQ 21
> +#define ARM_PPI_ID_CNTHPS 20
> +#define ARM_PPI_ID_CNTHVS 19
> +
> +///
> +/// PPI ID allowed ranges
> +///
> +#define ARM_PPI_ID_MAX 31
> +#define ARM_PPI_ID_MIN 16
> +#define ARM_PPI_ID_EXTENDED_MAX 1119
> +#define ARM_PPI_ID_EXTENDED_MIN 1056
Refer to CCS 2.1 Section 3.3.2, use #ifndef ... #define ... #endif to avoid multiple include.
Thanks,
Zhichao
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41670): https://edk2.groups.io/g/devel/message/41670
Mute This Topic: https://groups.io/mt/31642384/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