Hi Zhichao, Thank you for the review. You can see my responses in line with your comments marked with [Krzysztof]
Kind regards, Krzysztof -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via Groups.Io Sent: Monday, July 1, 2019 4:49 To: Krzysztof Koch <krzysztof.k...@arm.com>; devel@edk2.groups.io Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; Sami Mujawar <sami.muja...@arm.com>; Matteo Carlini <matteo.carl...@arm.com>; nd <n...@arm.com> 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.k...@arm.com] > Sent: Friday, June 28, 2019 6:25 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray > <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; > sami.muja...@arm.com; matteo.carl...@arm.com; n...@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 <krzysztof.k...@arm.com> > --- > > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d46 > 5cac779badc3c79982 > > Notes: > v1: > - Use macros to define constant values used for validation [Krzysztof] > - 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 <Library/PrintLib.h> > #include <Library/UefiLib.h> > #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 of 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[] = { > - PARSE_ACPI_HEADER (&AcpiHdrInfo) > -}; > +STATIC > +VOID > +EFIAPI > +ValidateCacheNumberOfSets ( > + IN UINT8* Ptr, > + IN VOID* Context > + ) > +{ > + UINT32 NumberOfSets; > + NumberOfSets = *(UINT32*)Ptr; > + > + if (NumberOfSets == 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) 'Associativity' > + 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 = *(UINT8*)Ptr; > + > + if (Associativity == 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_CACHE_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_CACHE_ASSOCIATIVITY_MAX to entirely cover Arm architecture. However, the Associativity 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. > > /** > This function validates the Cache Type Structure (Type 1) Line size field. > @@ -49,11 +121,14 @@ ValidateCacheLineSize ( > UINT16 LineSize; > LineSize = *(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 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 ( > } > } > > +/** > + An ACPI_PARSER array describing the ACPI PPTT Table. > +**/ > +STATIC CONST ACPI_PARSER PpttParser[] = { > + PARSE_ACPI_HEADER (&AcpiHdrInfo) > +}; > + > /** > An ACPI_PARSER array describing the processor topology structure header. > **/ > @@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER > CacheTypeStructureParser[] = { > {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==0001 > +#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==0000 > +#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)' > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43103): https://edk2.groups.io/g/devel/message/43103 Mute This Topic: https://groups.io/mt/32240395/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
