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; Sun, 30 Jun 2019 20:49:31 -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 fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jun 2019 20:49:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,437,1557212400"; d="scan'208";a="186259344" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga004.fm.intel.com with ESMTP; 30 Jun 2019 20:49:31 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 30 Jun 2019 20:49:30 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 30 Jun 2019 20:49:30 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.134]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.110]) with mapi id 14.03.0439.000; Mon, 1 Jul 2019 11:49:27 +0800 From: "Gao, Zhichao" To: Krzysztof Koch , "devel@edk2.groups.io" CC: "Carsey, Jaben" , "Ni, Ray" , "Sami.Mujawar@arm.com" , "Matteo.Carlini@arm.com" , "nd@arm.com" Subject: Re: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation Thread-Topic: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation Thread-Index: AQHVLZvCEocWyh+U/0qUD8Fg2zK3GKa1I7cw Date: Mon, 1 Jul 2019 03:49:26 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B8003A3@SHSMSX101.ccr.corp.intel.com> References: <20190628102438.30544-1-krzysztof.koch@arm.com> <20190628102438.30544-2-krzysztof.koch@arm.com> In-Reply-To: <20190628102438.30544-2-krzysztof.koch@arm.com> 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 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 ; > 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 >=20 > Add Cache Structure (Type 1) 'Number of sets' and 'Associativity' > field validation in the acpiview Processor Properties Topology Table (PPT= T) > parser. >=20 > Replace literal values with precompiler macros for existing Cache Structu= re > 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 [Krzysztof= ] > - Add two new PPTT Type 1 structure validation functions [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 > @@ STATIC CONST UINT32* NumberOfPrivateResources; STATIC > 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 s= ets' > + field. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information e.g. thi= s > + 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) 'Associativi= ty' > + field. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information e.g. thi= s > + 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_CACH= E_ASSOCIATIVITY_MAX but they are not include in ValidateCacheAssociativity. Is this a missing? Thanks, Zhichao >=20 > /** > This function validates the Cache Type Structure (Type 1) Line size fi= eld. > @@ -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 heade= r. > **/ > @@ -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, N= ULL}, > {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