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; Mon, 01 Jul 2019 22:56:12 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Jul 2019 22:56:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,442,1557212400"; d="scan'208";a="190514504" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 01 Jul 2019 22:56:12 -0700 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 1 Jul 2019 22:56:11 -0700 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 1 Jul 2019 22:56:11 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 1 Jul 2019 22:56:11 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.134]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.232]) with mapi id 14.03.0439.000; Tue, 2 Jul 2019 13:56:09 +0800 From: "Gao, Zhichao" To: Krzysztof Koch , "devel@edk2.groups.io" 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: AQHVLZvCEocWyh+U/0qUD8Fg2zK3GKa1I7cw//+4NYCAAf11IA== Date: Tue, 2 Jul 2019 05:56:09 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B803B4D@SHSMSX101.ccr.corp.intel.com> References: <20190628102438.30544-1-krzysztof.koch@arm.com> <20190628102438.30544-2-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B8003A3@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 OK. Got it. Series: Reviewed-by: Zhichao Gao Thanks, Zhichao > -----Original Message----- > From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com] > Sent: Monday, July 1, 2019 3:29 PM > To: devel@edk2.groups.io; Gao, Zhichao > Cc: nd > Subject: RE: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPT= T > table field validation >=20 > Hi Zhichao, >=20 > Thank you for the review. You can see my responses in line with your > comments marked with [Krzysztof] >=20 > Kind regards, >=20 > Krzysztof >=20 > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Gao, > Zhichao via Groups.Io > Sent: Monday, July 1, 2019 4:49 > To: Krzysztof Koch ; devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray ; > Sami Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPT= T > table field validation >=20 > Minor comment on ValidateCacheAssociativity: >=20 > > -----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 > > ; Gao, Zhichao ; > > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com > > Subject: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field > > validation > > > > Add Cache Structure (Type 1) 'Number of sets' and 'Associativity' > > field validation in the acpiview Processor Properties Topology Table > > (PPTT) parser. > > > > Replace literal values with precompiler macros for existing Cache > > Structure validation functions. > > > > Signed-off-by: Krzysztof Koch > > --- > > > > Changes can be seen at: > > > https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d46 > > 5cac779badc3c79982 > > > > Notes: > > v1: > > - Use macros to define constant values used for validation [Krzysz= tof] > > - Add two new PPTT Type 1 structure validation functions > > [Krzysztof] > > > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > > | 102 ++++++++++++++++++-- > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h > > | 38 ++++++++ > > 2 files changed, 130 insertions(+), 10 deletions(-) > > > > 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 > > > > @par Reference(s): > > - - ACPI 6.2 Specification - Errata A, September 2017 > > + - ACPI 6.3 Specification - January 2019 > > + - ARM Architecture Reference Manual ARMv8 (D.a) > > **/ > > > > #include > > #include > > #include "AcpiParser.h" > > +#include "AcpiView.h" > > +#include "PpttParser.h" > > > > // Local variables > > STATIC CONST UINT8* ProcessorTopologyStructureType; @@ -19,11 > +22,80 > > @@ STATIC CONST UINT32* NumberOfPrivateResources; STATIC > > ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; > > > > /** > > - An ACPI_PARSER array describing the ACPI PPTT Table. > > + This function validates the Cache Type Structure (Type 1) 'Number o= f > sets' > > + field. > > + > > + @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 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) 'Associat= ivity' > > + field. > > + > > + @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 > > +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; > > + } > > +} >=20 > I see you add the PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX and > PPTT_ARM_CACHE_ASSOCIATIVITY_MAX but they are not include in > ValidateCacheAssociativity. > Is this a missing? >=20 > Thanks, > Zhichao >=20 > [Krzysztof] I added PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX and > PPTT_ARM_CACHE_ASSOCIATIVITY_MAX to entirely cover Arm architecture. > However, the Associativity field in PPTT Type 1 structure is only one by= te long, > therefore, values of 2^10 or 2^21 will never be reached. >=20 > These macros are not used as of now, but I think that this table field i= s 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= field. > > @@ -49,11 +121,14 @@ ValidateCacheLineSize ( > > UINT16 LineSize; > > LineSize =3D *(UINT16*)Ptr; > > > > - 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 byte= s" > > - 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 ( > > } > > } > > > > +/** > > + 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 > header. > > **/ > > @@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER > > 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, > > + ValidateCacheNumberOfSets, NULL}, {L"Associativity", 1, 20, L"%d", > > + 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, > > NULL} }; diff --git > > > 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 /// > > +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from > > +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 /// > > +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 > > +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 >=20 >=20