Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation
OK. Got it.
Series: Reviewed-by: Zhichao Gao
Thanks,
Zhichao
> -Original Message-
> From: Krzysztof Koch [mailto:krzysztof.k...@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 PPTT
> table field validation
>
> 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 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 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 ; Ni, Ray
> > ; Gao, Zhichao ;
> > 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
> > ---
> >
> > 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
> > #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 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 ()
> > -};
> > +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 (Number
Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation
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 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 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 ; Ni, Ray
> ; Gao, Zhichao ;
> 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
> ---
>
> 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
> #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 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 ()
> -};
> +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
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 ; Ni, Ray ;
> Gao, Zhichao ; 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
> ---
>
> 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
> #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 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 ()
> -};
> +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 <
Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation
Reviewed-by: Alexei Fedorov -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43011): https://edk2.groups.io/g/devel/message/43011 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] -=-=-=-=-=-=-=-=-=-=-=-
