public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser
@ 2020-08-25 11:06 Sami Mujawar
  2020-09-10 11:36 ` Sami Mujawar
  2020-09-17  2:17 ` [edk2-devel] " Gao, Zhichao
  0 siblings, 2 replies; 4+ messages in thread
From: Sami Mujawar @ 2020-08-25 11:06 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ray.ni, zhichao.gao, marc.moisson-franckhauser,
	Guillaume.Letellier, Matteo.Carlini, Ben.Adderson, nd

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.inf |   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..969cc0b371852f01f30c88dc506374a459c9c19e 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..f8e8b5979f3be041bbc8d17042b2db8e0b73f205 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..57b93cca6ba24ed77f9dcd7bf2f45ba9f0cb9f26
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.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_BANDWIDTH_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_ATTRIBUTES*
+  Attributes;
+
+  Attributes =
+    (EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES*)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_ATTRIBUTES*
+  Attributes;
+
+  Attributes =
+    (EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES*)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;
+    }
+  } // switch
+
+  if (IndexInitiator <= MAX_MEMORY_DOMAIN_TARGET_PRINT_MATRIX) {
+    // 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");
+    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)]
+          );
+      } // 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]
+      );
+  }
+}
+
+/**
+  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_INFO: {
+        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
+          );
+      }
+    }
+
+    HmatStructurePtr += *HmatStructureLength;
+    Offset += *HmatStructureLength;
+  } // while
+}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
index d2f26ff89f12e596702281c38ab0de3729aa68e4..c9e3054a5c413ba95e6420ce3d02f0d954e99d90 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
index 91459f9ec632635ee453c5ef46f67445cd9eee0c..e970c4259c143b5b06fb1a759d5819e7cb93e749 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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/UefiShellAcpiViewCommandLib.uni b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
index 7cd43d0518fd0a23dc547a5cab0d08b62602a113..6b0537b47a751184e8a68a6530aa85eb28ea33ba 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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)'


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser
  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 ` [edk2-devel] " Gao, Zhichao
  1 sibling, 0 replies; 4+ messages in thread
From: Sami Mujawar @ 2020-09-10 11:36 UTC (permalink / raw)
  To: zhichao.gao@intel.com, devel@edk2.groups.io
  Cc: ray.ni@intel.com, Marc Moisson-Franckhauser, Guillaume Letellier,
	Matteo Carlini, Ben Adderson, nd, Sami Mujawar

Hi Zhichao,

Would it be possible for you to provide feedback for this patch, please?

Regards,

Sami Mujawar

-----Original Message-----
From: Sami Mujawar <sami.mujawar@arm.com> 
Sent: 25 August 2020 12:07 PM
To: devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; ray.ni@intel.com; zhichao.gao@intel.com; Marc Moisson-Franckhauser <Marc.Moisson-Franckhauser@arm.com>; Guillaume Letellier <Guillaume.Letellier@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Ben Adderson <Ben.Adderson@arm.com>; nd <nd@arm.com>
Subject: [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.inf |   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..969cc0b371852f01f30c88dc506374a459c9c19e 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..f8e8b5979f3be041bbc8d17042b2db8e0b73f205 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..57b93cca6ba24ed77f9dcd7bf2f45ba9f0cb9f26
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.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_BANDWIDTH_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_ATTRIBUTES*
+  Attributes;
+
+  Attributes =
+    (EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES*)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_ATTRIBUTES*
+  Attributes;
+
+  Attributes =
+    (EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES*)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;
+    }
+  } // switch
+
+  if (IndexInitiator <= MAX_MEMORY_DOMAIN_TARGET_PRINT_MATRIX) {
+    // 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");
+    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)]
+          );
+      } // 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]
+      );
+  }
+}
+
+/**
+  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_INFO: {
+        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
+          );
+      }
+    }
+
+    HmatStructurePtr += *HmatStructureLength;
+    Offset += *HmatStructureLength;
+  } // while
+}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
index d2f26ff89f12e596702281c38ab0de3729aa68e4..c9e3054a5c413ba95e6420ce3d02f0d954e99d90 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
index 91459f9ec632635ee453c5ef46f67445cd9eee0c..e970c4259c143b5b06fb1a759d5819e7cb93e749 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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/UefiShellAcpiViewCommandLib.uni b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
index 7cd43d0518fd0a23dc547a5cab0d08b62602a113..6b0537b47a751184e8a68a6530aa85eb28ea33ba 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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)'


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser
  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
  2020-09-18  9:38   ` Sami Mujawar
  1 sibling, 1 reply; 4+ messages in thread
From: Gao, Zhichao @ 2020-09-17  2:17 UTC (permalink / raw)
  To: devel@edk2.groups.io, sami.mujawar@arm.com
  Cc: Ni, Ray, marc.moisson-franckhauser@arm.com,
	Guillaume.Letellier@arm.com, Matteo.Carlini@arm.com,
	Ben.Adderson@arm.com, nd@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)'
> 
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser
  2020-09-17  2:17 ` [edk2-devel] " Gao, Zhichao
@ 2020-09-18  9:38   ` Sami Mujawar
  0 siblings, 0 replies; 4+ messages in thread
From: Sami Mujawar @ 2020-09-18  9:38 UTC (permalink / raw)
  To: devel@edk2.groups.io, zhichao.gao@intel.com
  Cc: Ni, Ray, Marc Moisson-Franckhauser, Guillaume Letellier,
	Matteo Carlini, Ben Adderson, nd

Hi Zhichao,

Thank you for reviewing this patch.

I have incorporated the review comments and will send an updated patch.
The HMAT spec issue has also been raised with ASWG.

Regards,

Sami Mujawar

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via groups.io
Sent: 17 September 2020 03:18 AM
To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Ni, Ray <ray.ni@intel.com>; Marc Moisson-Franckhauser <Marc.Moisson-Franckhauser@arm.com>; Guillaume Letellier <Guillaume.Letellier@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Ben Adderson <Ben.Adderson@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1] ShellPkg/AcpiView: HMAT Parser

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)'
> 
> 
> 







^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-18  9:38 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 ` [edk2-devel] " Gao, Zhichao
2020-09-18  9:38   ` Sami Mujawar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox