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=HMouHXTx; spf=pass (domain: arm.com, ip: 40.107.5.63, mailfrom: krzysztof.koch@arm.com) Received: from EUR03-VE1-obe.outbound.protection.outlook.com (EUR03-VE1-obe.outbound.protection.outlook.com [40.107.5.63]) by groups.io with SMTP; Mon, 01 Jul 2019 00:28:53 -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=llu2u3l4jHMjOg0LpByVOKVKIou1XpLBjjSFIex6zN8=; b=HMouHXTxUoH+UbUTVe/e322yRr41IDF7xGnHOjyheROQHRirm9/+um/dBtYNW1GqUDvFwuDVEIfuL0BC+VfnN2KDZrC80bmVIm/1N0Ro0QRR07e/ekMRKSWSFbEod3BhoR8N9FwtbrH9/mXbQbDMlGMgUdVvy9CI6fGMJ0bm3Oo= Received: from VE1PR08MB4783.eurprd08.prod.outlook.com (10.255.114.16) by VE1PR08MB4989.eurprd08.prod.outlook.com (10.255.158.214) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2032.20; Mon, 1 Jul 2019 07:28:50 +0000 Received: from VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::9554:1492:6d83:be6b]) by VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::9554:1492:6d83:be6b%4]) with mapi id 15.20.2032.019; Mon, 1 Jul 2019 07:28:49 +0000 From: "Krzysztof Koch" To: "devel@edk2.groups.io" , "zhichao.gao@intel.com" CC: nd Subject: Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation Thread-Topic: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation Thread-Index: AQHVL7/6iTZ9ALO6YE28BWxuWl+8G6a1WWXw Date: Mon, 1 Jul 2019 07:28:49 +0000 Message-ID: References: <20190628102438.30544-1-krzysztof.koch@arm.com> <20190628102438.30544-2-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B8003A3@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B8003A3@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: cb7a9cb5-2879-4b6c-8ee8-b5f9ed1398a4.1 x-checkrecipientchecked: true authentication-results: spf=none (sender IP is ) smtp.mailfrom=Krzysztof.Koch@arm.com; x-originating-ip: [217.140.106.55] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 7763c5df-c588-4e9f-3a3a-08d6fdf5c2f9 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:VE1PR08MB4989; x-ms-traffictypediagnostic: VE1PR08MB4989: x-ms-exchange-purlcount: 4 nodisclaimer: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:1850; x-forefront-prvs: 00851CA28B x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(366004)(376002)(136003)(346002)(396003)(39850400004)(51444003)(13464003)(199004)(189003)(186003)(26005)(5660300002)(256004)(25786009)(4326008)(14454004)(52536014)(76116006)(66946007)(64756008)(66556008)(66476007)(7696005)(66066001)(6506007)(68736007)(966005)(73956011)(6116002)(3846002)(76176011)(102836004)(53546011)(86362001)(99286004)(305945005)(229853002)(8936002)(2906002)(66446008)(2501003)(55016002)(6246003)(74316002)(53936002)(8676002)(81166006)(110136005)(81156014)(316002)(33656002)(476003)(6306002)(486006)(9686003)(478600001)(6436002)(72206003)(71200400001)(71190400001)(7736002)(446003)(11346002);DIR:OUT;SFP:1101;SCL:1;SRVR:VE1PR08MB4989;H:VE1PR08MB4783.eurprd08.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX: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: fbm4vF2dZlHF5Ad5/6fZVKGz6S+6TgPeTeFHrFLTZqsRx2Ul1lbm99iRPejrYnDefdEApoM/fH5kJAfKNItLTS3HSVLzCDU/v9pD9TAQ3lHpXJDb8Ae0/Ue8NwT8jVHxezt0bFi2RBzzPpjSJCPH5lj9i6W6exEnDWavvkBXRt+riDnSCkVfr9u0FyCt61iygMweqm1MlrZwDASJPwkRaXiZG/kKhaKvwNDKL7TF+0axqoMuK8khXUlbkfc2mJ1mKV8ARt10CzSoMPqKaZRSlp+WfEsKfH4k/+jsitbPmFo3q45kM1DiJW0M5vk/scxb5FKsGfDnJ5ys3vG64Cs0tztQC2rqMBQPCQVN8AXmG0odcaZYWeqZOTqhuu9LJ377TxXOSa2DBCjEFqkzKnIhuZpo1SXTAUpRQxId5QRrNy0= MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7763c5df-c588-4e9f-3a3a-08d6fdf5c2f9 X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Jul 2019 07:28:49.7732 (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: VE1PR08MB4989 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Zhichao, Thank you for the review. You can see my responses in line with your comme= nts marked with [Krzysztof] Kind regards, Krzysztof -----Original Message----- From: devel@edk2.groups.io On Behalf Of Gao, Zhicha= o via Groups.Io Sent: Monday, July 1, 2019 4:49 To: Krzysztof Koch ; devel@edk2.groups.io Cc: Carsey, Jaben ; Ni, Ray ; Sa= mi Mujawar ; Matteo Carlini ;= nd Subject: Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT = table field validation Minor comment on ValidateCacheAssociativity: > -----Original Message----- > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com] > Sent: Friday, June 28, 2019 6:25 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray=20 > ; Gao, Zhichao ;=20 > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com > Subject: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field=20 > validation >=20 > Add Cache Structure (Type 1) 'Number of sets' and 'Associativity' > field validation in the acpiview Processor Properties Topology Table=20 > (PPTT) parser. >=20 > Replace literal values with precompiler macros for existing Cache=20 > Structure validation functions. >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d46 > 5cac779badc3c79982 >=20 > Notes: > v1: > - Use macros to define constant values used for validation [Krzyszto= f] > - Add two new PPTT Type 1 structure validation functions=20 > [Krzysztof] >=20 > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > | 102 ++++++++++++++++++-- > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h > | 38 ++++++++ > 2 files changed, 130 insertions(+), 10 deletions(-) >=20 > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser. > c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser. > c > index > 71b6e7ae7c727ee0ea12f74e60c27c4c46e05872..cec57be55e77096f9448f637e > a129af2b42111ad 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser. > c > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars > +++ er.c > @@ -5,12 +5,15 @@ > 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 Architecture Reference Manual ARMv8 (D.a) > **/ >=20 > #include > #include > #include "AcpiParser.h" > +#include "AcpiView.h" > +#include "PpttParser.h" >=20 > // Local variables > STATIC CONST UINT8* ProcessorTopologyStructureType; @@ -19,11 +22,80= =20 > @@ STATIC CONST UINT32* NumberOfPrivateResources; STATIC=20 > ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; >=20 > /** > - An ACPI_PARSER array describing the ACPI PPTT Table. > + This function validates the Cache Type Structure (Type 1) 'Number of = sets' > + field. > + > + @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 CONST ACPI_PARSER PpttParser[] =3D { > - PARSE_ACPI_HEADER (&AcpiHdrInfo) > -}; > +STATIC > +VOID > +EFIAPI > +ValidateCacheNumberOfSets ( > + IN UINT8* Ptr, > + IN VOID* Context > + ) > +{ > + UINT32 NumberOfSets; > + NumberOfSets =3D *(UINT32*)Ptr; > + > + if (NumberOfSets =3D=3D 0) { > + IncrementErrorCount (); > + Print (L"\nERROR: Cache number of sets must be greater than 0"); > + return; > + } > + > +#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > + if (NumberOfSets > PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX) > { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: When ARMv8.3-CCIDX is implemented the maximum cache > number of " > + L"sets must be less than or equal to %d", > + PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX > + ); > + return; > + } > + > + if (NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) { > + IncrementWarningCount (); > + Print ( > + L"\nWARNING: Without ARMv8.3-CCIDX, the maximum cache number > of sets " > + L"must be less than or equal to %d. Ignore this message if " > + L"ARMv8.3-CCIDX is implemented", > + PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX > + ); > + return; > + } > +#endif > + > +} > + > +/** > + This function validates the Cache Type Structure (Type 1) 'Associativ= ity' > + field. > + > + @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 > +ValidateCacheAssociativity ( > + IN UINT8* Ptr, > + IN VOID* Context > + ) > +{ > + UINT8 Associativity; > + Associativity =3D *(UINT8*)Ptr; > + > + if (Associativity =3D=3D 0) { > + IncrementErrorCount (); > + Print (L"\nERROR: Cache associativity must be greater than 0"); > + return; > + } > +} I see you add the PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX and PPTT_ARM_CAC= HE_ASSOCIATIVITY_MAX but they are not include in ValidateCacheAssociativity= . Is this a missing? Thanks, Zhichao [Krzysztof] I added PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX and PPTT_ARM_C= ACHE_ASSOCIATIVITY_MAX to entirely cover Arm architecture. However, the Ass= ociativity field in PPTT Type 1 structure is only one byte long, therefore,= values of 2^10 or 2^21 will never be reached. These macros are not used as of now, but I think that this table field is = likely to get expanded in the future and PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY= _MAX and PPTT_ARM_CACHE_ASSOCIATIVITY_MAX will become valid checks. >=20 > /** > This function validates the Cache Type Structure (Type 1) Line size f= ield. > @@ -49,11 +121,14 @@ ValidateCacheLineSize ( > UINT16 LineSize; > LineSize =3D *(UINT16*)Ptr; >=20 > - if ((LineSize < 16) || (LineSize > 2048)) { > + if ((LineSize < PPTT_ARM_CACHE_LINE_SIZE_MIN) || > + (LineSize > PPTT_ARM_CACHE_LINE_SIZE_MAX)) { > IncrementErrorCount (); > Print ( > - L"\nERROR: The cache line size must be between 16 and 2048 bytes" > - L" on ARM Platforms." > + L"\nERROR: The cache line size must be between %d and %d bytes" > + L" on ARM Platforms.", > + PPTT_ARM_CACHE_LINE_SIZE_MIN, > + PPTT_ARM_CACHE_LINE_SIZE_MAX > ); > return; > } > @@ -96,6 +171,13 @@ ValidateCacheAttributes ( > } > } >=20 > +/** > + An ACPI_PARSER array describing the ACPI PPTT Table. > +**/ > +STATIC CONST ACPI_PARSER PpttParser[] =3D { > + PARSE_ACPI_HEADER (&AcpiHdrInfo) > +}; > + > /** > An ACPI_PARSER array describing the processor topology structure head= er. > **/ > @@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER=20 > CacheTypeStructureParser[] =3D { > {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL}, > {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL}, > {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL}, > - {L"Number of sets", 4, 16, L"%d", NULL, NULL, NULL, NULL}, > - {L"Associativity", 1, 20, L"%d", NULL, NULL, NULL, NULL}, > + {L"Number of sets", 4, 16, L"%d", NULL, NULL,=20 > + ValidateCacheNumberOfSets, NULL}, {L"Associativity", 1, 20, L"%d",=20 > + NULL, NULL, ValidateCacheAssociativity, NULL}, > {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, = NULL}, > {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize,=20 > NULL} }; diff --git=20 > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser. > h > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser. > h > new file mode 100644 > index > 0000000000000000000000000000000000000000..2a671203fb0035bbc407ff4bb0 > ca9960706fa588 > --- /dev/null > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars > +++ er.h > @@ -0,0 +1,38 @@ > +/** @file > + Header file for PPTT parser > + > + Copyright (c) 2019, ARM Limited. All rights reserved. > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Reference(s): > + - ARM Architecture Reference Manual ARMv8 (D.a) **/ > + > +#ifndef PPTT_PARSER_H_ > +#define PPTT_PARSER_H_ > + > +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > + > +/// Cache parameters allowed by the architecture with ///=20 > +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from=20 > +CCSIDR_EL1 when > +ID_AA64MMFR2_EL1.CCIDX=3D=3D0001 > +#define PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX (1 << 24) > +#define PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX (1 << 21) > + > +/// Cache parameters allowed by the architecture without ///=20 > +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from > +CCSIDR_EL1 when ID_AA64MMFR2_EL1.CCIDX=3D=3D0000 > +#define PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX (1 << 15) > +#define PPTT_ARM_CACHE_ASSOCIATIVITY_MAX (1 << 10) > + > +/// Common cache parameters > +/// Derived from CCSIDR_EL1 > +/// The LineSize is represented by bits 2:0 /// (Log2(Number of bytes= =20 > +in cache line)) - 4 is used to represent /// the LineSize bits. > +#define PPTT_ARM_CACHE_LINE_SIZE_MAX (1 << 11) > +#define PPTT_ARM_CACHE_LINE_SIZE_MIN (1 << 4) > + > +#endif // if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > + > +#endif // PPTT_PARSER_H_ > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >=20