Re: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser

["Gao, Zhichao" <zhichao.gao@intel.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)'
>