* [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
@ 2019-05-16 16:06 Krzysztof Koch
2019-05-30 8:54 ` [edk2-devel] " Gao, Zhichao
0 siblings, 1 reply; 3+ messages in thread
From: Krzysztof Koch @ 2019-05-16 16:06 UTC (permalink / raw)
To: devel
Cc: Sami.Mujawar, jaben.carsey, ray.ni, Matteo.Carlini,
Stephanie.Hughes-Fitt, nd
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@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/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index a1bf86ade5313f954a77b325c13394cfce133939..59c3df0cc8a080497b517baf36fc63f1e4ab866f 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.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
+ );
+
/**
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
+ );
+ }
+}
+
/**
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,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h
new file mode 100644
index 0000000000000000000000000000000000000000..462243647e77a53694ccdd88723ed48fbe3c7cef
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.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
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-05-16 16:06 [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Krzysztof Koch
@ 2019-05-30 8:54 ` Gao, Zhichao
2019-05-30 10:49 ` Krzysztof Koch
0 siblings, 1 reply; 3+ messages in thread
From: Gao, Zhichao @ 2019-05-30 8:54 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Sami.Mujawar@arm.com, Carsey, Jaben, Ni, Ray,
Matteo.Carlini@arm.com, Stephanie.Hughes-Fitt@arm.com, nd@arm.com
Sorry for late update.
Some minor comments below.
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Friday, May 17, 2019 12:06 AM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Matteo.Carlini@arm.com; Stephanie.Hughes-
> Fitt@arm.com; nd@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@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)'
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-05-30 8:54 ` [edk2-devel] " Gao, Zhichao
@ 2019-05-30 10:49 ` Krzysztof Koch
0 siblings, 0 replies; 3+ messages in thread
From: Krzysztof Koch @ 2019-05-30 10:49 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com
Cc: Sami Mujawar, Carsey, Jaben, Ni, Ray, Matteo Carlini,
Stephanie Hughes-Fitt, nd
Hi Zhichao,
Thanks for the review. My responses are added inline as [Krzysztof].
Kind regards,
Krzysztof
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via Groups.Io
Sent: Thursday, May 30, 2019 9:55
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
Sorry for late update.
Some minor comments below.
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Friday, May 17, 2019 12:06 AM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Matteo.Carlini@arm.com; Stephanie.Hughes-
> Fitt@arm.com; nd@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@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.
[Krzysztof]
This validation is carried out against ARM-related specs. I will now add them to the commit message as references. I'm already referencing them at the beginning of MadtParser.c:
- Arm Generic Interrupt Controller Architecture Specification, GIC architecture version 3 and version 4, issue E
- Arm Server Base System Architecture 5.0
> /**
> 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.
[Krzysztof]
>From ACPI 6.3 Section 5.2.12:
"Supported interrupt models include the PC-AT-compatible dual 8259 interrupt controller, for Intel processor-based systems, the Intel Advanced Programmable Interrupt Controller (APIC) and Intel Streamlined Advanced Programmable Interrupt Controller (SAPIC), and, for ARM processor-based systems, the Generic Interrupt Controller (GIC)."
Consequently, ACPI GICC structures can only be found on ARM platforms.
> /**
> 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.
[Krzysztof]
Acpi63.h is already upstream, see: https://github.com/tianocore/edk2/commit/a40f30398ab0f13cd79048b4b2f98449ebad96cd
> 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.
[Krzysztof]
I will change the header file accordingly. Thanks for spotting that.
Thanks,
Zhichao
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-30 10:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-16 16:06 [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Krzysztof Koch
2019-05-30 8:54 ` [edk2-devel] " Gao, Zhichao
2019-05-30 10:49 ` Krzysztof Koch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox