Thanks for the interpret. Seems I read the old version spec. Now it is clear for me.
Reviewed-by: Zhichao Gao <zhichao....@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....@intel.com>; devel@edk2.groups.io > Cc: Sami Mujawar <sami.muja...@arm.com>; Carsey, Jaben > <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; nd <n...@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....@intel.com> > Sent: Monday, June 10, 2019 2:08 > To: Krzysztof Koch <krzysztof.k...@arm.com>; devel@edk2.groups.io > Cc: Sami Mujawar <sami.muja...@arm.com>; Carsey, Jaben > <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; Matteo Carlini > <matteo.carl...@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes- > f...@arm.com>; nd <n...@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.k...@arm.com] > > Sent: Friday, June 7, 2019 4:48 PM > > To: devel@edk2.groups.io > > Cc: sami.muja...@arm.com; Carsey, Jaben <jaben.car...@intel.com>; Ni, > > Ray <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; > > matteo.carl...@arm.com; stephanie.hughes-f...@arm.com; n...@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.k...@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)' > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42320): https://edk2.groups.io/g/devel/message/42320 Mute This Topic: https://groups.io/mt/31972729/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
