[PATCH v1 0/4] Fix a number of small issues in acpiview

[PATCH v1 0/4] Fix a number of small issues in acpiview

[Krzysztof Koch]

The following patches introduce a number of unrelated changes which fix
a number of minor issues in the Acpiview UEFI shell tool.

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_fixes_v1

Krzysztof Koch (4):
  ShellPkg: acpiview: Improve PPTT table field validation
  ShellPkg: acpiview: Make DBG2 output consistent with other tables
  ShellPkg: acpiview: Remove redundant IORT node types enum
  ShellPkg: acpiview: Remove duplicate indentation in IORT parser

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |   3 +-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c |  48 ++++-----
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 102 ++++++++++++++++++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h |  38 ++++++++
 4 files changed, 149 insertions(+), 42 deletions(-)
 create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h

--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation

[Krzysztof Koch]

Add Cache Structure (Type 1) 'Number of sets' and 'Associativity'
field validation in the acpiview Processor Properties Topology
Table (PPTT) parser.

Replace literal values with precompiler macros for existing
Cache Structure validation functions.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d465cac779badc3c79982

Notes:
    v1:
    - Use macros to define constant values used for validation [Krzysztof]
    - Add two new PPTT Type 1 structure validation functions [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 102 ++++++++++++++++++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h |  38 ++++++++
 2 files changed, 130 insertions(+), 10 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index 71b6e7ae7c727ee0ea12f74e60c27c4c46e05872..cec57be55e77096f9448f637ea129af2b42111ad 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -5,12 +5,15 @@
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
-    - ACPI 6.2 Specification - Errata A, September 2017
+    - ACPI 6.3 Specification - January 2019
+    - ARM Architecture Reference Manual ARMv8 (D.a)
 **/
 
 #include <Library/PrintLib.h>
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
+#include "AcpiView.h"
+#include "PpttParser.h"
 
 // Local variables
 STATIC CONST UINT8*  ProcessorTopologyStructureType;
@@ -19,11 +22,80 @@ STATIC CONST UINT32* NumberOfPrivateResources;
 STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
 
 /**
-  An ACPI_PARSER array describing the ACPI PPTT Table.
+  This function validates the Cache Type Structure (Type 1) 'Number of sets'
+  field.
+
+  @param [in] Ptr       Pointer to the start of the field data.
+  @param [in] Context   Pointer to context specific information e.g. this
+                        could be a pointer to the ACPI table header.
 **/
-STATIC CONST ACPI_PARSER PpttParser[] = {
-  PARSE_ACPI_HEADER (&AcpiHdrInfo)
-};
+STATIC
+VOID
+EFIAPI
+ValidateCacheNumberOfSets (
+  IN UINT8* Ptr,
+  IN VOID*  Context
+  )
+{
+  UINT32 NumberOfSets;
+  NumberOfSets = *(UINT32*)Ptr;
+
+  if (NumberOfSets == 0) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: Cache number of sets must be greater than 0");
+    return;
+  }
+
+#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  if (NumberOfSets > PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: When ARMv8.3-CCIDX is implemented the maximum cache number of "
+        L"sets must be less than or equal to %d",
+      PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX
+      );
+    return;
+  }
+
+  if (NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) {
+    IncrementWarningCount ();
+    Print (
+      L"\nWARNING: Without ARMv8.3-CCIDX, the maximum cache number of sets "
+        L"must be less than or equal to %d. Ignore this message if "
+        L"ARMv8.3-CCIDX is implemented",
+      PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX
+      );
+    return;
+  }
+#endif
+
+}
+
+/**
+  This function validates the Cache Type Structure (Type 1) 'Associativity'
+  field.
+
+  @param [in] Ptr       Pointer to the start of the field data.
+  @param [in] Context   Pointer to context specific information e.g. this
+                        could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateCacheAssociativity (
+  IN UINT8* Ptr,
+  IN VOID*  Context
+  )
+{
+  UINT8 Associativity;
+  Associativity = *(UINT8*)Ptr;
+
+  if (Associativity == 0) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: Cache associativity must be greater than 0");
+    return;
+  }
+}
 
 /**
   This function validates the Cache Type Structure (Type 1) Line size field.
@@ -49,11 +121,14 @@ ValidateCacheLineSize (
   UINT16 LineSize;
   LineSize = *(UINT16*)Ptr;
 
-  if ((LineSize < 16) || (LineSize > 2048)) {
+  if ((LineSize < PPTT_ARM_CACHE_LINE_SIZE_MIN) ||
+      (LineSize > PPTT_ARM_CACHE_LINE_SIZE_MAX)) {
     IncrementErrorCount ();
     Print (
-      L"\nERROR: The cache line size must be between 16 and 2048 bytes"
-        L" on ARM Platforms."
+      L"\nERROR: The cache line size must be between %d and %d bytes"
+        L" on ARM Platforms.",
+      PPTT_ARM_CACHE_LINE_SIZE_MIN,
+      PPTT_ARM_CACHE_LINE_SIZE_MAX
       );
     return;
   }
@@ -96,6 +171,13 @@ ValidateCacheAttributes (
   }
 }
 
+/**
+  An ACPI_PARSER array describing the ACPI PPTT Table.
+**/
+STATIC CONST ACPI_PARSER PpttParser[] = {
+  PARSE_ACPI_HEADER (&AcpiHdrInfo)
+};
+
 /**
   An ACPI_PARSER array describing the processor topology structure header.
 **/
