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=TB+oStnC; spf=pass (domain: arm.com, ip: 40.107.7.41, mailfrom: krzysztof.koch@arm.com) Received: from EUR04-HE1-obe.outbound.protection.outlook.com (EUR04-HE1-obe.outbound.protection.outlook.com [40.107.7.41]) by groups.io with SMTP; Wed, 12 Jun 2019 03:44:30 -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=LTqzpCKOwrQ/9U8EgC4kU4Za1MrQubcPyl2XDRaczlY=; b=TB+oStnCZ6e8UinDdh52PwaKVJgDc9ZjZ5Oow7PaacjpchYRnNSwUY0xti5kJ54hPuQco59hLc6E/9Y+9P64TW8AhmbNhzuVW2tcdfiDV5edJe/WPxFuugcsgE11sB+xnGT6LG2SaH3Ih5N3snU2eGzbIuMf+66zVVhBC3F10Tg= Received: from VE1PR08MB4783.eurprd08.prod.outlook.com (10.255.114.16) by VE1PR08MB4685.eurprd08.prod.outlook.com (10.255.115.74) 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 10:44:26 +0000 Received: from VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::c405:d5d9:604c:dab5]) by VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::c405:d5d9:604c:dab5%5]) with mapi id 15.20.1965.017; Wed, 12 Jun 2019 10:44:26 +0000 From: "Krzysztof Koch" 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 parser Thread-Topic: [PATCH v2 1/1] ShellPkg: acpiview: ACPI 6.3 update for MADT parser Thread-Index: AQHVHyj5u/RXS60/i0qMyi2VhhU+k6aX0gYg Date: Wed, 12 Jun 2019 10:44:26 +0000 Message-ID: References: <20190607084741.19300-1-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B7E5031@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B7E5031@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: 3b6761a3-6a47-4a0d-bb44-a6f94340fdc8.1 x-checkrecipientchecked: true authentication-results: spf=none (sender IP is ) smtp.mailfrom=Krzysztof.Koch@arm.com; x-originating-ip: [217.140.106.53] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 3141644e-519c-4d46-261f-08d6ef22f09a 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:VE1PR08MB4685; x-ms-traffictypediagnostic: VE1PR08MB4685: x-ms-exchange-purlcount: 1 nodisclaimer: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:7691; x-forefront-prvs: 0066D63CE6 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(136003)(366004)(396003)(346002)(376002)(39860400002)(189003)(199004)(13464003)(11346002)(478600001)(68736007)(476003)(33656002)(486006)(7736002)(7696005)(305945005)(6506007)(2501003)(446003)(76176011)(15650500001)(26005)(53936002)(9686003)(6246003)(6436002)(186003)(229853002)(3846002)(6116002)(102836004)(74316002)(76116006)(55016002)(316002)(2906002)(53546011)(6306002)(71190400001)(66946007)(66446008)(25786009)(64756008)(8936002)(66556008)(71200400001)(73956011)(54906003)(99286004)(4326008)(66476007)(66066001)(86362001)(81166006)(966005)(14454004)(14444005)(110136005)(52536014)(81156014)(8676002)(5660300002)(72206003)(256004);DIR:OUT;SFP:1101;SCL:1;SRVR:VE1PR08MB4685;H:VE1PR08MB4783.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: lcE2s2+GEojevR0ZgGAbQdQaZHFVQM6PtUwdqLzXhNtOYE1NMUCigp7l3bFCIPV1Px4TBOc9NzxTHFVdkdd4YiowwMLsflTS6ngJxmltuj1Kc2TWDRnWlVht5fRrv8jYI8mpFlE8e3rXyDqK6ioaF8ZOu0w/PilKKOSZ4LCFglmQq4N4yjQQHcsweuo4twNZkxazEKd7/HTSVRd7vHDJL7t+HHMy8pc+cQP25m4fNyXoBEvjfvugj92fQEGMcUoG9v7FTf7yVe8jXxR/fp8fFGE1yPw0vq6pzaEKFV13ly5ZNfQ/gNmoDWTvHmR+e/NMiFZjm98EBE/fWoO8DEgDX920nac0Zl7b1abtBr8g/ags7kFcsKhsJA/Vic+2fsva0Nj9nZKau5uOQYlGZa5twZUU+oUv2ujwYc/ABSrsC9Q= MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3141644e-519c-4d46-261f-08d6ef22f09a X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jun 2019 10:44:26.2345 (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: Krzysztof.Koch@arm.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR08MB4685 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Zhichao, Please find my comments inline below. They are marked with [Krzysztof] Kind regards, Krzysztof -----Original Message----- From: Gao, Zhichao =20 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 @@=20 > -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