From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=UlcFIxXL; spf=pass (domain: arm.com, ip: 40.107.5.56, mailfrom: sami.mujawar@arm.com) Received: from EUR03-VE1-obe.outbound.protection.outlook.com (EUR03-VE1-obe.outbound.protection.outlook.com [40.107.5.56]) by groups.io with SMTP; Wed, 12 Jun 2019 14:03:55 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=N7eheN2uDcFN/OOyuOTRlsm0T6b/w0g+Q39aUUqUwsQ=; b=UlcFIxXL7MY8KKpZoEa6Aun3xhzaNPek4Mbuc4dXcfdoCbvsk6UMUqi2gvOSC5FMpeCBLNAXFG+fLQrK/MniDmVXUmqvNxCHcPI/u4RDGfJPB/e7Jq1y23rmHVykjD4FxO0ch/Y5ane8IxkvBitdc3PGzJ2x9WcJV/V/iaF25Q0= Received: from DB6PR0802MB2375.eurprd08.prod.outlook.com (10.172.228.142) by DB6PR0802MB2183.eurprd08.prod.outlook.com (10.172.227.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1965.14; Wed, 12 Jun 2019 21:03:52 +0000 Received: from DB6PR0802MB2375.eurprd08.prod.outlook.com ([fe80::a46c:aa3a:17bf:7909]) by DB6PR0802MB2375.eurprd08.prod.outlook.com ([fe80::a46c:aa3a:17bf:7909%3]) with mapi id 15.20.1987.010; Wed, 12 Jun 2019 21:03:52 +0000 From: "Sami Mujawar" To: "Gao, Zhichao" , "devel@edk2.groups.io" CC: "Carsey, Jaben" , "Ni, Ray" , Krzysztof Koch , nd Subject: Re: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Topic: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Index: AQHVHQ20i0fK6Fo4JEujJvPWF3Zlb6aUF9cAgAPFqQCAAAF54A== Date: Wed, 12 Jun 2019 21:03:52 +0000 Message-ID: References: <20190607084741.19300-1-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B7E5031@SHSMSX101.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: e3dfca9b-2c4c-4afe-96cb-b4c4edd760c2.0 x-checkrecipientchecked: true authentication-results: spf=none (sender IP is ) smtp.mailfrom=Sami.Mujawar@arm.com; x-originating-ip: [2a00:23c6:5485:a200:d878:de60:dcf3:89c4] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: bd7e7b2e-a165-4f3c-c761-08d6ef79792a x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(4618075)(2017052603328)(7193020);SRVR:DB6PR0802MB2183; x-ms-traffictypediagnostic: DB6PR0802MB2183: x-ms-exchange-purlcount: 1 nodisclaimer: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:4941; x-forefront-prvs: 0066D63CE6 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(39860400002)(396003)(376002)(346002)(366004)(136003)(13464003)(189003)(199004)(74316002)(66476007)(71200400001)(9686003)(46003)(81166006)(76116006)(8676002)(71190400001)(15650500001)(446003)(478600001)(66556008)(86362001)(53936002)(4326008)(7736002)(486006)(186003)(8936002)(33656002)(476003)(305945005)(73956011)(14454004)(966005)(66446008)(25786009)(72206003)(6116002)(11346002)(64756008)(6246003)(99286004)(256004)(229853002)(55016002)(6436002)(2501003)(66946007)(68736007)(2906002)(81156014)(7696005)(76176011)(102836004)(52536014)(54906003)(6306002)(110136005)(6506007)(316002)(53546011)(14444005)(5660300002);DIR:OUT;SFP:1101;SCL:1;SRVR:DB6PR0802MB2183;H:DB6PR0802MB2375.eurprd08.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: LaBp9w1Sk/lpJniRprsqazEsqsu2LthG06DQSg7h/u3m5XJD3KMlZLS5f2oCPFh02YYE+WBJcYPihwHxnU3xMEPD8NbDLG5Yg3NKq65eWp63z3TwO6d5vm8OlEzxk1YWic0WgHvgf67L04qaqinB1SynjsMtLHcN2EkQVmJAo88NP4kuTQEHEZ4UYiVW2O2eaEBfSHJL/nLLx5ZKI5Mv7zVxU6g87zEssUyyFUVVkwhty0TtcToYJey/OM/q7HYZQiQpOeIolr7Dj7Yln/C7jl+CZLPfdWvc5Bgn0hg7BpmTKJKK4wM/ahgMNTBdcDSLP7uvufK55Zqh/+eiCIh/NLGpc3qEy8gSGIi7WJ785akc8a2JycPLz/lAsntgQnSC806Ld2Ha61GMqi7M9UGphuMljiq5DgIW0gT8E5PP55Q= MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: bd7e7b2e-a165-4f3c-c761-08d6ef79792a X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jun 2019 21:03:52.1691 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Sami.Mujawar@arm.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2183 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Sami Mujawar -----Original Message----- From: Krzysztof Koch =20 Sent: 12 June 2019 11:44 AM To: Gao, Zhichao ; devel@edk2.groups.io Cc: Sami Mujawar ; Carsey, Jaben ; Ni, Ray ; nd Subject: RE: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT pa= rser Hi Zhichao, Please find my comments inline below. They are marked with [Krzysztof] Kind regards, Krzysztof -----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 ; nd Subject: RE: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT pa= rser 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 ; Ni,=20 > Ray ; Gao, Zhichao ;=20 > 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=20 > parser >=20 > The ACPI 6.3 specification introduces a 'SPE overflow Interrupt' field=20 > as part of the GICC structure. >=20 > Update the MADT parser to decode this field and validate the interrupt=20 > ID used. >=20 > 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 >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/tree/477_acpiview_spe_v2 >=20 > Notes: > v2: > - Add include sandwich in MadtParser.h [Zhichao] > - Add references to specifications in commit message [Zhichao] >=20 > 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 | 40 +++++++++ > 2 files changed, 118 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 @@=20 > 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 > + ); > + > /** > An ACPI_PARSER array describing the GICC Interrupt Controller Structur= e. > **/ > @@ -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, 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=20 > + 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 P= PI 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=20 > + 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 fo= r ARM arch but not for IA32/X64? If so, it is better to distinguish them. I used to view the MACRO MDE_CPU_A= RM and MDE_CPU_AARCH64. Did they work for this purpose? There would always be a warning if the SpeOverflowInterrupt is unequal to A= RM_PPI_ID_PMBIRQ. Is that expected? [Krzysztof] All these values (ARM_PPI_ID_MIN, ARM_PPI_ID_EXTENDED_MAX and s= o on) are only true for the Arm interrupt model. This validation is perform= ed against the SPE overflow Interrupt field inside the GICC structure (Tabl= e 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 m= eant 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 Architectur= e 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 ( > } >=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, > 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=20 > +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 hel= p 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 Controll= er 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 > + > +#endif // MADT_PARSER_H_ > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >=20