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.115, mailfrom: zhichao.gao@intel.com) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by groups.io with SMTP; Wed, 12 Jun 2019 18:10:36 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jun 2019 18:10:36 -0700 X-ExtLoop1: 1 Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 12 Jun 2019 18:10:36 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 12 Jun 2019 18:10:36 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 12 Jun 2019 18:10:35 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.104]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.185]) with mapi id 14.03.0439.000; Thu, 13 Jun 2019 09:10:33 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: Sami Mujawar , "Carsey, Jaben" , "Ni, Ray" , nd Subject: Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Topic: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Index: AQHVHQ28QfuD3sQkeEC+0r3UnREtd6aUEBSwgANHUACAAXY48A== Date: Thu, 13 Jun 2019 01:10:32 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B7F302A@SHSMSX101.ccr.corp.intel.com> References: <20190607084741.19300-1-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B7E5031@SHSMSX101.ccr.corp.intel.com> In-Reply-To: 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 Thanks for the interpret. Seems I read the old version spec. Now it is clea= r for me. Reviewed-by: Zhichao Gao > -----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 ; devel@edk2.groups.io > Cc: Sami Mujawar ; Carsey, Jaben > ; Ni, Ray ; nd > Subject: Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 up= date > for MADT parser >=20 > Hi Zhichao, >=20 > Please find my comments inline below. They are marked with [Krzysztof] >=20 > Kind regards, >=20 > Krzysztof >=20 > -----Original Message----- > From: Gao, Zhichao > Sent: Monday, June 10, 2019 2:08 > To: Krzysztof Koch ; devel@edk2.groups.io > Cc: Sami Mujawar ; Carsey, Jaben > ; Ni, Ray ; Matteo Carlini > ; Stephanie Hughes-Fitt Fitt@arm.com>; nd > Subject: RE: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT > parser >=20 > Sorry for late update. >=20 > > -----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 ; Ni, > > Ray ; Gao, Zhichao ; > > 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 > > --- > > > > 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 > > #include > > #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. th= is > > + 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 Struc= ture. > > **/ > > @@ -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, N= ULL, > > 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. th= is > > + 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 allowe= d 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 > > + with > > SBSA " > > + L"Level 3 PPI ID assignment: %d.", > > + SpeOverflowInterrupt, > > + ARM_PPI_ID_PMBIRQ > > + ); > > + } > > +} > > + >=20 > 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 t= o > ARM_PPI_ID_PMBIRQ. Is that expected? >=20 > [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 f= ield > 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. Therefo= re, > I don't think it is necessary to enclose this code in the macro > 'MDE_CPU_ARM'. >=20 > [Krzysztof] To be level 3 compliant with Arm Server Base System Architec= ture > 5.0 one should use Interrupt ID of 21 for Statistical Profiling Interrup= t, if > Statistical Profiling Extensions are implemented (see Section 4.1.5 of S= BSA). > That's why there is a warning (not error) if the value provided is diffe= rent > from 21. >=20 > > /** > > 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 >=20 > 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 he= lp > to review. >=20 > [Krzysztof] The definition of the SPE overflow Interrupt field states th= at " This > interrupt is a level triggered PPI". Arm Generic Interrupt Controller > Architecture Specification, GIC architecture version 3 and version 4, is= sue E > defines the valid ranges for INTIDs in Table 2-1. >=20 > Thanks, > Zhichao >=20 > > + > > +#endif // MADT_PARSER_H_ > > -- > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > >=20 >=20 >=20