From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: zhichao.gao@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Thu, 30 May 2019 01:54:52 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 May 2019 01:54:52 -0700 X-ExtLoop1: 1 Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 30 May 2019 01:54:52 -0700 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 30 May 2019 01:54:52 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 30 May 2019 01:54:52 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.10]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.188]) with mapi id 14.03.0415.000; Thu, 30 May 2019 16:54:49 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: "Sami.Mujawar@arm.com" , "Carsey, Jaben" , "Ni, Ray" , "Matteo.Carlini@arm.com" , "Stephanie.Hughes-Fitt@arm.com" , "nd@arm.com" Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Topic: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Index: AQHVDAFley+yGW4jxUCNy5TjV69NGaaDcLZg Date: Thu, 30 May 2019 08:54:48 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B7E1C15@SHSMSX101.ccr.corp.intel.com> References: <20190516160621.62264-1-krzysztof.koch@arm.com> In-Reply-To: <20190516160621.62264-1-krzysztof.koch@arm.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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.Mujawar@arm.com; Carsey, Jaben ; Ni, > Ray ; Matteo.Carlini@arm.com; Stephanie.Hughes- > Fitt@arm.com; nd@arm.com > Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: ACPI 6.3 update= for > MADT parser >=20 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1820 >=20 > The ACPI 6.3 specification introduces a 'SPE overflow Interrupt' field a= s part > of the GICC structure. >=20 > Update the MADT parser to decode this field and validate the interrupt I= D > used. >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v1 >=20 > Notes: > v1: > - Decode and validate SPE Overflow Interrupt field [Krzysztof] >=20 >=20 > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser. > c | 86 ++++++++++++++++++-- > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser. > h | 35 ++++++++ > 2 files changed, 113 insertions(+), 8 deletions(-) >=20 > 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 >=20 > - 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 >=20 > @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 > **/ >=20 > #include > #include > #include "AcpiParser.h" > #include "AcpiTableParser.h" > +#include "MadtParser.h" >=20 > // Local Variables > STATIC CONST UINT8* MadtInterruptControllerType; @@ -33,6 +37,21 @@ > ValidateGICDSystemVectorBase ( > IN VOID* Context > ); >=20 > +/** > + 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. > /** > An ACPI_PARSER array describing the GICC Interrupt Controller Structu= re. > **/ > @@ -56,7 +75,9 @@ STATIC CONST ACPI_PARSER GicCParser[] =3D { > {L"MPIDR", 8, 68, L"0x%lx", NULL, NULL, NULL, NULL}, > {L"Processor Power Efficiency Class", 1, 76, L"0x%x", NULL, NULL, NUL= L, > 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} > }; >=20 > /** > @@ -160,6 +181,55 @@ ValidateGICDSystemVectorBase ( > } > } >=20 > +/** > + 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 =3D *(UINT16*)Ptr; > + > + // SPE not supported by this processor if (SpeOverflowInterrupt =3D= =3D > + 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 !=3D ARM_PPI_ID_PMBIRQ) { > + IncrementWarningCount(); > + Print ( > + L"\nWARNING: SPE Overflow Interrupt ID of %d is not compliant wit= h > 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 w= ith ARM arch is better. > /** > This function parses the ACPI MADT table. > When trace is enabled this function parses the MADT table and @@ -233= ,7 > +303,7 @@ ParseAcpiMadt ( > } >=20 > switch (*MadtInterruptControllerType) { > - case EFI_ACPI_6_2_GIC: { > + case EFI_ACPI_6_3_GIC: { > ParseAcpi ( > TRUE, > 2, > @@ -245,7 +315,7 @@ ParseAcpiMadt ( > break; > } >=20 > - case EFI_ACPI_6_2_GICD: { > + case EFI_ACPI_6_3_GICD: { > if (++GICDCount > 1) { > IncrementErrorCount (); > Print ( > @@ -265,7 +335,7 @@ ParseAcpiMadt ( > break; > } >=20 > - 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; > } >=20 > - case EFI_ACPI_6_2_GICR: { > + case EFI_ACPI_6_3_GICR: { > ParseAcpi ( > TRUE, > 2, > @@ -289,7 +359,7 @@ ParseAcpiMadt ( > break; > } >=20 > - 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 p= atches. > 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 avoi= d multiple include. Thanks, Zhichao > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >=20 >=20 >=20 >=20