@@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER CacheTypeStructureParser[] = {
   {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
   {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
   {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
-  {L"Number of sets", 4, 16, L"%d", NULL, NULL, NULL, NULL},
-  {L"Associativity", 1, 20, L"%d", NULL, NULL, NULL, NULL},
+  {L"Number of sets", 4, 16, L"%d", NULL, NULL, ValidateCacheNumberOfSets, NULL},
+  {L"Associativity", 1, 20, L"%d", NULL, NULL, ValidateCacheAssociativity, NULL},
   {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, NULL},
   {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, NULL}
 };
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h
new file mode 100644
index 0000000000000000000000000000000000000000..2a671203fb0035bbc407ff4bb0ca9960706fa588
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h
@@ -0,0 +1,38 @@
+/** @file
+  Header file for PPTT parser
+
+  Copyright (c) 2019, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+    - ARM Architecture Reference Manual ARMv8 (D.a)
+**/
+
+#ifndef PPTT_PARSER_H_
+#define PPTT_PARSER_H_
+
+#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+
+/// Cache parameters allowed by the architecture with
+/// ARMv8.3-CCIDX (Cache extended number of sets)
+/// Derived from CCSIDR_EL1 when ID_AA64MMFR2_EL1.CCIDX==0001
+#define PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX       (1 << 24)
+#define PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX        (1 << 21)
+
+/// Cache parameters allowed by the architecture without
+/// ARMv8.3-CCIDX (Cache extended number of sets)
+/// Derived from CCSIDR_EL1 when ID_AA64MMFR2_EL1.CCIDX==0000
+#define PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX             (1 << 15)
+#define PPTT_ARM_CACHE_ASSOCIATIVITY_MAX              (1 << 10)
+
+/// Common cache parameters
+/// Derived from CCSIDR_EL1
+/// The LineSize is represented by bits 2:0
+/// (Log2(Number of bytes in cache line)) - 4 is used to represent
+/// the LineSize bits.
+#define PPTT_ARM_CACHE_LINE_SIZE_MAX                  (1 << 11)
+#define PPTT_ARM_CACHE_LINE_SIZE_MIN                  (1 << 4)
+
+#endif // if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+
+#endif // PPTT_PARSER_H_
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 2/4] ShellPkg: acpiview: Make DBG2 output consistent with other tables

[Krzysztof Koch]

Print an extra newline character at the end DBG2 table parsing in order
to make the output resemble the one for other ACPI table parsers.

With this change, there is now a blank line between the DBG2 table dump
and the 'Table Statistics' section.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/397c37eb5a73ee0f3bbd363075f97b19c0edaf2e

Notes:
    v1:
    - Print one more newline character after DBG2 table dump [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 310d3f18ec24532289c8f6a58cbd117fed0ca071..8de5ebf74775bab8e765849cba6ef4eb6f659a5a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -1,7 +1,7 @@
 /** @file
   DBG2 table parser
 
-  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
@@ -190,6 +190,7 @@ DumpDbgDeviceInfo (
       Print (L"\n%-*s   ", OUTPUT_FIELD_COLUMN_WIDTH, L"");
     }
   }
+  Print (L"\n");
 
   *Length = *DbgDevInfoLen;
 }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 3/4] ShellPkg: acpiview: Remove redundant IORT node types enum

[Krzysztof Koch]

Replace the enum defining valid node types in the IORT table with
macros from IoRemappingTable.h.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/79650cd6f2552e849afcd89aa016f35774376408

Notes:
    v1:
    - Remove redundant enum defining allowed IORT node types [Krzysztof]
    - Use macros from IoRemappingTable.h instead [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 30 ++++++--------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index e3d9bc9a996001f77ed8b13a1c57505496807f4c..a91a4f9db13a52285bf56abe33f359a771fc04bd 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -1,7 +1,7 @@
 /** @file
   IORT table parser
 
-  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
@@ -17,24 +17,10 @@
 // Local variables
 STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
 
-/**
-  The EIORT_NODE enum describes the IORT Node types.
-**/
-typedef enum IortNode {
-  Iort_Node_ITS_Group,        ///< ITS Group node
-  Iort_Node_Named_Component,  ///< Named Component node
-  Iort_Node_Root_Complex,     ///< Root Complex node
-  Iort_Node_SMMUV1_V2,        ///< SMMU v1/v2 node
-  Iort_Node_SMMUV3,           ///< SMMU v3 node
-  Iort_Node_PMCG,             ///< PMC group node
-  Iort_Node_Max
-} EIORT_NODE;
-
-// Local Variables
 STATIC CONST UINT32* IortNodeCount;
 STATIC CONST UINT32* IortNodeOffset;
 
-STATIC CONST UINT8* IortNodeType;
+STATIC CONST UINT8*  IortNodeType;
 STATIC CONST UINT16* IortNodeLength;
 STATIC CONST UINT32* IortIdMappingCount;
 STATIC CONST UINT32* IortIdMappingOffset;
@@ -659,13 +645,13 @@ ParseAcpiIort (
     Print (L"0x%x\n", Offset);
 
     switch (*IortNodeType) {
-      case Iort_Node_ITS_Group:
+      case EFI_ACPI_IORT_TYPE_ITS_GROUP:
         DumpIortNodeIts (
           NodePtr,
           *IortNodeLength
           );
         break;
-      case Iort_Node_Named_Component:
+      case EFI_ACPI_IORT_TYPE_NAMED_COMP:
         DumpIortNodeNamedComponent (
           NodePtr,
           *IortNodeLength,
@@ -673,7 +659,7 @@ ParseAcpiIort (
           *IortIdMappingOffset
           );
         break;
-      case Iort_Node_Root_Complex:
+      case EFI_ACPI_IORT_TYPE_ROOT_COMPLEX:
         DumpIortNodeRootComplex (
           NodePtr,
           *IortNodeLength,
@@ -681,7 +667,7 @@ ParseAcpiIort (
           *IortIdMappingOffset
           );
         break;
-      case Iort_Node_SMMUV1_V2:
+      case EFI_ACPI_IORT_TYPE_SMMUv1v2:
         DumpIortNodeSmmuV1V2 (
           NodePtr,
           *IortNodeLength,
@@ -689,7 +675,7 @@ ParseAcpiIort (
           *IortIdMappingOffset
           );
         break;
-      case Iort_Node_SMMUV3:
+      case EFI_ACPI_IORT_TYPE_SMMUv3:
         DumpIortNodeSmmuV3 (
           NodePtr,
           *IortNodeLength,
@@ -697,7 +683,7 @@ ParseAcpiIort (
           *IortIdMappingOffset
           );
         break;
-      case Iort_Node_PMCG:
+      case EFI_ACPI_IORT_TYPE_PMCG:
         DumpIortNodePmcg (
           NodePtr,
           *IortNodeLength,
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 4/4] ShellPkg: acpiview: Remove duplicate indentation in IORT parser

[Krzysztof Koch]

Remove redundant whitespace characters at the beginning of the strings
describing IORT table field names.

When dumping ACPI table contents, the indentation level for printing
field names is controled using the 'Indent' argument to the 'ParseAcpi'
function. In the IORT acpiview parser, both 'Indent' and extra
whitespace characters are used for indentation, which results in
excess indentation.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/b44e007195f32246f33000934a6178d36f79e4b1

Notes:
    v1:
    - fix indentation issues in the IORT table parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index a91a4f9db13a52285bf56abe33f359a771fc04bd..93f78e1a9786ed53f6b5529f478b72a220b4f8df 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -131,19 +131,19 @@ STATIC CONST ACPI_PARSER IortNodeSmmuV1V2Parser[] = {
   An ACPI_PARSER array describing the SMMUv1/2 Node Interrupt Array.
 **/
 STATIC CONST ACPI_PARSER InterruptArrayParser[] = {
-  {L"  Interrupt GSIV", 4, 0, L"0x%x", NULL, NULL, NULL, NULL},
-  {L"  Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL}
+  {L"Interrupt GSIV", 4, 0, L"0x%x", NULL, NULL, NULL, NULL},
+  {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL}
 };
 
 /**
   An ACPI_PARSER array describing the IORT ID Mapping.
 **/
 STATIC CONST ACPI_PARSER IortNodeIdMappingParser[] = {
-  {L"  Input base", 4, 0, L"0x%x", NULL, NULL, NULL, NULL},
-  {L"  Number of IDs", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
-  {L"  Output base", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
-  {L"  Output reference", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
-  {L"  Flags", 4, 16, L"0x%x", NULL, NULL, NULL, NULL}
+  {L"Input base", 4, 0, L"0x%x", NULL, NULL, NULL, NULL},
+  {L"Number of IDs", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
+  {L"Output base", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
+  {L"Output reference", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
+  {L"Flags", 4, 16, L"0x%x", NULL, NULL, NULL, NULL}
 };
 
 /**
@@ -170,14 +170,14 @@ STATIC CONST ACPI_PARSER IortNodeItsParser[] = {
     ValidateItsIdMappingCount,
     ValidateItsIdArrayReference
     ),
-  {L"  Number of ITSs", 4, 16, L"%d", NULL, (VOID**)&ItsCount, NULL}
+  {L"Number of ITSs", 4, 16, L"%d", NULL, (VOID**)&ItsCount, NULL}
 };
 
 /**
   An ACPI_PARSER array describing the ITS ID.
 **/
 STATIC CONST ACPI_PARSER ItsIdParser[] = {
-  { L"  GIC ITS Identifier", 4, 0, L"%d", NULL, NULL, NULL }
+  { L"GIC ITS Identifier", 4, 0, L"%d", NULL, NULL, NULL }
 };
 
 /**
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 4/4] ShellPkg: acpiview: Remove duplicate indentation in IORT parser

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 53 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@am.com>

[-- Attachment #2: Type: text/html, Size: 59 bytes --]

Re: [edk2-devel] [PATCH v1 3/4] ShellPkg: acpiview: Remove redundant IORT node types enum

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 53 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@am.com>

[-- Attachment #2: Type: text/html, Size: 59 bytes --]

Re: [edk2-devel] [PATCH v1 2/4] ShellPkg: acpiview: Make DBG2 output consistent with other tables

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 53 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@am.com>

[-- Attachment #2: Type: text/html, Size: 59 bytes --]

Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [edk2-devel] [PATCH v1 4/4] ShellPkg: acpiview: Remove duplicate indentation in IORT parser

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [edk2-devel] [PATCH v1 3/4] ShellPkg: acpiview: Remove redundant IORT node types enum

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [edk2-devel] [PATCH v1 2/4] ShellPkg: acpiview: Make DBG2 output consistent with other tables

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Fix a number of small issues in acpiview

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation

[Gao, Zhichao]

Minor comment on ValidateCacheAssociativity:

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, June 28, 2019 6:25 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Sami.Mujawar@arm.com;
> Matteo.Carlini@arm.com; nd@arm.com
> Subject: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field
> validation
> 
> Add Cache Structure (Type 1) 'Number of sets' and 'Associativity'
> field validation in the acpiview Processor Properties Topology Table (PPTT)
> parser.
> 
> Replace literal values with precompiler macros for existing Cache Structure
> validation functions.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d46
> 5cac779badc3c79982
> 
> Notes:
>     v1:
>     - Use macros to define constant values used for validation [Krzysztof]
>     - Add two new PPTT Type 1 structure validation functions [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> | 102 ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h
> |  38 ++++++++
>  2 files changed, 130 insertions(+), 10 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> index
> 71b6e7ae7c727ee0ea12f74e60c27c4c46e05872..cec57be55e77096f9448f637e
> a129af2b42111ad 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> +++ er.c
> @@ -5,12 +5,15 @@
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>    @par Reference(s):
> -    - ACPI 6.2 Specification - Errata A, September 2017
> +    - ACPI 6.3 Specification - January 2019
> +    - ARM Architecture Reference Manual ARMv8 (D.a)
>  **/
> 
>  #include <Library/PrintLib.h>
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
> +#include "AcpiView.h"
> +#include "PpttParser.h"
> 
>  // Local variables
>  STATIC CONST UINT8*  ProcessorTopologyStructureType; @@ -19,11 +22,80
> @@ STATIC CONST UINT32* NumberOfPrivateResources;  STATIC
> ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> 
>  /**
> -  An ACPI_PARSER array describing the ACPI PPTT Table.
> +  This function validates the Cache Type Structure (Type 1) 'Number of sets'
> +  field.
> +
> +  @param [in] Ptr       Pointer to the start of the field data.
> +  @param [in] Context   Pointer to context specific information e.g. this
> +                        could be a pointer to the ACPI table header.
>  **/
> -STATIC CONST ACPI_PARSER PpttParser[] = {
> -  PARSE_ACPI_HEADER (&AcpiHdrInfo)
> -};
> +STATIC
> +VOID
> +EFIAPI
> +ValidateCacheNumberOfSets (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT32 NumberOfSets;
> +  NumberOfSets = *(UINT32*)Ptr;
> +
> +  if (NumberOfSets == 0) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Cache number of sets must be greater than 0");
> +    return;
> +  }
> +
> +#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (NumberOfSets > PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX)
> {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: When ARMv8.3-CCIDX is implemented the maximum cache
> number of "
> +        L"sets must be less than or equal to %d",
> +      PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX
> +      );
> +    return;
> +  }
> +
> +  if (NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) {
> +    IncrementWarningCount ();
> +    Print (
> +      L"\nWARNING: Without ARMv8.3-CCIDX, the maximum cache number
> of sets "
> +        L"must be less than or equal to %d. Ignore this message if "
> +        L"ARMv8.3-CCIDX is implemented",
> +      PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX
> +      );
> +    return;
> +  }
> +#endif
> +
> +}
> +
> +/**
> +  This function validates the Cache Type Structure (Type 1) 'Associativity'
> +  field.
> +
> +  @param [in] Ptr       Pointer to the start of the field data.
> +  @param [in] Context   Pointer to context specific information e.g. this
> +                        could be a pointer to the ACPI table header.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ValidateCacheAssociativity (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT8 Associativity;
> +  Associativity = *(UINT8*)Ptr;
> +
> +  if (Associativity == 0) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Cache associativity must be greater than 0");
> +    return;
> +  }
> +}

I see you add the PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and PPTT_ARM_CACHE_ASSOCIATIVITY_MAX but they are not include in ValidateCacheAssociativity.
Is this a missing?

Thanks,
Zhichao

> 
>  /**
>    This function validates the Cache Type Structure (Type 1) Line size field.
> @@ -49,11 +121,14 @@ ValidateCacheLineSize (
>    UINT16 LineSize;
>    LineSize = *(UINT16*)Ptr;
> 
> -  if ((LineSize < 16) || (LineSize > 2048)) {
> +  if ((LineSize < PPTT_ARM_CACHE_LINE_SIZE_MIN) ||
> +      (LineSize > PPTT_ARM_CACHE_LINE_SIZE_MAX)) {
>      IncrementErrorCount ();
>      Print (
> -      L"\nERROR: The cache line size must be between 16 and 2048 bytes"
> -        L" on ARM Platforms."
> +      L"\nERROR: The cache line size must be between %d and %d bytes"
> +        L" on ARM Platforms.",
> +      PPTT_ARM_CACHE_LINE_SIZE_MIN,
> +      PPTT_ARM_CACHE_LINE_SIZE_MAX
>        );
>      return;
>    }
> @@ -96,6 +171,13 @@ ValidateCacheAttributes (
>    }
>  }
> 
> +/**
> +  An ACPI_PARSER array describing the ACPI PPTT Table.
> +**/
> +STATIC CONST ACPI_PARSER PpttParser[] = {
> +  PARSE_ACPI_HEADER (&AcpiHdrInfo)
> +};
> +
>  /**
>    An ACPI_PARSER array describing the processor topology structure header.
>  **/
> @@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER
> CacheTypeStructureParser[] = {
>    {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
>    {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
>    {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
> -  {L"Number of sets", 4, 16, L"%d", NULL, NULL, NULL, NULL},
> -  {L"Associativity", 1, 20, L"%d", NULL, NULL, NULL, NULL},
> +  {L"Number of sets", 4, 16, L"%d", NULL, NULL,
> + ValidateCacheNumberOfSets, NULL},  {L"Associativity", 1, 20, L"%d",
> + NULL, NULL, ValidateCacheAssociativity, NULL},
>    {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, NULL},
>    {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, NULL}  }; diff
> --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..2a671203fb0035bbc407ff4bb0
> ca9960706fa588
> --- /dev/null
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> +++ er.h
> @@ -0,0 +1,38 @@
> +/** @file
> +  Header file for PPTT parser
> +
> +  Copyright (c) 2019, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Reference(s):
> +    - ARM Architecture Reference Manual ARMv8 (D.a) **/
> +
> +#ifndef PPTT_PARSER_H_
> +#define PPTT_PARSER_H_
> +
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +
> +/// Cache parameters allowed by the architecture with /// ARMv8.3-CCIDX
> +(Cache extended number of sets) /// Derived from CCSIDR_EL1 when
> +ID_AA64MMFR2_EL1.CCIDX==0001
> +#define PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX       (1 << 24)
> +#define PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX        (1 << 21)
> +
> +/// Cache parameters allowed by the architecture without ///
> +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from
> +CCSIDR_EL1 when ID_AA64MMFR2_EL1.CCIDX==0000
> +#define PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX             (1 << 15)
> +#define PPTT_ARM_CACHE_ASSOCIATIVITY_MAX              (1 << 10)
> +
> +/// Common cache parameters
> +/// Derived from CCSIDR_EL1
> +/// The LineSize is represented by bits 2:0 /// (Log2(Number of bytes
> +in cache line)) - 4 is used to represent /// the LineSize bits.
> +#define PPTT_ARM_CACHE_LINE_SIZE_MAX                  (1 << 11)
> +#define PPTT_ARM_CACHE_LINE_SIZE_MIN                  (1 << 4)
> +
> +#endif // if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +
> +#endif // PPTT_PARSER_H_
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation

[Krzysztof Koch]

Hi Zhichao,

Thank you for the review. You can see my responses in line with your comments marked with [Krzysztof]

Kind regards,

Krzysztof

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via Groups.Io
Sent: Monday, July 1, 2019 4:49
To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation

Minor comment on ValidateCacheAssociativity:

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, June 28, 2019 6:25 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; 
> Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> Subject: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field 
> validation
> 
> Add Cache Structure (Type 1) 'Number of sets' and 'Associativity'
> field validation in the acpiview Processor Properties Topology Table 
> (PPTT) parser.
> 
> Replace literal values with precompiler macros for existing Cache 
> Structure validation functions.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d46
> 5cac779badc3c79982
> 
> Notes:
>     v1:
>     - Use macros to define constant values used for validation [Krzysztof]
>     - Add two new PPTT Type 1 structure validation functions 
> [Krzysztof]
> 
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> | 102 ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h
> |  38 ++++++++
>  2 files changed, 130 insertions(+), 10 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> index
> 71b6e7ae7c727ee0ea12f74e60c27c4c46e05872..cec57be55e77096f9448f637e
> a129af2b42111ad 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> +++ er.c
> @@ -5,12 +5,15 @@
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>    @par Reference(s):
> -    - ACPI 6.2 Specification - Errata A, September 2017
> +    - ACPI 6.3 Specification - January 2019
> +    - ARM Architecture Reference Manual ARMv8 (D.a)
>  **/
> 
>  #include <Library/PrintLib.h>
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
> +#include "AcpiView.h"
> +#include "PpttParser.h"
> 
>  // Local variables
>  STATIC CONST UINT8*  ProcessorTopologyStructureType; @@ -19,11 +22,80 
> @@ STATIC CONST UINT32* NumberOfPrivateResources;  STATIC 
> ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> 
>  /**
> -  An ACPI_PARSER array describing the ACPI PPTT Table.
> +  This function validates the Cache Type Structure (Type 1) 'Number of sets'
> +  field.
> +
> +  @param [in] Ptr       Pointer to the start of the field data.
> +  @param [in] Context   Pointer to context specific information e.g. this
> +                        could be a pointer to the ACPI table header.
>  **/
> -STATIC CONST ACPI_PARSER PpttParser[] = {
> -  PARSE_ACPI_HEADER (&AcpiHdrInfo)
> -};
> +STATIC
> +VOID
> +EFIAPI
> +ValidateCacheNumberOfSets (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT32 NumberOfSets;
> +  NumberOfSets = *(UINT32*)Ptr;
> +
> +  if (NumberOfSets == 0) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Cache number of sets must be greater than 0");
> +    return;
> +  }
> +
> +#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (NumberOfSets > PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX)
> {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: When ARMv8.3-CCIDX is implemented the maximum cache
> number of "
> +        L"sets must be less than or equal to %d",
> +      PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX
> +      );
> +    return;
> +  }
> +
> +  if (NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) {
> +    IncrementWarningCount ();
> +    Print (
> +      L"\nWARNING: Without ARMv8.3-CCIDX, the maximum cache number
> of sets "
> +        L"must be less than or equal to %d. Ignore this message if "
> +        L"ARMv8.3-CCIDX is implemented",
> +      PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX
> +      );
> +    return;
> +  }
> +#endif
> +
> +}
> +
> +/**
> +  This function validates the Cache Type Structure (Type 1) 'Associativity'
> +  field.
> +
> +  @param [in] Ptr       Pointer to the start of the field data.
> +  @param [in] Context   Pointer to context specific information e.g. this
> +                        could be a pointer to the ACPI table header.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ValidateCacheAssociativity (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT8 Associativity;
> +  Associativity = *(UINT8*)Ptr;
> +
> +  if (Associativity == 0) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Cache associativity must be greater than 0");
> +    return;
> +  }
> +}

I see you add the PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and PPTT_ARM_CACHE_ASSOCIATIVITY_MAX but they are not include in ValidateCacheAssociativity.
Is this a missing?

Thanks,
Zhichao

[Krzysztof] I added PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and PPTT_ARM_CACHE_ASSOCIATIVITY_MAX to entirely cover Arm architecture. However, the Associativity field in PPTT Type 1 structure is only one byte long, therefore, values of 2^10 or 2^21 will never be reached.

These macros are not used as of now, but I think that this table field is likely to get expanded in the future and PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and PPTT_ARM_CACHE_ASSOCIATIVITY_MAX will become valid checks.

> 
>  /**
>    This function validates the Cache Type Structure (Type 1) Line size field.
> @@ -49,11 +121,14 @@ ValidateCacheLineSize (
>    UINT16 LineSize;
>    LineSize = *(UINT16*)Ptr;
> 
> -  if ((LineSize < 16) || (LineSize > 2048)) {
> +  if ((LineSize < PPTT_ARM_CACHE_LINE_SIZE_MIN) ||
> +      (LineSize > PPTT_ARM_CACHE_LINE_SIZE_MAX)) {
>      IncrementErrorCount ();
>      Print (
> -      L"\nERROR: The cache line size must be between 16 and 2048 bytes"
> -        L" on ARM Platforms."
> +      L"\nERROR: The cache line size must be between %d and %d bytes"
> +        L" on ARM Platforms.",
> +      PPTT_ARM_CACHE_LINE_SIZE_MIN,
> +      PPTT_ARM_CACHE_LINE_SIZE_MAX
>        );
>      return;
>    }
> @@ -96,6 +171,13 @@ ValidateCacheAttributes (
>    }
>  }
> 
> +/**
> +  An ACPI_PARSER array describing the ACPI PPTT Table.
> +**/
> +STATIC CONST ACPI_PARSER PpttParser[] = {
> +  PARSE_ACPI_HEADER (&AcpiHdrInfo)
> +};
> +
>  /**
>    An ACPI_PARSER array describing the processor topology structure header.
>  **/
> @@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER 
> CacheTypeStructureParser[] = {
>    {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
>    {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
>    {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
> -  {L"Number of sets", 4, 16, L"%d", NULL, NULL, NULL, NULL},
> -  {L"Associativity", 1, 20, L"%d", NULL, NULL, NULL, NULL},
> +  {L"Number of sets", 4, 16, L"%d", NULL, NULL, 
> + ValidateCacheNumberOfSets, NULL},  {L"Associativity", 1, 20, L"%d", 
> + NULL, NULL, ValidateCacheAssociativity, NULL},
>    {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, NULL},
>    {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, 
> NULL}  }; diff --git 
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..2a671203fb0035bbc407ff4bb0
> ca9960706fa588
> --- /dev/null
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> +++ er.h
> @@ -0,0 +1,38 @@
> +/** @file
> +  Header file for PPTT parser
> +
> +  Copyright (c) 2019, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Reference(s):
> +    - ARM Architecture Reference Manual ARMv8 (D.a) **/
> +
> +#ifndef PPTT_PARSER_H_
> +#define PPTT_PARSER_H_
> +
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +
> +/// Cache parameters allowed by the architecture with /// 
> +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from 
> +CCSIDR_EL1 when
> +ID_AA64MMFR2_EL1.CCIDX==0001
> +#define PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX       (1 << 24)
> +#define PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX        (1 << 21)
> +
> +/// Cache parameters allowed by the architecture without /// 
> +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from
> +CCSIDR_EL1 when ID_AA64MMFR2_EL1.CCIDX==0000
> +#define PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX             (1 << 15)
> +#define PPTT_ARM_CACHE_ASSOCIATIVITY_MAX              (1 << 10)
> +
> +/// Common cache parameters
> +/// Derived from CCSIDR_EL1
> +/// The LineSize is represented by bits 2:0 /// (Log2(Number of bytes 
> +in cache line)) - 4 is used to represent /// the LineSize bits.
> +#define PPTT_ARM_CACHE_LINE_SIZE_MAX                  (1 << 11)
> +#define PPTT_ARM_CACHE_LINE_SIZE_MIN                  (1 << 4)
> +
> +#endif // if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +
> +#endif // PPTT_PARSER_H_
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field validation

[Gao, Zhichao]

OK. Got it.
Series: Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com]
> Sent: Monday, July 1, 2019 3:29 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: nd <nd@arm.com>
> Subject: RE: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT
> table field validation
> 
> Hi Zhichao,
> 
> Thank you for the review. You can see my responses in line with your
> comments marked with [Krzysztof]
> 
> Kind regards,
> 
> Krzysztof
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao via Groups.Io
> Sent: Monday, July 1, 2019 4:49
> To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT
> table field validation
> 
> Minor comment on ValidateCacheAssociativity:
> 
> > -----Original Message-----
> > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> > Sent: Friday, June 28, 2019 6:25 PM
> > To: devel@edk2.groups.io
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> > Subject: [PATCH v1 1/4] ShellPkg: acpiview: Improve PPTT table field
> > validation
> >
> > Add Cache Structure (Type 1) 'Number of sets' and 'Associativity'
> > field validation in the acpiview Processor Properties Topology Table
> > (PPTT) parser.
> >
> > Replace literal values with precompiler macros for existing Cache
> > Structure validation functions.
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Changes can be seen at:
> >
> https://github.com/KrzysztofKoch1/edk2/commit/014f98b8f1ba29607d8d46
> > 5cac779badc3c79982
> >
> > Notes:
> >     v1:
> >     - Use macros to define constant values used for validation [Krzysztof]
> >     - Add two new PPTT Type 1 structure validation functions
> > [Krzysztof]
> >
> >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> > | 102 ++++++++++++++++++--
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h
> > |  38 ++++++++
> >  2 files changed, 130 insertions(+), 10 deletions(-)
> >
> > diff --git
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> > c
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> > c
> > index
> >
> 71b6e7ae7c727ee0ea12f74e60c27c4c46e05872..cec57be55e77096f9448f637e
> > a129af2b42111ad 100644
> > ---
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> > c
> > +++
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> > +++ er.c
> > @@ -5,12 +5,15 @@
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >    @par Reference(s):
> > -    - ACPI 6.2 Specification - Errata A, September 2017
> > +    - ACPI 6.3 Specification - January 2019
> > +    - ARM Architecture Reference Manual ARMv8 (D.a)
> >  **/
> >
> >  #include <Library/PrintLib.h>
> >  #include <Library/UefiLib.h>
> >  #include "AcpiParser.h"
> > +#include "AcpiView.h"
> > +#include "PpttParser.h"
> >
> >  // Local variables
> >  STATIC CONST UINT8*  ProcessorTopologyStructureType; @@ -19,11
> +22,80
> > @@ STATIC CONST UINT32* NumberOfPrivateResources;  STATIC
> > ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> >
> >  /**
> > -  An ACPI_PARSER array describing the ACPI PPTT Table.
> > +  This function validates the Cache Type Structure (Type 1) 'Number of
> sets'
> > +  field.
> > +
> > +  @param [in] Ptr       Pointer to the start of the field data.
> > +  @param [in] Context   Pointer to context specific information e.g. this
> > +                        could be a pointer to the ACPI table header.
> >  **/
> > -STATIC CONST ACPI_PARSER PpttParser[] = {
> > -  PARSE_ACPI_HEADER (&AcpiHdrInfo)
> > -};
> > +STATIC
> > +VOID
> > +EFIAPI
> > +ValidateCacheNumberOfSets (
> > +  IN UINT8* Ptr,
> > +  IN VOID*  Context
> > +  )
> > +{
> > +  UINT32 NumberOfSets;
> > +  NumberOfSets = *(UINT32*)Ptr;
> > +
> > +  if (NumberOfSets == 0) {
> > +    IncrementErrorCount ();
> > +    Print (L"\nERROR: Cache number of sets must be greater than 0");
> > +    return;
> > +  }
> > +
> > +#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> > +  if (NumberOfSets >
> PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX)
> > {
> > +    IncrementErrorCount ();
> > +    Print (
> > +      L"\nERROR: When ARMv8.3-CCIDX is implemented the maximum
> cache
> > number of "
> > +        L"sets must be less than or equal to %d",
> > +      PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX
> > +      );
> > +    return;
> > +  }
> > +
> > +  if (NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) {
> > +    IncrementWarningCount ();
> > +    Print (
> > +      L"\nWARNING: Without ARMv8.3-CCIDX, the maximum cache number
> > of sets "
> > +        L"must be less than or equal to %d. Ignore this message if "
> > +        L"ARMv8.3-CCIDX is implemented",
> > +      PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX
> > +      );
> > +    return;
> > +  }
> > +#endif
> > +
> > +}
> > +
> > +/**
> > +  This function validates the Cache Type Structure (Type 1) 'Associativity'
> > +  field.
> > +
> > +  @param [in] Ptr       Pointer to the start of the field data.
> > +  @param [in] Context   Pointer to context specific information e.g. this
> > +                        could be a pointer to the ACPI table header.
> > +**/
> > +STATIC
> > +VOID
> > +EFIAPI
> > +ValidateCacheAssociativity (
> > +  IN UINT8* Ptr,
> > +  IN VOID*  Context
> > +  )
> > +{
> > +  UINT8 Associativity;
> > +  Associativity = *(UINT8*)Ptr;
> > +
> > +  if (Associativity == 0) {
> > +    IncrementErrorCount ();
> > +    Print (L"\nERROR: Cache associativity must be greater than 0");
> > +    return;
> > +  }
> > +}
> 
> I see you add the PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and
> PPTT_ARM_CACHE_ASSOCIATIVITY_MAX but they are not include in
> ValidateCacheAssociativity.
> Is this a missing?
> 
> Thanks,
> Zhichao
> 
> [Krzysztof] I added PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and
> PPTT_ARM_CACHE_ASSOCIATIVITY_MAX to entirely cover Arm architecture.
> However, the Associativity field in PPTT Type 1 structure is only one byte long,
> therefore, values of 2^10 or 2^21 will never be reached.
> 
> These macros are not used as of now, but I think that this table field is likely
> to get expanded in the future and
> PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX  and
> PPTT_ARM_CACHE_ASSOCIATIVITY_MAX will become valid checks.
> 
> >
> >  /**
> >    This function validates the Cache Type Structure (Type 1) Line size field.
> > @@ -49,11 +121,14 @@ ValidateCacheLineSize (
> >    UINT16 LineSize;
> >    LineSize = *(UINT16*)Ptr;
> >
> > -  if ((LineSize < 16) || (LineSize > 2048)) {
> > +  if ((LineSize < PPTT_ARM_CACHE_LINE_SIZE_MIN) ||
> > +      (LineSize > PPTT_ARM_CACHE_LINE_SIZE_MAX)) {
> >      IncrementErrorCount ();
> >      Print (
> > -      L"\nERROR: The cache line size must be between 16 and 2048 bytes"
> > -        L" on ARM Platforms."
> > +      L"\nERROR: The cache line size must be between %d and %d bytes"
> > +        L" on ARM Platforms.",
> > +      PPTT_ARM_CACHE_LINE_SIZE_MIN,
> > +      PPTT_ARM_CACHE_LINE_SIZE_MAX
> >        );
> >      return;
> >    }
> > @@ -96,6 +171,13 @@ ValidateCacheAttributes (
> >    }
> >  }
> >
> > +/**
> > +  An ACPI_PARSER array describing the ACPI PPTT Table.
> > +**/
> > +STATIC CONST ACPI_PARSER PpttParser[] = {
> > +  PARSE_ACPI_HEADER (&AcpiHdrInfo)
> > +};
> > +
> >  /**
> >    An ACPI_PARSER array describing the processor topology structure
> header.
> >  **/
> > @@ -133,8 +215,8 @@ STATIC CONST ACPI_PARSER
> > CacheTypeStructureParser[] = {
> >    {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
> >    {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
> >    {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
> > -  {L"Number of sets", 4, 16, L"%d", NULL, NULL, NULL, NULL},
> > -  {L"Associativity", 1, 20, L"%d", NULL, NULL, NULL, NULL},
> > +  {L"Number of sets", 4, 16, L"%d", NULL, NULL,
> > + ValidateCacheNumberOfSets, NULL},  {L"Associativity", 1, 20, L"%d",
> > + NULL, NULL, ValidateCacheAssociativity, NULL},
> >    {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes,
> NULL},
> >    {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize,
> > NULL}  }; diff --git
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> > h
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> > h
> > new file mode 100644
> > index
> >
> 0000000000000000000000000000000000000000..2a671203fb0035bbc407ff4bb0
> > ca9960706fa588
> > --- /dev/null
> > +++
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> > +++ er.h
> > @@ -0,0 +1,38 @@
> > +/** @file
> > +  Header file for PPTT parser
> > +
> > +  Copyright (c) 2019, ARM Limited. All rights reserved.
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +  @par Reference(s):
> > +    - ARM Architecture Reference Manual ARMv8 (D.a) **/
> > +
> > +#ifndef PPTT_PARSER_H_
> > +#define PPTT_PARSER_H_
> > +
> > +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> > +
> > +/// Cache parameters allowed by the architecture with ///
> > +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from
> > +CCSIDR_EL1 when
> > +ID_AA64MMFR2_EL1.CCIDX==0001
> > +#define PPTT_ARM_CCIDX_CACHE_NUMBER_OF_SETS_MAX       (1 << 24)
> > +#define PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX        (1 << 21)
> > +
> > +/// Cache parameters allowed by the architecture without ///
> > +ARMv8.3-CCIDX (Cache extended number of sets) /// Derived from
> > +CCSIDR_EL1 when ID_AA64MMFR2_EL1.CCIDX==0000
> > +#define PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX             (1 << 15)
> > +#define PPTT_ARM_CACHE_ASSOCIATIVITY_MAX              (1 << 10)
> > +
> > +/// Common cache parameters
> > +/// Derived from CCSIDR_EL1
> > +/// The LineSize is represented by bits 2:0 /// (Log2(Number of bytes
> > +in cache line)) - 4 is used to represent /// the LineSize bits.
> > +#define PPTT_ARM_CACHE_LINE_SIZE_MAX                  (1 << 11)
> > +#define PPTT_ARM_CACHE_LINE_SIZE_MIN                  (1 << 4)
> > +
> > +#endif // if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> > +
> > +#endif // PPTT_PARSER_H_
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> 
> 
> 

Re: [PATCH v1 0/4] Fix a number of small issues in acpiview

[Sami Mujawar]

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 28 June 2019 11:25 AM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; zhichao.gao@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 0/4] Fix a number of small issues in acpiview

The following patches introduce a number of unrelated changes which fix a number of minor issues in the Acpiview UEFI shell tool.

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_fixes_v1

Krzysztof Koch (4):
  ShellPkg: acpiview: Improve PPTT table field validation
  ShellPkg: acpiview: Make DBG2 output consistent with other tables
  ShellPkg: acpiview: Remove redundant IORT node types enum
  ShellPkg: acpiview: Remove duplicate indentation in IORT parser

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |   3 +-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c |  48 ++++-----  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 102 ++++++++++++++++++--  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h |  38 ++++++++
 4 files changed, 149 insertions(+), 42 deletions(-)  create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.h

--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'