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

["Gao, Zhichao" <zhichao.gao@intel.com>]

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