public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread

end of thread, other threads:[~2019-06-13  1:10 UTC | newest]

Thread overview: 7+ 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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox