From: "Gao, Zhichao" <zhichao.gao@intel.com>
To: Krzysztof Koch <krzysztof.koch@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Carsey, Jaben" <jaben.carsey@intel.com>,
"Ni, Ray" <ray.ni@intel.com>,
"Sami.Mujawar@arm.com" <Sami.Mujawar@arm.com>,
"Matteo.Carlini@arm.com" <Matteo.Carlini@arm.com>,
"nd@arm.com" <nd@arm.com>
Subject: Re: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation
Date: Mon, 1 Jul 2019 03:49:26 +0000 [thread overview]
Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B8003A3@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20190628102438.30544-2-krzysztof.koch@arm.com>
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 <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; 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 <krzysztof.koch@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
>
> /**
> 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)'
>
next prev parent reply other threads:[~2019-07-01 3:49 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-28 10:24 [PATCH v1 0/4] Fix a number of small issues in acpiview Krzysztof Koch
2019-06-28 10:24 ` [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation Krzysztof Koch
2019-06-28 11:00 ` [edk2-devel] " Alexei Fedorov
2019-07-01 3:49 ` Gao, Zhichao [this message]
2019-07-01 7:28 ` Krzysztof Koch
2019-07-02 5:56 ` Gao, Zhichao
2019-06-28 10:24 ` [PATCH v1 2/4] ShellPkg: acpiview: Make DBG2 output consistent with other tables Krzysztof Koch
2019-06-28 10:59 ` [edk2-devel] " Alexei Fedorov
2019-06-28 11:02 ` Alexei Fedorov
2019-06-28 10:24 ` [PATCH v1 3/4] ShellPkg: acpiview: Remove redundant IORT node types enum Krzysztof Koch
2019-06-28 10:59 ` [edk2-devel] " Alexei Fedorov
2019-06-28 11:02 ` Alexei Fedorov
2019-06-28 10:24 ` [PATCH v1 4/4] ShellPkg: acpiview: Remove duplicate indentation in IORT parser Krzysztof Koch
2019-06-28 10:58 ` [edk2-devel] " Alexei Fedorov
2019-06-28 11:01 ` Alexei Fedorov
2019-06-28 11:03 ` [edk2-devel] [PATCH v1 0/4] Fix a number of small issues in acpiview Alexei Fedorov
2019-07-02 8:44 ` Sami Mujawar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3CE959C139B4C44DBEA1810E3AA6F9000B8003A3@SHSMSX101.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox