Hi Zhichao, Thanks for the review. My responses are added inline as [Krzysztof].
Kind regards, Krzysztof -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via Groups.Io Sent: Thursday, May 30, 2019 9:55 To: devel@edk2.groups.io; Krzysztof Koch <krzysztof.k...@arm.com> 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: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Sorry for late update. Some minor comments below. > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Krzysztof Koch > Sent: Friday, May 17, 2019 12:06 AM > To: devel@edk2.groups.io > Cc: sami.muja...@arm.com; Carsey, Jaben <jaben.car...@intel.com>; Ni, > Ray <ray...@intel.com>; matteo.carl...@arm.com; Stephanie.Hughes- > f...@arm.com; n...@arm.com > Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 > update for MADT parser > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1820 > > 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. > > Signed-off-by: Krzysztof Koch <krzysztof.k...@arm.com> > --- > > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v1 > > Notes: > v1: > - Decode and validate SPE Overflow Interrupt field [Krzysztof] > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser. > c | 86 ++++++++++++++++++-- > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser. > h | 35 ++++++++ > 2 files changed, 113 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 > + ); > + >From ACPI 6.3 Table 5-60: Statistical Profiling Extension buffer overflow GSIV. This interrupt is a level triggered PPI. Zero if SPE is not supported by this processor. Seems it did descript which value is invalid. If it is mentioned in other spec, please help to figure out in the commit message. Maybe I miss something. If so, please help to point. [Krzysztof] This validation is carried out against ARM-related specs. I will now add them to the commit message as references. I'm already referencing them at the beginning of MadtParser.c: - Arm Generic Interrupt Controller Architecture Specification, GIC architecture version 3 and version 4, issue E - Arm Server Base System Architecture 5.0 > /** > 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 > + ); > + } > +} > + Seems the valid function is working with ARM arch. I am not sure it works fine with other ARCH. If you are not sure, making this function work only with ARM arch is better. [Krzysztof] >From ACPI 6.3 Section 5.2.12: "Supported interrupt models include the PC-AT-compatible dual 8259 interrupt controller, for Intel processor-based systems, the Intel Advanced Programmable Interrupt Controller (APIC) and Intel Streamlined Advanced Programmable Interrupt Controller (SAPIC), and, for ARM processor-based systems, the Generic Interrupt Controller (GIC)." Consequently, ACPI GICC structures can only be found on ARM platforms. > /** > 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, Before using the new definition, you should add the new Acpi63.h first. Or the patch should pending until finish adding the new spec header file. If the adding file would be done by yourself, make these changes as a series patches. [Krzysztof] Acpi63.h is already upstream, see: https://github.com/tianocore/edk2/commit/a40f30398ab0f13cd79048b4b2f98449ebad96cd > 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..462243647e77a53694ccdd887 > 23ed48fbe3c7cef > --- /dev/null > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars > +++ er.h > @@ -0,0 +1,35 @@ > +/** @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 **/ > + > +/// > +/// 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 Refer to CCS 2.1 Section 3.3.2, use #ifndef ... #define ... #endif to avoid multiple include. [Krzysztof] I will change the header file accordingly. Thanks for spotting that. Thanks, Zhichao > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41673): https://edk2.groups.io/g/devel/message/41673 Mute This Topic: https://groups.io/mt/31642384/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
