From: "Gao, Zhichao" <zhichao.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"sami.mujawar@arm.com" <sami.mujawar@arm.com>
Cc: "Ni, Ray" <ray.ni@intel.com>,
"marc.moisson-franckhauser@arm.com"
<marc.moisson-franckhauser@arm.com>,
"Guillaume.Letellier@arm.com" <Guillaume.Letellier@arm.com>,
"Matteo.Carlini@arm.com" <Matteo.Carlini@arm.com>,
"Ben.Adderson@arm.com" <Ben.Adderson@arm.com>,
"nd@arm.com" <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser
Date: Thu, 17 Sep 2020 02:17:33 +0000 [thread overview]
Message-ID: <DM6PR11MB4425025FB59631A7987AAB30F63E0@DM6PR11MB4425.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200825110651.17652-1-sami.mujawar@arm.com>
Hi Sami,
Please see below comment.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami
> Mujawar
> Sent: Tuesday, August 25, 2020 7:07 PM
> To: devel@edk2.groups.io
> Cc: Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>; Gao,
> Zhichao <zhichao.gao@intel.com>; marc.moisson-franckhauser@arm.com;
> Guillaume.Letellier@arm.com; Matteo.Carlini@arm.com;
> Ben.Adderson@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser
>
> From: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
>
> Add a new parser for the Heterogeneous Memory Attribute Table. The parser
> also validates some fields for this table.
>
> The HMAT table is used to describe the memory attributes such as memory side
> cache attributes and bandwidth and latency details related to memory proximity
> domains. The info in the HMAT table can be used by an operating system for
> optimisation.
>
> Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-
> franckhauser@arm.com>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/833_hmat_parser_v2
>
> Notes:
> v2:
> - Fixed minor output formatting in DumpCacheAttributes() [SAMI]
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 28 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h | 4
> +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
> | 641 ++++++++++++++++++++
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> | 3 +-
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.i
> nf | 3 +-
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.
> uni | 3 +-
> 6 files changed, 676 insertions(+), 6 deletions(-)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index
> f81ccac7e118378aa185db4b625e5bcd75f78347..969cc0b371852f01f30c88dc506
> 374a459c9c19e 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -1,7 +1,7 @@
> /** @file
> Header file for ACPI parser
>
> - Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
> + Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent **/
>
> @@ -592,6 +592,32 @@ ParseAcpiGtdt (
> );
>
> /**
> + This function parses the ACPI HMAT table.
> + When trace is enabled this function parses the HMAT table and traces
> + the ACPI table fields.
> +
> + This function parses the following HMAT structures:
> + - Memory Proximity Domain Attributes Structure (Type 0)
> + - System Locality Latency and Bandwidth Info Structure (Type 1)
> + - Memory Side Cache Info structure (Type 2)
> +
> + This function also performs validation of the ACPI table fields.
> +
> + @param [in] Trace If TRUE, trace the ACPI fields.
> + @param [in] Ptr Pointer to the start of the buffer.
> + @param [in] AcpiTableLength Length of the ACPI table.
> + @param [in] AcpiTableRevision Revision of the ACPI table.
> +**/
> +VOID
> +EFIAPI
> +ParseAcpiHmat (
> + IN BOOLEAN Trace,
> + IN UINT8* Ptr,
> + IN UINT32 AcpiTableLength,
> + IN UINT8 AcpiTableRevision
> + );
> +
> +/**
> This function parses the ACPI IORT table.
> When trace is enabled this function parses the IORT table and
> traces the ACPI fields.
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
> index
> 4f92596b90a6ee422d8d0959881015ffd3de4da0..f8e8b5979f3be041bbc8d17042
> b2db8e0b73f205 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
> @@ -1,7 +1,7 @@
> /** @file
> Header file for ACPI table parser
>
> - Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> + Copyright (c) 2016 - 2018, Arm Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent **/
>
> @@ -11,7 +11,7 @@
> /**
> The maximum number of ACPI table parsers.
> */
> -#define MAX_ACPI_TABLE_PARSERS 16
> +#define MAX_ACPI_TABLE_PARSERS 32
>
> /** An invalid/NULL signature value.
> */
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..57b93cca6ba24ed77f9dcd7bf
> 2f45ba9f0cb9f26
> --- /dev/null
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatPars
> +++ er.c
> @@ -0,0 +1,641 @@
> +/** @file
> + HMAT table parser
> +
> + Copyright (c) 2020, Arm Limited.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> + @par Reference(s):
> + - ACPI 6.3 Specification - January 2019
> +
> + @par Glossary:
> + - MPDA - Memory Proximity Domain Attributes
> + - SLLBI - System Locality Latency and Bandwidth Information
> + - MSCI - Memory Side Cache Information
> + - Dom - Domain
> +**/
> +
> +#include <Library/PrintLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/UefiLib.h>
> +#include "AcpiParser.h"
> +#include "AcpiView.h"
> +
> +// Maximum Memory Domain matrix print size.
> +#define MAX_MEMORY_DOMAIN_TARGET_PRINT_MATRIX 12
> +
> +// Local variables
> +STATIC CONST UINT16* HmatStructureType; STATIC CONST UINT32*
> +HmatStructureLength;
> +
> +STATIC CONST UINT32* NumberInitiatorProximityDomain; STATIC CONST
> +UINT32* NumberTargetProximityDomain; STATIC CONST
> +EFI_ACPI_6_3_HMAT_STRUCTURE_SYSTEM_LOCALITY_LATENCY_AND_BAND
> WIDTH_INFO_
> +FLAGS*
> +SllbiFlags;
> +
> +STATIC CONST UINT8* SllbiDataType;
> +STATIC CONST UINT32* NumberSMBIOSHandles;
> +
> +STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> +
> +/**
> + Names of System Locality Latency Bandwidth Information (SLLBI) data
> +types **/ STATIC CONST CHAR16* SllbiNames[] = {
> + L"Access %sLatency%s",
> + L"Read %sLatency%s",
> + L"Write %sLatency%s",
> + L"Access %sBandwidth%s",
> + L"Read %sBandwidth%s",
> + L"Write %sBandwidth%s"
> +};
> +
> +/**
> + This function validates the Cache Attributes 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
> +ValidateCacheAttributes (
> + IN UINT8* Ptr,
> + IN VOID* Context
> + )
> +{
> +
> EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTR
> IBUTES*
> + Attributes;
> +
> + Attributes =
> +
> +
> (EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATT
> RIBUTES*)
> + Ptr;
> +
> + if (Attributes->TotalCacheLevels > 0x3) {
> + IncrementErrorCount ();
> + Print (
> + L"\nERROR: Attributes bits [3:0] have invalid value: 0x%x",
> + Attributes->TotalCacheLevels
> + );
> + }
> + if (Attributes->CacheLevel > 0x3) {
> + IncrementErrorCount ();
> + Print (
> + L"\nERROR: Attributes bits [7:4] have invalid value: 0x%x",
> + Attributes->CacheLevel
> + );
> + }
> + if (Attributes->CacheAssociativity > 0x2) {
> + IncrementErrorCount ();
> + Print (
> + L"\nERROR: Attributes bits [11:8] have invalid value: 0x%x",
> + Attributes->CacheAssociativity
> + );
> + }
> + if (Attributes->WritePolicy > 0x2) {
> + IncrementErrorCount ();
> + Print (
> + L"\nERROR: Attributes bits [15:12] have invalid value: 0x%x",
> + Attributes->WritePolicy
> + );
> + }
> +}
> +
> +/**
> + Dumps the cache attributes field
> +
> + @param [in] Format Optional format string for tracing the data.
> + @param [in] Ptr Pointer to the start of the buffer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DumpCacheAttributes (
> + IN CONST CHAR16* Format OPTIONAL,
> + IN UINT8* Ptr
> + )
> +{
> +
> EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTR
> IBUTES*
> + Attributes;
> +
> + Attributes =
> +
> +
> (EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATT
> RIBUTES*)
> + Ptr;
> +
> + Print (L"\n");
> + PrintFieldName (4, L"Total Cache Levels");
> + Print (L"%d\n", Attributes->TotalCacheLevels);
> + PrintFieldName (4, L"Cache Level");
> + Print (L"%d\n", Attributes->CacheLevel);
> + PrintFieldName (4, L"Cache Associativity");
> + Print (L"%d\n", Attributes->CacheAssociativity);
> + PrintFieldName (4, L"Write Policy");
> + Print (L"%d\n", Attributes->WritePolicy);
> + PrintFieldName (4, L"Cache Line Size");
> + Print (L"%d\n", Attributes->CacheLineSize); }
> +
> +/**
> + An ACPI_PARSER array describing the ACPI HMAT Table.
> +*/
> +STATIC CONST ACPI_PARSER HmatParser[] = {
> + PARSE_ACPI_HEADER (&AcpiHdrInfo),
> + {L"Reserved", 4, 36, NULL, NULL, NULL, NULL, NULL} };
> +
> +/**
> + An ACPI_PARSER array describing the HMAT structure header.
> +*/
> +STATIC CONST ACPI_PARSER HmatStructureHeaderParser[] = {
> + {L"Type", 2, 0, NULL, NULL, (VOID**)&HmatStructureType, NULL, NULL},
> + {L"Reserved", 2, 2, NULL, NULL, NULL, NULL, NULL},
> + {L"Length", 4, 4, NULL, NULL, (VOID**)&HmatStructureLength, NULL,
> +NULL} };
> +
> +/**
> + An ACPI PARSER array describing the Memory Proximity Domain
> +Attributes
> + Structure - Type 0.
> +*/
> +STATIC CONST ACPI_PARSER MemProximityDomainAttributeParser[] = {
> + {L"Type", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Length", 4, 4, L"%d", NULL, NULL, NULL, NULL},
> + {L"Flags", 2, 8, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Reserved", 2, 10, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Proximity Dom for initiator", 4, 12, L"0x%x", NULL, NULL, NULL,
> +NULL},
> + {L"Proximity Dom for memory", 4, 16, L"0x%x", NULL, NULL, NULL,
> +NULL},
> + {L"Reserved", 4, 20, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Reserved", 8, 24, L"0x%lx", NULL, NULL, NULL, NULL},
> + {L"Reserved", 8, 32, L"0x%lx", NULL, NULL, NULL, NULL} };
> +
> +/**
> + An ACPI PARSER array describing the System Locality Latency and
> +Bandwidth
> + Information Structure - Type 1.
> +*/
> +STATIC CONST ACPI_PARSER
> +SllbiParser[] = {
> + {L"Type", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Length", 4, 4, L"%d", NULL, NULL, NULL, NULL},
> + {L"Flags", 1, 8, L"0x%x", NULL, (VOID**)&SllbiFlags, NULL, NULL},
> + {L"Data type", 1, 9, L"0x%x", NULL, (VOID**)&SllbiDataType, NULL,
> +NULL},
> + {L"Reserved", 2, 10, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Initiator Proximity Dom Count", 4, 12, L"%d", NULL,
> + (VOID**)&NumberInitiatorProximityDomain, NULL, NULL},
> + {L"Target Proximity Dom Count", 4, 16, L"%d", NULL,
> + (VOID**)&NumberTargetProximityDomain, NULL, NULL},
> + {L"Reserved", 4, 20, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Entry Base Unit", 8, 24, L"0x%lx", NULL, NULL, NULL, NULL}
> + // initiator Proximity Domain list ...
> + // target Proximity Domain list ...
> + // Latency/Bandwidth matrix ...
> +};
> +
> +/**
> + An ACPI PARSER array describing the Memory Side Cache Information
> + Structure - Type 2.
> +*/
> +STATIC CONST ACPI_PARSER MemSideCacheInfoParser[] = {
> + {L"Type", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Length", 4, 4, L"%d", NULL, NULL, NULL, NULL},
> + {L"Proximity Dom for memory", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Reserved", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Memory Side Cache Size", 8, 16, L"0x%lx", NULL, NULL, NULL, NULL},
> + {L"Cache Attributes", 4, 24, NULL, DumpCacheAttributes, NULL,
> + ValidateCacheAttributes, NULL},
> + {L"Reserved", 2, 28, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"SMBIOS Handle Count", 2, 30, L"%d", NULL,
> + (VOID**)&NumberSMBIOSHandles, NULL, NULL}
> + // SMBIOS handles List ...
> +};
> +
> +/**
> + This function parses the Memory Proximity Domain Attributes
> + Structure (Type 0).
> +
> + @param [in] Ptr Pointer to the start of the Memory Proximity Domain
> + Attributes Structure data.
> + @param [in] Length Length of the Memory Proximity Domain Attributes
> + Structure.
> +**/
> +STATIC
> +VOID
> +DumpMpda (
> + IN UINT8* Ptr,
> + IN UINT8 Length
> + )
> +{
> + ParseAcpi (
> + TRUE,
> + 2,
> + "Memory Proximity Domain Attributes Structure",
> + Ptr,
> + Length,
> + PARSER_PARAMS (MemProximityDomainAttributeParser)
> + );
> +}
> +
> +/**
> + This function parses the System Locality Latency and Bandwidth
> +Information
> + Structure (Type 1).
> +
> + @param [in] Ptr Pointer to the start of the System Locality Latency and
> + Bandwidth Information Structure data.
> + @param [in] Length Length of the System Locality Latency and Bandwidth
> + Information Structure.
> +**/
> +STATIC
> +VOID
> +DumpSllbi (
> + IN UINT8* Ptr,
> + IN UINT8 Length
> + )
> +{
> + CONST UINT32* InitiatorProximityDomainList;
> + CONST UINT32* TargetProximityDomainList;
> + CONST UINT16* LatencyBandwidthMatrix;
> + UINT32 Offset;
> + CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
> + CHAR16 SecondBuffer[OUTPUT_FIELD_COLUMN_WIDTH];
> + UINT32 RequiredTableSize;
> + UINT32 Index;
> + UINT32 IndexInitiator;
> + UINT32 IndexTarget;
> +
> + Offset = ParseAcpi (
> + TRUE,
> + 2,
> + "System Locality Latency and Bandwidth Information Structure",
> + Ptr,
> + Length,
> + PARSER_PARAMS (SllbiParser)
> + );
> +
> + // Check if the values used to control the parsing logic have been
> + // successfully read.
> + if ((SllbiFlags == NULL) ||
> + (SllbiDataType == NULL) ||
> + (NumberInitiatorProximityDomain == NULL) ||
> + (NumberTargetProximityDomain == NULL)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient remaining table buffer length to read the " \
> + L"SLLBI structure header. Length = %d.\n",
> + Length
> + );
> + return;
> + }
> +
> + RequiredTableSize = (*NumberInitiatorProximityDomain * sizeof (UINT32)) +
> + (*NumberTargetProximityDomain * sizeof (UINT32)) +
> + (*NumberInitiatorProximityDomain *
> + *NumberTargetProximityDomain * sizeof (UINT16)) +
> + Offset;
> +
> + if (RequiredTableSize > Length) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient System Locality Latency and Bandwidth" \
> + L"Information Structure length. TableLength = %d. " \
> + L"RequiredTableLength = %d.\n",
> + Length,
> + RequiredTableSize
> + );
> + return;
> + }
> +
> + InitiatorProximityDomainList = (UINT32*) (Ptr + Offset);
> + TargetProximityDomainList = InitiatorProximityDomainList +
> + *NumberInitiatorProximityDomain;
> + LatencyBandwidthMatrix = (UINT16*) (TargetProximityDomainList +
> + *NumberTargetProximityDomain);
> +
> + // Display each element of the Initiator Proximity Domain list for
> + (Index = 0; Index < *NumberInitiatorProximityDomain; Index++) {
> + UnicodeSPrint (
> + Buffer,
> + sizeof (Buffer),
> + L"Initiator Proximity Dom [%d]",
> + Index
> + );
> +
> + PrintFieldName (4, Buffer);
> + Print (
> + L"0x%x\n",
> + InitiatorProximityDomainList[Index]
> + );
> + }
> +
> + // Display each element of the Target Proximity Domain list for
> + (Index = 0; Index < *NumberTargetProximityDomain; Index++) {
> + UnicodeSPrint (
> + Buffer,
> + sizeof (Buffer),
> + L"Target Proximity Dom [%d]",
> + Index
> + );
> +
> + PrintFieldName (4, Buffer);
> + Print (
> + L"0x%x\n",
> + TargetProximityDomainList[Index]
> + );
> + }
> +
> + // Create base name depending on Data Type in this Structure if
> + (*SllbiDataType >= ARRAY_SIZE (SllbiNames)) {
> + IncrementErrorCount ();
> + Print (L"Error: Unkown Data Type. DataType = 0x%x.\n", *SllbiDataType);
> + return;
> + }
> + StrCpyS (Buffer, sizeof (Buffer), SllbiNames[*SllbiDataType]);
> +
> + // Adjust base name depending on Memory Hierarchy in this Structure
> + switch (SllbiFlags->MemoryHierarchy) {
> + case 0: {
> + UnicodeSPrint (
> + SecondBuffer,
> + sizeof (SecondBuffer),
> + Buffer,
> + L"",
> + L"%s"
> + );
> + break;
> + }
> +
> + case 1:
> + case 2:
> + case 3: {
> + UnicodeSPrint (
> + SecondBuffer,
> + sizeof (SecondBuffer),
> + Buffer,
> + L"Hit ",
> + L"%s"
> + );
> + break;
> + }
> +
> + default: {
> + IncrementErrorCount ();
> + Print (
> + L"Error: Invalid Memory Hierarchy. MemoryHierarchy = %d.\n",
> + SllbiFlags->MemoryHierarchy
> + );
> + return;
> + }
I don't think switch case need the '{' and '}' to indicated the block section.
And refer to APCI spec 6.3 table 5-146. It says for memory hierarchy == 1, 2, 3 or 4, the data type is 'hit'. I am not sure if it is a spec mistake in 'Flags' or 'Data Type'.
> + } // switch
> +
> + if (IndexInitiator <= MAX_MEMORY_DOMAIN_TARGET_PRINT_MATRIX) {
The IndexInitiator is not initialized before use it. And it should not be the Initiator, it should be the target as the horizontal axis.
The IndexInitiator should be *NumberTargetProximityDomain. If I am wrong, please feel free to point out.
> + // Display the latency/bandwidth matrix as a matrix
> + UnicodeSPrint (
> + Buffer,
> + sizeof (Buffer),
> + SecondBuffer,
> + L""
> + );
> + PrintFieldName (4, Buffer);
> + Print (L"\n\n Initiator ->\nTarget\n|\nV\n");
As mentioned above, the Initiator and Target should be exchanged.
> + Print (L" |");
> +
> + for (IndexTarget = 0;
> + IndexTarget < *NumberTargetProximityDomain;
> + IndexTarget++) {
> + Print (L" %2d", IndexTarget);
> + }
> +
> + Print (L"\n---+");
> + for (IndexTarget = 0;
> + IndexTarget < *NumberTargetProximityDomain;
> + IndexTarget++) {
> + Print (L"-------");
> + }
> + Print (L"\n");
> +
> + for (IndexInitiator = 0;
> + IndexInitiator < *NumberInitiatorProximityDomain;
> + IndexInitiator++) {
> + Print (L"%2d |", IndexInitiator);
> + for (IndexTarget = 0;
> + IndexTarget < *NumberTargetProximityDomain;
> + IndexTarget++) {
> + Print (
> + L" %5d",
> + LatencyBandwidthMatrix[IndexInitiator *
> + (*NumberTargetProximityDomain + IndexTarget)]
Incorrect index value for the array. It should be "IndexInitiator * (*NumberTargetProximityDomain) + IndexTarget".
> + );
> + } // for Target
> + Print (L"\n");
> + } // for Initiator
> + Print (L"\n");
> + } else {
> + // Display the latency/bandwidth matrix as a list
> + UnicodeSPrint (
> + Buffer,
> + sizeof (Buffer),
> + SecondBuffer,
> + L" [%d][%d]"
> + );
> + for (IndexInitiator = 0;
> + IndexInitiator < *NumberInitiatorProximityDomain;
> + IndexInitiator++) {
> + for (IndexTarget = 0;
> + IndexTarget < *NumberTargetProximityDomain;
> + IndexTarget++) {
> + UnicodeSPrint (
> + Buffer,
> + sizeof (Buffer),
> + SecondBuffer,
> + IndexInitiator,
> + IndexTarget
> + );
> +
> + PrintFieldName (4, Buffer);
> + Print (
> + L"%d\n",
> + LatencyBandwidthMatrix[IndexInitiator *
> + (*NumberTargetProximityDomain + IndexTarget)]
> + );
> + } // for Target
> + } // for Initiator
> + }
> +}
> +
> +/**
> + This function parses the Memory Side Cache Information Structure (Type 2).
> +
> + @param [in] Ptr Pointer to the start of the Memory Side Cache Information
> + Structure data.
> + @param [in] Length Length of the Memory Side Cache Information Structure.
> +**/
> +STATIC
> +VOID
> +DumpMsci (
> + IN UINT8* Ptr,
> + IN UINT8 Length
> + )
> +{
> + CONST UINT16* SMBIOSHandlesList;
> + CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
> + UINT32 Offset;
> + UINT16 Index;
> +
> + Offset = ParseAcpi (
> + TRUE,
> + 2,
> + "Memory Side Cache Information Structure",
> + Ptr,
> + Length,
> + PARSER_PARAMS (MemSideCacheInfoParser)
> + );
> +
> + // Check if the values used to control the parsing logic have been
> + // successfully read.
> + if (NumberSMBIOSHandles == NULL) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient remaining table buffer length to read the " \
> + L"MSCI structure header. Length = %d.\n",
> + Length
> + );
> + return;
> + }
> +
> + if ((*NumberSMBIOSHandles * sizeof (UINT16)) > (Length - Offset)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Number of SMBIOS Handles. SMBIOSHandlesCount = %d."
> \
> + L"RemainingBufferLength = %d.\n",
> + *NumberSMBIOSHandles,
> + Length - Offset
> + );
> + return;
> + }
> +
> + SMBIOSHandlesList = (UINT16*) (Ptr + Offset);
> +
> + for (Index = 0; Index < *NumberSMBIOSHandles; Index++) {
> + UnicodeSPrint (
> + Buffer,
> + sizeof (Buffer),
> + L"SMBIOS Handles [%d]",
> + Index
> + );
> +
> + PrintFieldName (4, Buffer);
> + Print (
> + L"0x%x\n",
> + SMBIOSHandlesList[Index]
> + );
> + }
> +}
The three dump functions use UINT8 to transfer the length. It should be UINT32 refer to the caller and the usage in the function.
> +
> +/**
> + This function parses the ACPI HMAT table.
> + When trace is enabled this function parses the HMAT table and
> + traces the ACPI table fields.
> +
> + This function parses the following HMAT structures:
> + - Memory Proximity Domain Attributes Structure (Type 0)
> + - System Locality Latency and Bandwidth Info Structure (Type 1)
> + - Memory Side Cache Info structure (Type 2)
> +
> + This function also performs validation of the ACPI table fields.
> +
> + @param [in] Trace If TRUE, trace the ACPI fields.
> + @param [in] Ptr Pointer to the start of the buffer.
> + @param [in] AcpiTableLength Length of the ACPI table.
> + @param [in] AcpiTableRevision Revision of the ACPI table.
> +**/
> +VOID
> +EFIAPI
> +ParseAcpiHmat (
> + IN BOOLEAN Trace,
> + IN UINT8* Ptr,
> + IN UINT32 AcpiTableLength,
> + IN UINT8 AcpiTableRevision
> + )
> +{
> + UINT32 Offset;
> + UINT8* HmatStructurePtr;
> +
> + if (!Trace) {
> + return;
> + }
> +
> + Offset = ParseAcpi (
> + Trace,
> + 0,
> + "HMAT",
> + Ptr,
> + AcpiTableLength,
> + PARSER_PARAMS (HmatParser)
> + );
> +
> + HmatStructurePtr = Ptr + Offset;
> +
> + while (Offset < AcpiTableLength) {
> + // Parse HMAT Structure Header to obtain Type and Length.
> + ParseAcpi (
> + FALSE,
> + 0,
> + NULL,
> + HmatStructurePtr,
> + AcpiTableLength - Offset,
> + PARSER_PARAMS (HmatStructureHeaderParser)
> + );
> +
> + // Check if the values used to control the parsing logic have been
> + // successfully read.
> + if ((HmatStructureType == NULL) ||
> + (HmatStructureLength == NULL)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient remaining table buffer length to read the " \
> + L"HMAT structure header. Length = %d.\n",
> + AcpiTableLength - Offset
> + );
> + return;
> + }
> +
> + switch (*HmatStructureType) {
> + case
> EFI_ACPI_6_3_HMAT_TYPE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES: {
> + DumpMpda (
> + HmatStructurePtr,
> + *HmatStructureLength
> + );
> + break;
> + }
> +
> + case
> EFI_ACPI_6_3_HMAT_TYPE_SYSTEM_LOCALITY_LATENCY_AND_BANDWIDTH_I
> NFO: {
> + DumpSllbi (
> + HmatStructurePtr,
> + *HmatStructureLength
> + );
> + break;
> + }
> +
> + case EFI_ACPI_6_3_HMAT_TYPE_MEMORY_SIDE_CACHE_INFO: {
> + DumpMsci (
> + HmatStructurePtr,
> + *HmatStructureLength
> + );
> + break;
> + }
> +
> + default: {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Unknown HMAT structure:"
> + L" Type = %d, Length = %d\n",
> + *HmatStructureType,
> + *HmatStructureLength
> + );
> + }
Redundant '{' and '}' for switch case and missing break for 'default'.
Please test the patch before send to community.
Thanks,
Zhichao
> + }
> +
> + HmatStructurePtr += *HmatStructureLength;
> + Offset += *HmatStructureLength;
> + } // while
> +}
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.c
> index
> d2f26ff89f12e596702281c38ab0de3729aa68e4..c9e3054a5c413ba95e6420ce3d
> 02f0d954e99d90 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> +++ andLib.c
> @@ -1,7 +1,7 @@
> /** @file
> Main file for 'acpiview' Shell command function.
>
> - Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.<BR>
> + Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent **/
>
> @@ -53,6 +53,7 @@ ACPI_TABLE_PARSER ParserList[] = {
> {EFI_ACPI_6_3_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE,
> ParseAcpiFacs},
> {EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, ParseAcpiFadt},
> {EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
> ParseAcpiGtdt},
> + {EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_SIGNATURE,
> + ParseAcpiHmat},
> {EFI_ACPI_6_2_IO_REMAPPING_TABLE_SIGNATURE, ParseAcpiIort},
> {EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
> ParseAcpiMadt},
>
> {EFI_ACPI_6_2_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BA
> SE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.inf
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.inf
> index
> 91459f9ec632635ee453c5ef46f67445cd9eee0c..e970c4259c143b5b06fb1a759d5
> 819e7cb93e749 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.inf
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> +++ andLib.inf
> @@ -1,7 +1,7 @@
> ## @file
> # Provides Shell 'acpiview' command functions # -# Copyright (c) 2016 - 2020,
> ARM Limited. All rights reserved.<BR>
> +# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -33,6 +33,7 @@
> [Sources.common]
> Parsers/Facs/FacsParser.c
> Parsers/Fadt/FadtParser.c
> Parsers/Gtdt/GtdtParser.c
> + Parsers/Hmat/HmatParser.c
> Parsers/Iort/IortParser.c
> Parsers/Madt/MadtParser.c
> Parsers/Madt/MadtParser.h
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.uni
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.uni
> index
> 7cd43d0518fd0a23dc547a5cab0d08b62602a113..6b0537b47a751184e8a68a653
> 0aa85eb28ea33ba 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.uni
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> +++ andLib.uni
> @@ -1,6 +1,6 @@
> // /**
> //
> -// Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.<BR>
> +// Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
> // SPDX-License-Identifier: BSD-2-Clause-Patent // // Module Name:
> @@ -84,6 +84,7 @@
> " DSDT - Differentiated System Description Table\r\n"
> " FACP - Fixed ACPI Description Table (FADT)\r\n"
> " GTDT - Generic Timer Description Table\r\n"
> +" HMAT - Heterogeneous Memory Attributes Table\r\n"
> " IORT - IO Remapping Table\r\n"
> " MCFG - Memory Mapped Config Space Base Address Description
> Table\r\n"
> " PPTT - Processor Properties Topology Table\r\n"
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
next prev parent reply other threads:[~2020-09-17 2:17 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-25 11:06 [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser Sami Mujawar
2020-09-10 11:36 ` Sami Mujawar
2020-09-17 2:17 ` Gao, Zhichao [this message]
2020-09-18 9:38 ` [edk2-devel] " 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=DM6PR11MB4425025FB59631A7987AAB30F63E0@DM6PR11MB4425.namprd11.prod.outlook.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