* [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
@ 2019-06-07 8:47 Krzysztof Koch
2019-06-07 8:54 ` [edk2-devel] " Alexei.Fedorov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Krzysztof Koch @ 2019-06-07 8:47 UTC (permalink / raw)
To: devel
Cc: Sami.Mujawar, jaben.carsey, ray.ni, zhichao.gao, Matteo.Carlini,
Stephanie.Hughes-Fitt, nd
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.
References:
- 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
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
Notes:
v2:
- Add include sandwich in MadtParser.h [Zhichao]
- Add references to specifications in commit message [Zhichao]
v1:
- Decode and validate SPE Overflow Interrupt field [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 86 ++++++++++++++++++--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h | 40 +++++++++
2 files changed, 118 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..fbbc43e09adbdf9fea302a03a61e6dc179f06a62
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h
@@ -0,0 +1,40 @@
+/** @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
+**/
+
+#ifndef MADT_PARSER_H_
+#define MADT_PARSER_H_
+
+///
+/// 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
+
+#endif // MADT_PARSER_H_
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-06-07 8:47 [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Krzysztof Koch
@ 2019-06-07 8:54 ` Alexei.Fedorov
2019-06-07 16:59 ` Sami Mujawar
2019-06-10 1:08 ` Gao, Zhichao
2 siblings, 0 replies; 8+ messages in thread
From: Alexei.Fedorov @ 2019-06-07 8:54 UTC (permalink / raw)
To: Krzysztof Koch, devel
[-- Attachment #1: Type: text/plain, Size: 54 bytes --]
Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
[-- Attachment #2: Type: text/html, Size: 60 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-06-07 8:47 [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Krzysztof Koch
2019-06-07 8:54 ` [edk2-devel] " Alexei.Fedorov
@ 2019-06-07 16:59 ` Sami Mujawar
2019-06-10 1:08 ` Gao, Zhichao
2 siblings, 0 replies; 8+ messages in thread
From: Sami Mujawar @ 2019-06-07 16:59 UTC (permalink / raw)
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com, ray.ni@intel.com, zhichao.gao@intel.com,
Krzysztof Koch, nd
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com>
Sent: 07 June 2019 09:48 AM
To: devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; jaben.carsey@intel.com; ray.ni@intel.com; zhichao.gao@intel.com; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
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.
References:
- 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
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
Notes:
v2:
- Add include sandwich in MadtParser.h [Zhichao]
- Add references to specifications in commit message [Zhichao]
v1:
- Decode and validate SPE Overflow Interrupt field [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 86 ++++++++++++++++++-- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h | 40 +++++++++
2 files changed, 118 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/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
+ );
+
/**
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..fbbc43e09adbdf9fea302a03a61e6dc179f06a62
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
+++ er.h
@@ -0,0 +1,40 @@
+/** @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 **/
+
+#ifndef MADT_PARSER_H_
+#define MADT_PARSER_H_
+
+///
+/// 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
+
+#endif // MADT_PARSER_H_
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-06-07 8:47 [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Krzysztof Koch
2019-06-07 8:54 ` [edk2-devel] " Alexei.Fedorov
2019-06-07 16:59 ` Sami Mujawar
@ 2019-06-10 1:08 ` Gao, Zhichao
2019-06-12 10:44 ` Krzysztof Koch
2 siblings, 1 reply; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-10 1:08 UTC (permalink / raw)
To: Krzysztof Koch, devel@edk2.groups.io
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.
> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, June 7, 2019 4:48 PM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; nd@arm.com
> Subject: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
>
> 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.
>
> References:
> - 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
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
>
> Notes:
> v2:
> - Add include sandwich in MadtParser.h [Zhichao]
> - Add references to specifications in commit message [Zhichao]
>
> v1:
> - Decode and validate SPE Overflow Interrupt field [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 86 ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> h | 40 +++++++++
> 2 files changed, 118 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
> + );
> +
> /**
> 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
> + );
> + }
> +}
> +
The checking values are named with ARM prefix. Can I think they are only for ARM arch but not for IA32/X64?
If so, it is better to distinguish them. I used to view the MACRO MDE_CPU_ARM and MDE_CPU_AARCH64. Did they work for this purpose?
There would always be a warning if the SpeOverflowInterrupt is unequal to ARM_PPI_ID_PMBIRQ. Is that expected?
> /**
> 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/MadtPars
> er.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..fbbc43e09adbdf9fea302a03a6
> 1e6dc179f06a62
> --- /dev/null
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> +++ er.h
> @@ -0,0 +1,40 @@
> +/** @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 **/
> +
> +#ifndef MADT_PARSER_H_
> +#define MADT_PARSER_H_
> +
> +///
> +/// 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
I can't find the info about "ARM_PPI_ID_EXTENDED_MAX 1119". Can you help to clarify it? Is this for future SBSA usage.
I am not familiar with the ARM things. Someone is export in ARM could help to review.
Thanks,
Zhichao
> +
> +#endif // MADT_PARSER_H_
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-06-10 1:08 ` Gao, Zhichao
@ 2019-06-12 10:44 ` Krzysztof Koch
2019-06-12 21:03 ` Sami Mujawar
2019-06-13 1:10 ` [edk2-devel] " Gao, Zhichao
0 siblings, 2 replies; 8+ messages in thread
From: Krzysztof Koch @ 2019-06-12 10:44 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Sami Mujawar, Carsey, Jaben, Ni, Ray, nd
Hi Zhichao,
Please find my comments inline below. They are marked with [Krzysztof]
Kind regards,
Krzysztof
-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com>
Sent: Monday, June 10, 2019 2:08
To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
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: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
Sorry for late update.
> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, June 7, 2019 4:48 PM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; nd@arm.com
> Subject: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT
> parser
>
> 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.
>
> References:
> - 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
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
>
> Notes:
> v2:
> - Add include sandwich in MadtParser.h [Zhichao]
> - Add references to specifications in commit message [Zhichao]
>
> v1:
> - Decode and validate SPE Overflow Interrupt field [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 86 ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> h | 40 +++++++++
> 2 files changed, 118 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
> + );
> +
> /**
> 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
> + );
> + }
> +}
> +
The checking values are named with ARM prefix. Can I think they are only for ARM arch but not for IA32/X64?
If so, it is better to distinguish them. I used to view the MACRO MDE_CPU_ARM and MDE_CPU_AARCH64. Did they work for this purpose?
There would always be a warning if the SpeOverflowInterrupt is unequal to ARM_PPI_ID_PMBIRQ. Is that expected?
[Krzysztof] All these values (ARM_PPI_ID_MIN, ARM_PPI_ID_EXTENDED_MAX and so on) are only true for the Arm interrupt model. This validation is performed against the SPE overflow Interrupt field inside the GICC structure (Table 5-60, ACPI 6.3). If you have the GICC structure in your MADT table, then it is implicit that you are using the Arm interrupt model and the code is meant to run on an Arm platform. Therefore, I don't think it is necessary to enclose this code in the macro 'MDE_CPU_ARM'.
[Krzysztof] To be level 3 compliant with Arm Server Base System Architecture 5.0 one should use Interrupt ID of 21 for Statistical Profiling Interrupt, if Statistical Profiling Extensions are implemented (see Section 4.1.5 of SBSA). That's why there is a warning (not error) if the value provided is different from 21.
> /**
> 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/MadtPars
> er.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..fbbc43e09adbdf9fea302a03a6
> 1e6dc179f06a62
> --- /dev/null
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> +++ er.h
> @@ -0,0 +1,40 @@
> +/** @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 **/
> +
> +#ifndef MADT_PARSER_H_
> +#define MADT_PARSER_H_
> +
> +///
> +/// 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
I can't find the info about "ARM_PPI_ID_EXTENDED_MAX 1119". Can you help to clarify it? Is this for future SBSA usage.
I am not familiar with the ARM things. Someone is export in ARM could help to review.
[Krzysztof] The definition of the SPE overflow Interrupt field states that " This interrupt is a level triggered PPI". Arm Generic Interrupt Controller Architecture Specification, GIC architecture version 3 and version 4, issue E defines the valid ranges for INTIDs in Table 2-1.
Thanks,
Zhichao
> +
> +#endif // MADT_PARSER_H_
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-06-12 10:44 ` Krzysztof Koch
@ 2019-06-12 21:03 ` Sami Mujawar
2019-06-13 1:10 ` [edk2-devel] " Gao, Zhichao
1 sibling, 0 replies; 8+ messages in thread
From: Sami Mujawar @ 2019-06-12 21:03 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Carsey, Jaben, Ni, Ray, Krzysztof Koch, nd
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
-----Original Message-----
From: Krzysztof Koch <Krzysztof.Koch@arm.com>
Sent: 12 June 2019 11:44 AM
To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; nd <nd@arm.com>
Subject: RE: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
Hi Zhichao,
Please find my comments inline below. They are marked with [Krzysztof]
Kind regards,
Krzysztof
-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com>
Sent: Monday, June 10, 2019 2:08
To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
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: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
Sorry for late update.
> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, June 7, 2019 4:48 PM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; nd@arm.com
> Subject: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT
> parser
>
> 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.
>
> References:
> - 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
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
>
> Notes:
> v2:
> - Add include sandwich in MadtParser.h [Zhichao]
> - Add references to specifications in commit message [Zhichao]
>
> v1:
> - Decode and validate SPE Overflow Interrupt field [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 86 ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> h | 40 +++++++++
> 2 files changed, 118 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
> + );
> +
> /**
> 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
> + );
> + }
> +}
> +
The checking values are named with ARM prefix. Can I think they are only for ARM arch but not for IA32/X64?
If so, it is better to distinguish them. I used to view the MACRO MDE_CPU_ARM and MDE_CPU_AARCH64. Did they work for this purpose?
There would always be a warning if the SpeOverflowInterrupt is unequal to ARM_PPI_ID_PMBIRQ. Is that expected?
[Krzysztof] All these values (ARM_PPI_ID_MIN, ARM_PPI_ID_EXTENDED_MAX and so on) are only true for the Arm interrupt model. This validation is performed against the SPE overflow Interrupt field inside the GICC structure (Table 5-60, ACPI 6.3). If you have the GICC structure in your MADT table, then it is implicit that you are using the Arm interrupt model and the code is meant to run on an Arm platform. Therefore, I don't think it is necessary to enclose this code in the macro 'MDE_CPU_ARM'.
[Krzysztof] To be level 3 compliant with Arm Server Base System Architecture 5.0 one should use Interrupt ID of 21 for Statistical Profiling Interrupt, if Statistical Profiling Extensions are implemented (see Section 4.1.5 of SBSA). That's why there is a warning (not error) if the value provided is different from 21.
> /**
> 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/MadtPars
> er.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> er.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..fbbc43e09adbdf9fea302a03a6
> 1e6dc179f06a62
> --- /dev/null
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> +++ er.h
> @@ -0,0 +1,40 @@
> +/** @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 **/
> +
> +#ifndef MADT_PARSER_H_
> +#define MADT_PARSER_H_
> +
> +///
> +/// 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
I can't find the info about "ARM_PPI_ID_EXTENDED_MAX 1119". Can you help to clarify it? Is this for future SBSA usage.
I am not familiar with the ARM things. Someone is export in ARM could help to review.
[Krzysztof] The definition of the SPE overflow Interrupt field states that " This interrupt is a level triggered PPI". Arm Generic Interrupt Controller Architecture Specification, GIC architecture version 3 and version 4, issue E defines the valid ranges for INTIDs in Table 2-1.
Thanks,
Zhichao
> +
> +#endif // MADT_PARSER_H_
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
2019-06-12 10:44 ` Krzysztof Koch
2019-06-12 21:03 ` Sami Mujawar
@ 2019-06-13 1:10 ` Gao, Zhichao
1 sibling, 0 replies; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-13 1:10 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Sami Mujawar, Carsey, Jaben, Ni, Ray, nd
Thanks for the interpret. Seems I read the old version spec. Now it is clear for me.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Wednesday, June 12, 2019 6:44 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Carsey, Jaben
> <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update
> for MADT parser
>
> Hi Zhichao,
>
> Please find my comments inline below. They are marked with [Krzysztof]
>
> Kind regards,
>
> Krzysztof
>
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, June 10, 2019 2:08
> To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
> 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: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT
> parser
>
> Sorry for late update.
>
> > -----Original Message-----
> > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> > Sent: Friday, June 7, 2019 4:48 PM
> > To: devel@edk2.groups.io
> > Cc: Sami.Mujawar@arm.com; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> > Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; nd@arm.com
> > Subject: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT
> > parser
> >
> > 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.
> >
> > References:
> > - 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
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Changes can be seen at:
> > https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
> >
> > Notes:
> > v2:
> > - Add include sandwich in MadtParser.h [Zhichao]
> > - Add references to specifications in commit message [Zhichao]
> >
> > v1:
> > - Decode and validate SPE Overflow Interrupt field [Krzysztof]
> >
> >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> > c | 86 ++++++++++++++++++--
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> > h | 40 +++++++++
> > 2 files changed, 118 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
> > + );
> > +
> > /**
> > 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
> > + );
> > + }
> > +}
> > +
>
> The checking values are named with ARM prefix. Can I think they are only for
> ARM arch but not for IA32/X64?
> If so, it is better to distinguish them. I used to view the MACRO
> MDE_CPU_ARM and MDE_CPU_AARCH64. Did they work for this purpose?
> There would always be a warning if the SpeOverflowInterrupt is unequal to
> ARM_PPI_ID_PMBIRQ. Is that expected?
>
> [Krzysztof] All these values (ARM_PPI_ID_MIN,
> ARM_PPI_ID_EXTENDED_MAX and so on) are only true for the Arm interrupt
> model. This validation is performed against the SPE overflow Interrupt field
> inside the GICC structure (Table 5-60, ACPI 6.3). If you have the GICC
> structure in your MADT table, then it is implicit that you are using the Arm
> interrupt model and the code is meant to run on an Arm platform. Therefore,
> I don't think it is necessary to enclose this code in the macro
> 'MDE_CPU_ARM'.
>
> [Krzysztof] To be level 3 compliant with Arm Server Base System Architecture
> 5.0 one should use Interrupt ID of 21 for Statistical Profiling Interrupt, if
> Statistical Profiling Extensions are implemented (see Section 4.1.5 of SBSA).
> That's why there is a warning (not error) if the value provided is different
> from 21.
>
> > /**
> > 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/MadtPars
> > er.h
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> > er.h
> > new file mode 100644
> > index
> >
> 0000000000000000000000000000000000000000..fbbc43e09adbdf9fea302a03a6
> > 1e6dc179f06a62
> > --- /dev/null
> > +++
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
> > +++ er.h
> > @@ -0,0 +1,40 @@
> > +/** @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 **/
> > +
> > +#ifndef MADT_PARSER_H_
> > +#define MADT_PARSER_H_
> > +
> > +///
> > +/// 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
>
> I can't find the info about "ARM_PPI_ID_EXTENDED_MAX 1119". Can you
> help to clarify it? Is this for future SBSA usage.
> I am not familiar with the ARM things. Someone is export in ARM could help
> to review.
>
> [Krzysztof] The definition of the SPE overflow Interrupt field states that " This
> interrupt is a level triggered PPI". Arm Generic Interrupt Controller
> Architecture Specification, GIC architecture version 3 and version 4, issue E
> defines the valid ranges for INTIDs in Table 2-1.
>
> Thanks,
> Zhichao
>
> > +
> > +#endif // MADT_PARSER_H_
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser
@ 2019-05-30 12:06 Krzysztof Koch
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Koch @ 2019-05-30 12:06 UTC (permalink / raw)
To: devel
Cc: Sami.Mujawar, jaben.carsey, ray.ni, zhichao.gao, Matteo.Carlini,
Stephanie.Hughes-Fitt, nd
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.
References:
- 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
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2
Notes:
v2:
- Add include sandwich in MadtParser.h [Zhichao]
- Add references to specifications in commit message [Zhichao]
v1:
- Decode and validate SPE Overflow Interrupt field [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 86 ++++++++++++++++++--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h | 40 +++++++++
2 files changed, 118 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..fbbc43e09adbdf9fea302a03a61e6dc179f06a62
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.h
@@ -0,0 +1,40 @@
+/** @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
+**/
+
+#ifndef MADT_PARSER_H_
+#define MADT_PARSER_H_
+
+///
+/// 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
+
+#endif // MADT_PARSER_H_
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-13 1:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-07 8:47 [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Krzysztof Koch
2019-06-07 8:54 ` [edk2-devel] " Alexei.Fedorov
2019-06-07 16:59 ` Sami Mujawar
2019-06-10 1:08 ` Gao, Zhichao
2019-06-12 10:44 ` Krzysztof Koch
2019-06-12 21:03 ` Sami Mujawar
2019-06-13 1:10 ` [edk2-devel] " Gao, Zhichao
-- strict thread matches above, loose matches on Subject: below --
2019-05-30 12:06 Krzysztof Koch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox