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

["Krzysztof Koch" <krzysztof.koch@arm.com>]

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)'
>