[PATCH v1 00/19] Fix warnings reported by VS2017 compiler

[PATCH v1 00/19] Fix warnings reported by VS2017 compiler

[Sami Mujawar]

This patch series:
  - fixes warnings reported by the VS2017 compiler
  - adds an option to enable VS2017 static code
    analysis
  - fixes warnings reported by the edk2 build
    system

The issues fixed in this series were identified
by building DynamicTablesPkg/DynamicTablesPkg.dsc
using the VS2017 compiler with the static code
analysis option.

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/503_vs2017_compile_issue_v1

Sami Mujawar (19):
  DynamicTablesPkg: Fix entry point param definition
  DynamicTablesPkg: Fix missing local header warning
  DynamicTablesPkg: Remove struct CM_ARM_CPU_INFO
  DynamicTablesPkg: Fix serial port subtype warning
  DynamicTablesPkg: Fix Proc node length assignment
  DynamicTablesPkg: Fix GT Block length assignment
  DynamicTablesPkg: Fix Boot arch flag width
  DynamicTablesPkg: Fix ACPI table rev field width
  DynamicTablesPkg: Fix unaligned pointers usage
  DynamicTablesPkg: Serial debug port initialisation
  DynamicTablesPkg: Remove redundant frame count check
  DynamicTablesPkg: Fix IORT node length assignment
  DynamicTablesPkg: IORT: Fix uninitialized memory usage
  DynamicTablesPkg: PPTT: Fix uninitialized memory usage
  DynamicTablesPkg: Remove erroneous use of EFIAPI
  DynamicTablesPkg: Option for VS2017 static code analysis
  ArmPlatformPkg: Fix UART divisor warning
  ArmPlatformPkg: Fix comparison of constants warning
  MdePkg: Initialise VA_LIST variables before use

 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c                         |  16 +-
 DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c   |   6 +-
 DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf |   1 +
 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c   |   4 +-
 DynamicTablesPkg/DynamicTablesPkg.dsc                                      |   9 +-
 DynamicTablesPkg/Include/AcpiTableGenerator.h                              |   4 +-
 DynamicTablesPkg/Include/ArmNameSpaceObjects.h                             |   6 +-
 DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h            |  11 +-
 DynamicTablesPkg/Include/StandardNameSpaceObjects.h                        |   2 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c           |  35 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c           |   8 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c           |  43 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf        |   3 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c           | 454 +++++++++++++-------
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c           |   8 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c           |   8 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf        |   1 +
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c           |  98 +++--
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h           |  13 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c             |   8 +-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c           |  23 +-
 MdePkg/Library/BaseLib/SwitchStack.c                                       |   9 +
 MdePkg/Library/BasePrintLib/PrintLib.c                                     |   5 +
 MdePkg/Library/BasePrintLib/PrintLibInternal.c                             |   9 +
 24 files changed, 501 insertions(+), 283 deletions(-)

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

[PATCH v1 01/19] DynamicTablesPkg: Fix entry point param definition

[Sami Mujawar]

VS2017 reports 'warning C4028: formal parameter 2 different
from declaration' for the library constructor and destructor
interfaces for the Generator modules. VS2017 compiler also
reports similar warnings for the DXE entry points.

Remove the CONST qualifier for the SystemTable pointer (the
second parameter to the constructor/destructor/DXE Entry
point) to make it compatible with the formal declaration.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c | 6 +++---
 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 4 ++--
 DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c           | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c         | 8 ++++----
 11 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c
index d87ea7809eae09cdb5ff06290cff8c2d7e7fa904..d1432348f05143bed8588cea54d7b9b6722b9b6d 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c
@@ -1,7 +1,7 @@
 /** @file
   Dynamic Table Factory Dxe
 
-  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -60,8 +60,8 @@ EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL DynamicTableFactoryProtocol = {
 EFI_STATUS
 EFIAPI
 DynamicTableFactoryDxeInitialize (
-  IN  EFI_HANDLE                 ImageHandle,
-  IN  EFI_SYSTEM_TABLE   * CONST SystemTable
+  IN  EFI_HANDLE            ImageHandle,
+  IN  EFI_SYSTEM_TABLE   *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index 39b75996d9922d2daf953bb28809aa285f7d214c..e27dcaf37470d811c4d6218d4f40d2260f550c02 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -655,8 +655,8 @@ ProcessAcpiTables (
 EFI_STATUS
 EFIAPI
 DynamicTableManagerDxeInitialize (
-  IN CONST EFI_HANDLE                 ImageHandle,
-  IN       EFI_SYSTEM_TABLE   * CONST SystemTable
+  IN  EFI_HANDLE            ImageHandle,
+  IN  EFI_SYSTEM_TABLE   *  SystemTable
   )
 {
   EFI_STATUS                                 Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
index 0b45e15d2d1c2c1e35e801ca719d68d9ff88a65e..346ab5b22f5402bf87c385558f68f080d1b454ed 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
@@ -418,8 +418,8 @@ ACPI_TABLE_GENERATOR Dbg2Generator = {
 EFI_STATUS
 EFIAPI
 AcpiDbg2LibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -442,8 +442,8 @@ AcpiDbg2LibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiDbg2LibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
index e9e4991dc127abb97f859436df0c4ceafdb3f5ec..6cb60e898f697ce4347ad3adeb5bca1cc1f30a0c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
@@ -648,8 +648,8 @@ ACPI_TABLE_GENERATOR FadtGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiFadtLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -671,8 +671,8 @@ AcpiFadtLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiFadtLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
index a30870806d7f223fde7311e3609cca92f47731cc..7e86c30649bd36ecebac75c7e3a86a0c25cd590b 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
@@ -738,8 +738,8 @@ ACPI_TABLE_GENERATOR GtdtGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiGtdtLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -761,8 +761,8 @@ AcpiGtdtLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiGtdtLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
index 05e360c899c7563aed04c071921865929524fdc4..f303e9f29cc700634d236c08505cab91b9d62cb8 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
@@ -2040,8 +2040,8 @@ ACPI_IORT_GENERATOR IortGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiIortLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -2063,8 +2063,8 @@ AcpiIortLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiIortLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
index dc5238078376746847e7ae14258bbe35ca07ca2c..2651ca15414694c494678bcec655fc882ee2cc0c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
@@ -795,8 +795,8 @@ ACPI_TABLE_GENERATOR MadtGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiMadtLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -818,8 +818,8 @@ AcpiMadtLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiMadtLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
index d219e2f04ad6602b8157b048ae840a3094492680..1293ac8e43c8e0ed641b20ba42519172bc541b54 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
@@ -329,8 +329,8 @@ ACPI_TABLE_GENERATOR McfgGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiMcfgLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -352,8 +352,8 @@ AcpiMcfgLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiMcfgLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index c8713dec625cc18b0806659ffcdc3d9e70411e11..9e42eee9b75bb330833b0f56c98947563d9eb821 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -1492,8 +1492,8 @@ ACPI_PPTT_GENERATOR PpttGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiPpttLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -1516,8 +1516,8 @@ AcpiPpttLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiPpttLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
index 544ea06f0bcb3442b78cf6409846f70ae5360664..2facfaa048115575829c9787084876efbe60834a 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
@@ -109,8 +109,8 @@ ACPI_TABLE_GENERATOR RawGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiRawLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -132,8 +132,8 @@ AcpiRawLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiRawLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
index 7b70ba2c2c55a9a15a569680d1a57b359db4e73d..1404279f828d5c06bb7605cb2fe6d864ef7a080e 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
@@ -308,8 +308,8 @@ ACPI_TABLE_GENERATOR SpcrGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiSpcrLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -331,8 +331,8 @@ AcpiSpcrLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiSpcrLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 02/19] DynamicTablesPkg: Fix missing local header warning

[Sami Mujawar]

The edk2 BaseTools report a warning if a local header file
is not listed under the [Sources] section in the INF file.

Add header files to the [Sources] section in the respective
INF files to fix the warnings.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf | 1 +
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf        | 3 ++-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf        | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
index b47a690dd72702a2d7157cbb21249ba1d01d08cd..56b69591ac52ca3ba51250dc01a0846b22442ce9 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
@@ -25,6 +25,7 @@ [Sources]
   DeviceTreeTableFactory/DeviceTreeTableFactory.c
   DynamicTableFactoryDxe.c
   SmbiosTableFactory/SmbiosTableFactory.c
+  DynamicTableFactory.h
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
index 3bbd0e7818c6172f9178b12a3d0960e2d298bca1..2f987e3b6d339d4b5f342cb52b8dc712cf4e585c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
@@ -1,7 +1,7 @@
 ## @file
 #  IORT Table Generator
 #
-#  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+#  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 ##
@@ -18,6 +18,7 @@ [Defines]
 
 [Sources]
   IortGenerator.c
+  IortGenerator.h
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
index 3cb13d7d8fd3df04f3e16cc6161d9736f7579a80..2c7d19513d6ceea29fd9f271f983a6cf6d2c1046 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
@@ -18,6 +18,7 @@ [Defines]
 
 [Sources]
   PpttGenerator.c
+  PpttGenerator.h
 
 [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 03/19] DynamicTablesPkg: Remove struct CM_ARM_CPU_INFO

[Sami Mujawar]

The VS2017 compiler reports 'error C2016: C requires that
a struct or union has at least one member' for the struct
CM_ARM_CPU_INFO.

Remove struct CM_ARM_CPU_INFO as this is not in use.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
index ac451b306dfd7ba299a83209675b21696be235be..f08b8257678c8b6aacc863bbe1db09587b51a291 100644
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
@@ -67,10 +67,6 @@ typedef struct CmArmBootArchInfo {
   UINT32  BootArchFlags;
 } CM_ARM_BOOT_ARCH_INFO;
 
-typedef struct CmArmCpuInfo {
-  // Reserved for use when SMBIOS tables are implemented
-} CM_ARM_CPU_INFO;
-
 /** A structure that describes the
     Power Management Profile Information for the Platform.
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 04/19] DynamicTablesPkg: Fix serial port subtype warning

[Sami Mujawar]

The VS2017 compiler reports 'warning C4244: '=': conversion
from 'UINT16' to 'UINT8', possible loss of data' for the
SPCR InterfaceType field assignment.

The SPCR InterfaceType field uses the same encoding as that
of the DBG2 table Port Subtype field. However SPCR.InterfaceType
is 8-bit while the Port Subtype field in DBG2 table is 16-bit.

Since the Configuration Manager represents the Serial port
information using the struct CM_ARM_SERIAL_PORT_INFO, the
PortSubtype member in this struct is 16-bit.

To fix the warning an explicit type case is added. A validation
is also added to ensure that the Serial Port Subtype value
provided by the Configuration Manager is within the 8-bit
range (less than 256).

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
index 1404279f828d5c06bb7605cb2fe6d864ef7a080e..4b2580da7df9b4472ae9bcc3df2138a9e13004d0 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
@@ -217,8 +217,21 @@ BuildSpcrTable (
     goto error_handler;
   }
 
+  // The SPCR InterfaceType uses the same encoding as that of the
+  // DBG2 table Port Subtype field. However InterfaceType is 8-bit
+  // while the Port Subtype field in the DBG2 table is 16-bit.
+  if ((SerialPortInfo->PortSubtype & 0xFF00) != 0) {
+    Status = EFI_INVALID_PARAMETER;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: SPCR: Invalid Port Sybtype (must be < 256). Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
   // Update the serial port subtype
-  AcpiSpcr.InterfaceType = SerialPortInfo->PortSubtype;
+  AcpiSpcr.InterfaceType = (UINT8)SerialPortInfo->PortSubtype;
 
   // Update the base address
   AcpiSpcr.BaseAddress.Address = SerialPortInfo->BaseAddress;
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 05/19] DynamicTablesPkg: Fix Proc node length assignment

[Sami Mujawar]

The length field for the Processor Hierarchy node structure is
8-bit wide while the number of private resource field is 32-bit
wide. Therefore, the GetProcHierarchyNodeSize() returns the size
as a 32-bit value.

The VS2017 compiler reports 'warning C4244: '=': conversion from
'UINT32' to 'UINT8', possible loss of data' while assigning the
length field of the Processor Hierarchy node structure.

To fix this, a type cast is added. In addition, there is a check
to ensure that the Processor Hierarchy node size does not exceed
MAX_UINT8.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index 9e42eee9b75bb330833b0f56c98947563d9eb821..40699ce113caa8530c89ac20562cf5abda26b88e 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -504,6 +504,7 @@ AddProcHierarchyNodes (
 
   PPTT_NODE_INDEXER                     * ProcNodeIterator;
   UINT32                                  NodeCount;
+  UINT32                                  Length;
 
   ASSERT (
     (Generator != NULL) &&
@@ -539,8 +540,8 @@ AddProcHierarchyNodes (
     // imposed on the Processor Hierarchy node by the specification.
     // Note: The length field is 8 bit wide while the number of private
     // resource field is 32 bit wide.
-    if ((sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR) +
-        (ProcInfoNode->NoOfPrivateResources * sizeof (UINT32))) > MAX_UINT8) {
+    Length = GetProcHierarchyNodeSize (ProcInfoNode);
+    if (Length > MAX_UINT8) {
       Status = EFI_INVALID_PARAMETER;
       DEBUG ((
         DEBUG_ERROR,
@@ -556,7 +557,7 @@ AddProcHierarchyNodes (
 
     // Populate the node header
     ProcStruct->Type = EFI_ACPI_6_3_PPTT_TYPE_PROCESSOR;
-    ProcStruct->Length = GetProcHierarchyNodeSize (ProcInfoNode);
+    ProcStruct->Length = (UINT8)Length;
     ProcStruct->Reserved[0] = EFI_ACPI_RESERVED_BYTE;
     ProcStruct->Reserved[1] = EFI_ACPI_RESERVED_BYTE;
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 06/19] DynamicTablesPkg: Fix GT Block length assignment

[Sami Mujawar]

The VS2017 compiler reports 'warning C4267: '=': conversion from
'size_t' to 'UINT16', possible loss of data'.

The sizeof() operator is used to calculate the size of the
GT Block structure. The length field in the GT Block structure
is 16-bit wide. Since the return type of sizeof() operator
is size_t the VS2017 compiler reports the above warning.

To fix the warning, an explicit type cast is added. An additional
check is also performed to ensure that the calculated GT Block
length does not exceed MAX_UINT16.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c | 25 ++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
index 7e86c30649bd36ecebac75c7e3a86a0c25cd590b..0e996698887aefca8a7240fc27fe3cb9324fd3e2 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
@@ -350,6 +350,7 @@ AddGTBlockList (
   EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE  * GtBlockFrame;
   CM_ARM_GTBLOCK_TIMER_FRAME_INFO             * GTBlockTimerFrameList;
   UINT32                                        GTBlockTimerFrameCount;
+  UINTN                                         Length;
 
   ASSERT (Gtdt != NULL);
   ASSERT (GTBlockInfo != NULL);
@@ -376,11 +377,27 @@ AddGTBlockList (
       return Status;
     }
 
+    Length = sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_STRUCTURE) +
+               (sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE) *
+                GTBlockInfo->GTBlockTimerFrameCount);
+
+    // Check that the length of the GT block does not
+    // exceed MAX_UINT16
+    if (Length > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: GTDT: Too many GT Frames. Count = %d. " \
+        "Maximum supported GT Block size exceeded. " \
+        "Status = %r\n",
+        GTBlockInfo->GTBlockTimerFrameCount,
+        Status
+        ));
+      return Status;
+    }
+
     GTBlock->Type = EFI_ACPI_6_3_GTDT_GT_BLOCK;
-    GTBlock->Length = sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_STRUCTURE) +
-                        (sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE) *
-                          GTBlockInfo->GTBlockTimerFrameCount);
-
+    GTBlock->Length = (UINT16)Length;
     GTBlock->Reserved = EFI_ACPI_RESERVED_BYTE;
     GTBlock->CntCtlBase = GTBlockInfo->GTBlockPhysicalAddress;
     GTBlock->GTBlockTimerCount = GTBlockInfo->GTBlockTimerFrameCount;
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 07/19] DynamicTablesPkg: Fix Boot arch flag width

[Sami Mujawar]

The ArmBootArch field of the FADT table is 16-bit wide. The
VS2017 compiler reports 'warning C4244: '=': conversion from
'UINT32' to 'UINT16', possible loss of data' when assigning the
CM_ARM_BOOT_ARCH_INFO.BootArchFlags value as the width of this
field in CM_ARM_BOOT_ARCH_INFO is 32-bit wide.

To fix this warning, update the CM_ARM_BOOT_ARCH_INFO struct
to make the  BootArchFlags field 16-bit wide. This also makes
it compatible with the ACPI FADT specification.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
index f08b8257678c8b6aacc863bbe1db09587b51a291..cd955379c90d8633c89e324b5567643d2676e557 100644
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
@@ -64,7 +64,7 @@ typedef struct CmArmBootArchInfo {
   /** This is the ARM_BOOT_ARCH flags field of the FADT Table
       described in the ACPI Table Specification.
   */
-  UINT32  BootArchFlags;
+  UINT16  BootArchFlags;
 } CM_ARM_BOOT_ARCH_INFO;
 
 /** A structure that describes the
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 08/19] DynamicTablesPkg: Fix ACPI table rev field width

[Sami Mujawar]

The VS2017 compiler reports 'warning C4244: '=': conversion from
'const UINT32' to 'UINT8', possible loss of data' when the ACPI
table revision field is being updated.

The width of the revision field in the EFI_ACPI_DESCRIPTION_HEADER
struct is 8-bit wide. Therefore, to fix the above warning make the
ACPI Table revision field usage 8-bit wide across Dynamic Tables
Framework.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/AcpiTableGenerator.h       | 4 ++--
 DynamicTablesPkg/Include/StandardNameSpaceObjects.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h
index 7d6d3442276db7b4abaeb3b053ba489258adea0b..d7a1209c4d40c2e6ffb3dac786c276a30d70b6df 100644
--- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
+++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
@@ -284,10 +284,10 @@ typedef struct AcpiTableGenerator {
   UINT32                                 AcpiTableSignature;
 
   /// The ACPI table revision.
-  UINT32                                 AcpiTableRevision;
+  UINT8                                  AcpiTableRevision;
 
   /// The minimum supported ACPI table revision.
-  UINT32                                 MinAcpiTableRevision;
+  UINT8                                  MinAcpiTableRevision;
 
   /// The ACPI table creator ID.
   UINT32                                 CreatorId;
diff --git a/DynamicTablesPkg/Include/StandardNameSpaceObjects.h b/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
index 21bb7de4044ffd6f97156f761e30fc1790f11c0c..0ba6b163691ea127ae7a7d57fb5f9fdf84789338 100644
--- a/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
@@ -79,7 +79,7 @@ typedef struct CmAStdObjAcpiTableInfo {
   UINT32                         AcpiTableSignature;
 
   /// The ACPI table revision
-  UINT32                         AcpiTableRevision;
+  UINT8                          AcpiTableRevision;
 
   /// The ACPI Table Generator ID
   ACPI_TABLE_GENERATOR_ID        TableGeneratorId;
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 09/19] DynamicTablesPkg: Fix unaligned pointers usage

[Sami Mujawar]

The VS2017 compiler reports 'warning C4366: The result of
the unary '&' operator may be unaligned' if an address of
an unaligned structure member is passed as an argument to
a function.

Fix this warning by using local variables in place of
unaligned structure members.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 24 +++++++++++++++-----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h | 13 +++++------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index 40699ce113caa8530c89ac20562cf5abda26b88e..82070403ac8757f54e839fd00eb4acb3292fc60c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -1066,6 +1066,9 @@ BuildPpttTable (
   EFI_STATUS                      Status;
   UINT32                          TableSize;
   UINT32                          ProcTopologyStructCount;
+  UINT32                          ProcHierarchyNodeCount;
+  UINT32                          CacheStructCount;
+  UINT32                          IdStructCount;
 
   UINT32                          ProcHierarchyNodeOffset;
   UINT32                          CacheStructOffset;
@@ -1113,7 +1116,7 @@ BuildPpttTable (
              CfgMgrProtocol,
              CM_NULL_TOKEN,
              &ProcHierarchyNodeList,
-             &Generator->ProcHierarchyNodeCount
+             &ProcHierarchyNodeCount
              );
   if (EFI_ERROR (Status)) {
     DEBUG ((
@@ -1124,7 +1127,8 @@ BuildPpttTable (
     goto error_handler;
   }
 
-  ProcTopologyStructCount = Generator->ProcHierarchyNodeCount;
+  ProcTopologyStructCount = ProcHierarchyNodeCount;
+  Generator->ProcHierarchyNodeCount = ProcHierarchyNodeCount;
 
   // Get the cache info and update the processor topology structure count with
   // Cache Type Structures (Type 1)
@@ -1132,7 +1136,7 @@ BuildPpttTable (
              CfgMgrProtocol,
              CM_NULL_TOKEN,
              &CacheStructList,
-             &Generator->CacheStructCount
+             &CacheStructCount
              );
   if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
     DEBUG ((
@@ -1143,7 +1147,8 @@ BuildPpttTable (
     goto error_handler;
   }
 
-  ProcTopologyStructCount += Generator->CacheStructCount;
+  ProcTopologyStructCount += CacheStructCount;
+  Generator->CacheStructCount = CacheStructCount;
 
   // Get the processor hierarchy node ID info and update the processor topology
   // structure count with ID Structures (Type 2)
@@ -1151,7 +1156,7 @@ BuildPpttTable (
              CfgMgrProtocol,
              CM_NULL_TOKEN,
              &IdStructList,
-             &Generator->IdStructCount
+             &IdStructCount
              );
   if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
     DEBUG ((
@@ -1163,7 +1168,8 @@ BuildPpttTable (
     goto error_handler;
   }
 
-  ProcTopologyStructCount += Generator->IdStructCount;
+  ProcTopologyStructCount += IdStructCount;
+  Generator->IdStructCount = IdStructCount;
 
   // Allocate Node Indexer array
   NodeIndexer = (PPTT_NODE_INDEXER*)AllocateZeroPool (
@@ -1475,6 +1481,12 @@ ACPI_PPTT_GENERATOR PpttGenerator = {
 
   // Processor topology node count
   0,
+  // Count of Processor Hierarchy Nodes
+  0,
+  // Count of Cache Structures
+  0,
+  // Count of Id Structures
+  0,
   // Pointer to PPTT Node Indexer
   NULL
 };
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h
index 6a0fdd08e1533c57285f420555586314c70a5ed5..0a14da502d595e27d87262b1bac681318f1d9ced 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h
@@ -167,6 +167,12 @@ typedef struct AcpiPpttGenerator {
   ACPI_TABLE_GENERATOR  Header;
   /// PPTT structure count
   UINT32                ProcTopologyStructCount;
+  /// Count of Processor Hierarchy Nodes
+  UINT32                ProcHierarchyNodeCount;
+  /// Count of Cache Structures
+  UINT32                CacheStructCount;
+  /// Count of Id Structures
+  UINT32                IdStructCount;
   /// List of indexed CM objects for PPTT generation
   PPTT_NODE_INDEXER   * NodeIndexer;
   /// Pointer to the start of Processor Hierarchy nodes in
@@ -176,13 +182,6 @@ typedef struct AcpiPpttGenerator {
   PPTT_NODE_INDEXER   * CacheStructIndexedList;
   /// Pointer to the start of Id Structures in the Node Indexer array
   PPTT_NODE_INDEXER   * IdStructIndexedList;
-  /// Count of Processor Hierarchy Nodes
-  UINT32                ProcHierarchyNodeCount;
-  /// Count of Cache Structures
-  UINT32                CacheStructCount;
-  /// Count of Id Structures
-  UINT32                IdStructCount;
-
 } ACPI_PPTT_GENERATOR;
 
 #pragma pack()
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

[Sami Mujawar]

The ARM DCC serial port subtype is an option that is
supported by the DBG2 generator. However, the serial
port initialisation should only be done for PL011/SBSA
compatible UARTs.

Add check to conditionally initialise the serial port.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
@@ -175,7 +175,7 @@ GET_OBJECT_LIST (
   CM_ARM_SERIAL_PORT_INFO
   );
 
-/** Initialize the PL011 UART with the parameters obtained from
+/** Initialize the PL011/SBSA UART with the parameters obtained from
     the Configuration Manager.
 
   @param [in]  SerialPortInfo Pointer to the Serial Port Information.
@@ -353,15 +353,22 @@ BuildDbg2Table (
   AcpiDbg2.Dbg2DeviceInfo[DBG_PORT_INDEX_PORT1].Dbg2Device.PortSubtype =
     SerialPortInfo->PortSubtype;
 
-  // Initialize the serial port
-  Status = SetupDebugUart (SerialPortInfo);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((
-      DEBUG_ERROR,
-      "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
-      Status
-      ));
-    goto error_handler;
+  if ((SerialPortInfo->PortSubtype ==
+      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_PL011_UART)           ||
+      (SerialPortInfo->PortSubtype ==
+      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART_2X) ||
+      (SerialPortInfo->PortSubtype ==
+      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART)) {
+    // Initialize the serial port
+    Status = SetupDebugUart (SerialPortInfo);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
   }
 
   *Table = (EFI_ACPI_DESCRIPTION_HEADER*)&AcpiDbg2;
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 11/19] DynamicTablesPkg: Remove redundant frame count check

[Sami Mujawar]

Removing GT Block frame count check from AddGTBlockTimerFrames()
as this is already validated in BuildGtdtTable().

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
index 0e996698887aefca8a7240fc27fe3cb9324fd3e2..c109f2ac207311646e5a4ad3aa465b1c2883e465 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
@@ -259,16 +259,6 @@ AddGTBlockTimerFrames (
   ASSERT (GtBlockFrame != NULL);
   ASSERT (GTBlockTimerFrameList != NULL);
 
-  if (GTBlockFrameCount > 8) {
-    DEBUG ((
-      DEBUG_ERROR,
-      "ERROR: GTDT: GT Block Frame Count %d is greater than 8\n",
-      GTBlockFrameCount
-      ));
-    ASSERT (GTBlockFrameCount <= 8);
-    return EFI_INVALID_PARAMETER;
-  }
-
   IsFrameNumberDuplicated = FindDuplicateValue (
                               GTBlockTimerFrameList,
                               GTBlockFrameCount,
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 12/19] DynamicTablesPkg: Fix IORT node length assignment

[Sami Mujawar]

The VS2017 compiler reports 'warning C4267: 'return': conversion
from 'size_t' to 'UINT32', possible loss of data' for a number of
functions that compute the IORT node length. Similarly, it reports
warnings for IORT node length field assignments as the length
field is 16-bit wide.

This patch adds type casts at appropriate places and also implements
validations to ensure that the max width of the respective fields
is not exceeded.

This patch also fixes a typo in one of the local variable names.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c | 352 ++++++++++++++------
 1 file changed, 253 insertions(+), 99 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
index f303e9f29cc700634d236c08505cab91b9d62cb8..bd71220ca19679792de2bb6d88eed8d1913a7600 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
@@ -140,8 +140,8 @@ GetItsGroupNodeSize (
   /* Size of ITS Group Node +
      Size of ITS Identifier array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE) +
-           (Node->ItsIdCount * sizeof (UINT32));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE) +
+                    (Node->ItsIdCount * sizeof (UINT32)));
 }
 
 /** Returns the total size required for the ITS Group nodes and
@@ -160,7 +160,7 @@ GetItsGroupNodeSize (
     @retval Total size of the ITS Group Nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofItsGroupNodes (
   IN      CONST UINT32                         NodeStartOffset,
   IN      CONST CM_ARM_ITS_GROUP_NODE  *       NodeList,
@@ -168,7 +168,7 @@ GetSizeofItsGroupNodes (
   IN OUT        IORT_NODE_INDEXER     ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;
 
   ASSERT (NodeList != NULL);
 
@@ -176,7 +176,7 @@ GetSizeofItsGroupNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -211,10 +211,10 @@ GetNamedComponentNodeSize (
      Size of ID mapping array +
      Size of ASCII string + 'padding to 32-bit word aligned'.
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
-            (Node->IdMappingCount *
-             sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
-            ALIGN_VALUE (AsciiStrSize (Node->ObjectName), 4);
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
+                    ALIGN_VALUE (AsciiStrSize (Node->ObjectName), 4));
 }
 
 /** Returns the total size required for the Named Component nodes and
@@ -233,7 +233,7 @@ GetNamedComponentNodeSize (
     @retval Total size of the Named Component nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofNamedComponentNodes (
   IN      CONST UINT32                              NodeStartOffset,
   IN      CONST CM_ARM_NAMED_COMPONENT_NODE *       NodeList,
@@ -241,7 +241,7 @@ GetSizeofNamedComponentNodes (
   IN OUT        IORT_NODE_INDEXER          ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;
 
   ASSERT (NodeList != NULL);
 
@@ -249,7 +249,7 @@ GetSizeofNamedComponentNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -284,9 +284,9 @@ GetRootComplexNodeSize (
   /* Size of Root Complex node +
      Size of ID mapping array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_RC_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_RC_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)));
 }
 
 /** Returns the total size required for the Root Complex nodes and
@@ -305,7 +305,7 @@ GetRootComplexNodeSize (
     @retval Total size of the Root Complex nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofRootComplexNodes (
   IN      CONST UINT32                              NodeStartOffset,
   IN      CONST CM_ARM_ROOT_COMPLEX_NODE    *       NodeList,
@@ -313,7 +313,7 @@ GetSizeofRootComplexNodes (
   IN OUT        IORT_NODE_INDEXER          ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;
 
   ASSERT (NodeList != NULL);
 
@@ -321,7 +321,7 @@ GetSizeofRootComplexNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -358,13 +358,13 @@ GetSmmuV1V2NodeSize (
      Size of context interrupt array +
      Size of PMU interrupt array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
-           (Node->ContextInterruptCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT)) +
-           (Node->PmuInterruptCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
+                    (Node->ContextInterruptCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT)) +
+                    (Node->PmuInterruptCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT)));
 }
 
 /** Returns the total size required for the SMMUv1/SMMUv2 nodes and
@@ -383,7 +383,7 @@ GetSmmuV1V2NodeSize (
     @retval Total size of the SMMUv1/SMMUv2 nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofSmmuV1V2Nodes (
   IN      CONST UINT32                              NodeStartOffset,
   IN      CONST CM_ARM_SMMUV1_SMMUV2_NODE   *       NodeList,
@@ -391,7 +391,7 @@ GetSizeofSmmuV1V2Nodes (
   IN OUT        IORT_NODE_INDEXER          ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;
 
   ASSERT (NodeList != NULL);
 
@@ -399,7 +399,7 @@ GetSizeofSmmuV1V2Nodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -433,9 +433,9 @@ GetSmmuV3NodeSize (
   /* Size of SMMU v1/SMMU v2 node +
      Size of ID mapping array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)));
 }
 
 /** Returns the total size required for the SMMUv3 nodes and
@@ -454,7 +454,7 @@ GetSmmuV3NodeSize (
     @retval Total size of the SMMUv3 nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofSmmuV3Nodes (
   IN      CONST UINT32                       NodeStartOffset,
   IN      CONST CM_ARM_SMMUV3_NODE   *       NodeList,
@@ -462,7 +462,7 @@ GetSizeofSmmuV3Nodes (
   IN OUT        IORT_NODE_INDEXER   ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;
 
   ASSERT (NodeList != NULL);
 
@@ -470,7 +470,7 @@ GetSizeofSmmuV3Nodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -504,9 +504,9 @@ GetPmcgNodeSize (
   /* Size of PMCG node +
      Size of ID mapping array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)));
 }
 
 /** Returns the total size required for the PMCG nodes and
@@ -525,7 +525,7 @@ GetPmcgNodeSize (
     @retval Total size of the PMCG nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofPmcgNodes (
   IN      CONST UINT32                     NodeStartOffset,
   IN      CONST CM_ARM_PMCG_NODE   *       NodeList,
@@ -533,7 +533,7 @@ GetSizeofPmcgNodes (
   IN OUT        IORT_NODE_INDEXER ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;
 
   ASSERT (NodeList != NULL);
 
@@ -541,7 +541,7 @@ GetSizeofPmcgNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -735,6 +735,7 @@ AddItsGroupNodes (
   CM_ARM_ITS_IDENTIFIER               * ItsIdentifier;
   UINT32                                ItsIdentifierCount;
   UINT32                                IdIndex;
+  UINT64                                NodeLength;
 
   ASSERT (Iort != NULL);
 
@@ -742,9 +743,22 @@ AddItsGroupNodes (
                   NodesStartOffset);
 
   while (NodeCount-- != 0) {
+    NodeLength = GetItsGroupNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: ITS Id Array Node length 0x%lx > MAX_UINT16."
+        " Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     ItsGroupNode->Node.Type = EFI_ACPI_IORT_TYPE_ITS_GROUP;
-    ItsGroupNode->Node.Length = GetItsGroupNodeSize (NodeList);
+    ItsGroupNode->Node.Length = (UINT16)NodeLength;
     ItsGroupNode->Node.Revision = 0;
     ItsGroupNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     ItsGroupNode->Node.NumIdMappings = 0;
@@ -825,8 +839,9 @@ AddNamedComponentNodes (
   EFI_STATUS                                   Status;
   EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE  * NcNode;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE         * IdMapArray;
-  UINT32                                       ObjectNameLenght;
   CHAR8                                      * ObjectName;
+  UINTN                                        ObjectNameLength;
+  UINT64                                       NodeLength;
 
   ASSERT (Iort != NULL);
 
@@ -834,18 +849,30 @@ AddNamedComponentNodes (
             NodesStartOffset);
 
   while (NodeCount-- != 0) {
+    NodeLength = GetNamedComponentNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Named Component Node length 0x%lx > MAX_UINT16."
+        " Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     NcNode->Node.Type = EFI_ACPI_IORT_TYPE_NAMED_COMP;
-    NcNode->Node.Length =
-      GetNamedComponentNodeSize (NodeList);
+    NcNode->Node.Length = (UINT16)NodeLength;
     NcNode->Node.Revision = 2;
     NcNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     NcNode->Node.NumIdMappings = NodeList->IdMappingCount;
 
-    ObjectNameLenght = AsciiStrLen (NodeList->ObjectName) + 1;
+    ObjectNameLength = AsciiStrLen (NodeList->ObjectName) + 1;
     NcNode->Node.IdReference =
-      sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
-      (ALIGN_VALUE (ObjectNameLenght, 4));
+      (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
+        (ALIGN_VALUE (ObjectNameLength, 4)));
 
     // Named Component specific data
     NcNode->Flags = NodeList->Flags;
@@ -860,7 +887,7 @@ AddNamedComponentNodes (
       sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE));
     Status = AsciiStrCpyS (
                ObjectName,
-               ObjectNameLenght,
+               ObjectNameLength,
                NodeList->ObjectName
                );
     if (EFI_ERROR (Status)) {
@@ -936,6 +963,7 @@ AddRootComplexNodes (
   EFI_STATUS                           Status;
   EFI_ACPI_6_0_IO_REMAPPING_RC_NODE  * RcNode;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE * IdMapArray;
+  UINT64                               NodeLength;
 
   ASSERT (Iort != NULL);
 
@@ -943,9 +971,22 @@ AddRootComplexNodes (
             NodesStartOffset);
 
   while (NodeCount-- != 0) {
+    NodeLength = GetRootComplexNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Root Complex Node length 0x%lx > MAX_UINT16."
+        " Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     RcNode->Node.Type = EFI_ACPI_IORT_TYPE_ROOT_COMPLEX;
-    RcNode->Node.Length = GetRootComplexNodeSize (NodeList);
+    RcNode->Node.Length = (UINT16)NodeLength;
     RcNode->Node.Revision = 1;
     RcNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     RcNode->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1093,6 +1134,7 @@ AddSmmuV1V2Nodes (
 
   EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT  * ContextInterruptArray;
   EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT  * PmuInterruptArray;
+  UINT64                                NodeLength;
 
   ASSERT (Iort != NULL);
 
@@ -1100,9 +1142,21 @@ AddSmmuV1V2Nodes (
               NodesStartOffset);
 
   while (NodeCount-- != 0) {
+    NodeLength = GetSmmuV1V2NodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: SMMU V1/V2 Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     SmmuNode->Node.Type = EFI_ACPI_IORT_TYPE_SMMUv1v2;
-    SmmuNode->Node.Length = GetSmmuV1V2NodeSize (NodeList);
+    SmmuNode->Node.Length = (UINT16)NodeLength;
     SmmuNode->Node.Revision = 0;
     SmmuNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     SmmuNode->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1239,6 +1293,7 @@ AddSmmuV3Nodes (
   EFI_STATUS                             Status;
   EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE * SmmuV3Node;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE   * IdMapArray;
+  UINT64                                 NodeLength;
 
   ASSERT (Iort != NULL);
 
@@ -1246,9 +1301,21 @@ AddSmmuV3Nodes (
                 NodesStartOffset);
 
   while (NodeCount-- != 0) {
+    NodeLength = GetSmmuV3NodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: SMMU V3 Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     SmmuV3Node->Node.Type = EFI_ACPI_IORT_TYPE_SMMUv3;
-    SmmuV3Node->Node.Length = GetSmmuV3NodeSize (NodeList);
+    SmmuV3Node->Node.Length = (UINT16)NodeLength;
     SmmuV3Node->Node.Revision = 2;
     SmmuV3Node->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     SmmuV3Node->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1344,6 +1411,7 @@ AddPmcgNodes (
   EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE  * PmcgNode;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE   * IdMapArray;
   ACPI_IORT_GENERATOR                  * Generator;
+  UINT64                                 NodeLength;
 
   ASSERT (Iort != NULL);
 
@@ -1352,9 +1420,21 @@ AddPmcgNodes (
               NodesStartOffset);
 
   while (NodeCount-- != 0) {
+    NodeLength = GetPmcgNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: PMCG Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     PmcgNode->Node.Type = EFI_ACPI_IORT_TYPE_PMCG;
-    PmcgNode->Node.Length = GetPmcgNodeSize (NodeList);
+    PmcgNode->Node.Length = (UINT16)NodeLength;
     PmcgNode->Node.Revision = 1;
     PmcgNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     PmcgNode->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1448,9 +1528,11 @@ BuildIortTable (
   )
 {
   EFI_STATUS                             Status;
-  UINT32                                 TableSize;
+
+  UINT64                                 TableSize;
+  UINT64                                 NodeSize;
+
   UINT32                                 IortNodeCount;
-
   UINT32                                 ItsGroupNodeCount;
   UINT32                                 NamedComponentNodeCount;
   UINT32                                 RootComplexNodeCount;
@@ -1638,81 +1720,141 @@ BuildIortTable (
 
   // ITS Group Nodes
   if (ItsGroupNodeCount > 0) {
-    ItsGroupOffset = TableSize;
+    ItsGroupOffset = (UINT32)TableSize;
     // Size of ITS Group node list.
-    TableSize += GetSizeofItsGroupNodes (
-                   ItsGroupOffset,
-                   ItsGroupNodeList,
-                   ItsGroupNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofItsGroupNodes (
+                 ItsGroupOffset,
+                 ItsGroupNodeList,
+                 ItsGroupNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of Group Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }
 
   // Named Component Nodes
   if (NamedComponentNodeCount > 0) {
-    NamedComponentOffset = TableSize;
+    NamedComponentOffset = (UINT32)TableSize;
     // Size of Named Component node list.
-    TableSize += GetSizeofNamedComponentNodes (
-                   NamedComponentOffset,
-                   NamedComponentNodeList,
-                   NamedComponentNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofNamedComponentNodes (
+                 NamedComponentOffset,
+                 NamedComponentNodeList,
+                 NamedComponentNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of Named Component Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }
 
   // Root Complex Nodes
   if (RootComplexNodeCount > 0) {
-    RootComplexOffset = TableSize;
+    RootComplexOffset = (UINT32)TableSize;
     // Size of Root Complex node list.
-    TableSize += GetSizeofRootComplexNodes (
-                   RootComplexOffset,
-                   RootComplexNodeList,
-                   RootComplexNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofRootComplexNodes (
+                 RootComplexOffset,
+                 RootComplexNodeList,
+                 RootComplexNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of Root Complex Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }
 
   // SMMUv1/SMMUv2 Nodes
   if (SmmuV1V2NodeCount > 0) {
-    SmmuV1V2Offset = TableSize;
+    SmmuV1V2Offset = (UINT32)TableSize;
     // Size of SMMUv1/SMMUv2 node list.
-    TableSize += GetSizeofSmmuV1V2Nodes (
-                   SmmuV1V2Offset,
-                   SmmuV1V2NodeList,
-                   SmmuV1V2NodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofSmmuV1V2Nodes (
+                 SmmuV1V2Offset,
+                 SmmuV1V2NodeList,
+                 SmmuV1V2NodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of SMMUv1/v2 Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }
 
   // SMMUv3 Nodes
   if (SmmuV3NodeCount > 0) {
-    SmmuV3Offset = TableSize;
+    SmmuV3Offset = (UINT32)TableSize;
     // Size of SMMUv3 node list.
-    TableSize += GetSizeofSmmuV3Nodes (
-                   SmmuV3Offset,
-                   SmmuV3NodeList,
-                   SmmuV3NodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofSmmuV3Nodes (
+                 SmmuV3Offset,
+                 SmmuV3NodeList,
+                 SmmuV3NodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of SMMUv3 Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }
 
   // PMCG Nodes
   if (PmcgNodeCount > 0) {
-    PmcgOffset = TableSize;
+    PmcgOffset = (UINT32)TableSize;
     // Size of PMCG node list.
-    TableSize += GetSizeofPmcgNodes (
-                   PmcgOffset,
-                   PmcgNodeList,
-                   PmcgNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofPmcgNodes (
+                 PmcgOffset,
+                 PmcgNodeList,
+                 PmcgNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of PMCG Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }
 
   DEBUG ((
     DEBUG_INFO,
     "INFO: IORT:\n" \
     " IortNodeCount = %d\n" \
-    " TableSize = %d\n",
+    " TableSize = 0x%lx\n",
     IortNodeCount,
     TableSize
     ));
@@ -1765,6 +1907,18 @@ BuildIortTable (
     PmcgOffset
     ));
 
+  if (TableSize > MAX_UINT32) {
+    Status = EFI_INVALID_PARAMETER;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: IORT: IORT Table Size 0x%lx > MAX_UINT32," \
+      " Status = %r\n",
+      TableSize,
+      Status
+      ));
+    goto error_handler;
+  }
+
   // Allocate the Buffer for IORT table
   *Table = (EFI_ACPI_DESCRIPTION_HEADER*)AllocateZeroPool (TableSize);
   if (*Table == NULL) {
@@ -1783,7 +1937,7 @@ BuildIortTable (
 
   DEBUG ((
     DEBUG_INFO,
-    "IORT: Iort = 0x%p TableSize = 0x%x\n",
+    "IORT: Iort = 0x%p TableSize = 0x%lx\n",
     Iort,
     TableSize
     ));
@@ -1793,7 +1947,7 @@ BuildIortTable (
              This,
              &Iort->Header,
              AcpiTableInfo,
-             TableSize
+             (UINT32)TableSize
              );
   if (EFI_ERROR (Status)) {
     DEBUG ((
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 13/19] DynamicTablesPkg: IORT: Fix uninitialized memory usage

[Sami Mujawar]

On enabling the /analyse option the VS2017 compiler
reports: warning C6001: Using uninitialized memory.

This warning is reported as some variables that were
being logged were uninitialised. To fix this, moved
the logging code after the variables being logged are
initialised.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c | 96 ++++++++++----------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
index bd71220ca19679792de2bb6d88eed8d1913a7600..65d006c89e668c8b2b8a14326cddf1b69979597f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
@@ -1738,6 +1738,14 @@ BuildIortTable (
       goto error_handler;
     }
     TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " ItsGroupNodeCount = %d\n" \
+      " ItsGroupOffset = %d\n",
+      ItsGroupNodeCount,
+      ItsGroupOffset
+      ));
   }
 
   // Named Component Nodes
@@ -1760,6 +1768,14 @@ BuildIortTable (
       goto error_handler;
     }
     TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " NamedComponentNodeCount = %d\n" \
+      " NamedComponentOffset = %d\n",
+      NamedComponentNodeCount,
+      NamedComponentOffset
+      ));
   }
 
   // Root Complex Nodes
@@ -1782,6 +1798,14 @@ BuildIortTable (
       goto error_handler;
     }
     TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " RootComplexNodeCount = %d\n" \
+      " RootComplexOffset = %d\n",
+      RootComplexNodeCount,
+      RootComplexOffset
+      ));
   }
 
   // SMMUv1/SMMUv2 Nodes
@@ -1804,6 +1828,14 @@ BuildIortTable (
       goto error_handler;
     }
     TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " SmmuV1V2NodeCount = %d\n" \
+      " SmmuV1V2Offset = %d\n",
+      SmmuV1V2NodeCount,
+      SmmuV1V2Offset
+      ));
   }
 
   // SMMUv3 Nodes
@@ -1826,6 +1858,14 @@ BuildIortTable (
       goto error_handler;
     }
     TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " SmmuV3NodeCount = %d\n" \
+      " SmmuV3Offset = %d\n",
+      SmmuV3NodeCount,
+      SmmuV3Offset
+      ));
   }
 
   // PMCG Nodes
@@ -1848,6 +1888,14 @@ BuildIortTable (
       goto error_handler;
     }
     TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " PmcgNodeCount = %d\n" \
+      " PmcgOffset = %d\n",
+      PmcgNodeCount,
+      PmcgOffset
+      ));
   }
 
   DEBUG ((
@@ -1859,54 +1907,6 @@ BuildIortTable (
     TableSize
     ));
 
-  DEBUG ((
-    DEBUG_INFO,
-    " ItsGroupNodeCount = %d\n" \
-    " ItsGroupOffset = %d\n",
-    ItsGroupNodeCount,
-    ItsGroupOffset
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " NamedComponentNodeCount = %d\n" \
-    " NamedComponentOffset = %d\n",
-    NamedComponentNodeCount,
-    NamedComponentOffset
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " RootComplexNodeCount = %d\n" \
-    " RootComplexOffset = %d\n",
-    RootComplexNodeCount,
-    RootComplexOffset
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " SmmuV1V2NodeCount = %d\n" \
-    " SmmuV1V2Offset = %d\n",
-    SmmuV1V2NodeCount,
-    SmmuV1V2Offset
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " SmmuV3NodeCount = %d\n" \
-    " SmmuV3Offset = %d\n",
-    SmmuV3NodeCount,
-    SmmuV3Offset
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " PmcgNodeCount = %d\n" \
-    " PmcgOffset = %d\n",
-    PmcgNodeCount,
-    PmcgOffset
-    ));
-
   if (TableSize > MAX_UINT32) {
     Status = EFI_INVALID_PARAMETER;
     DEBUG ((
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 14/19] DynamicTablesPkg: PPTT: Fix uninitialized memory usage

[Sami Mujawar]

On enabling the /analyse option the VS2017 compiler
reports: warning C6001: Using uninitialized memory.

This warning is reported as some variables that were
being logged were uninitialised. To fix this, moved
the logging code after the variables being logged are
initialised.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 59 ++++++++++----------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index 82070403ac8757f54e839fd00eb4acb3292fc60c..d70fc59e754e7d348965b8c3739822a9f1c4b7e6 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -1203,6 +1203,17 @@ BuildPpttTable (
                    Generator->ProcHierarchyNodeCount,
                    &NodeIndexer
                    );
+
+    DEBUG ((
+      DEBUG_INFO,
+      " ProcHierarchyNodeCount = %d\n" \
+      " ProcHierarchyNodeOffset = 0x%x\n" \
+      " ProcHierarchyNodeIndexedList = 0x%p\n",
+      Generator->ProcHierarchyNodeCount,
+      ProcHierarchyNodeOffset,
+      Generator->ProcHierarchyNodeIndexedList
+      ));
+
   }
 
   // Include the size of Cache Type Structures and index them
@@ -1215,6 +1226,15 @@ BuildPpttTable (
                    Generator->CacheStructCount,
                    &NodeIndexer
                    );
+    DEBUG ((
+      DEBUG_INFO,
+      " CacheStructCount = %d\n" \
+      " CacheStructOffset = 0x%x\n" \
+      " CacheStructIndexedList = 0x%p\n",
+      Generator->CacheStructCount,
+      CacheStructOffset,
+      Generator->CacheStructIndexedList
+      ));
   }
 
   // Include the size of ID Type Structures and index them
@@ -1227,6 +1247,15 @@ BuildPpttTable (
                    Generator->IdStructCount,
                    &NodeIndexer
                    );
+    DEBUG ((
+      DEBUG_INFO,
+      " IdStructCount = %d\n" \
+      " IdStructOffset = 0x%x\n" \
+      " IdStructIndexedList = 0x%p\n",
+      Generator->IdStructCount,
+      IdStructOffset,
+      Generator->IdStructIndexedList
+      ));
   }
 
   DEBUG ((
@@ -1238,36 +1267,6 @@ BuildPpttTable (
     TableSize
     ));
 
-  DEBUG ((
-    DEBUG_INFO,
-    " ProcHierarchyNodeCount = %d\n" \
-    " ProcHierarchyNodeOffset = 0x%x\n" \
-    " ProcHierarchyNodeIndexedList = 0x%p\n",
-    Generator->ProcHierarchyNodeCount,
-    ProcHierarchyNodeOffset,
-    Generator->ProcHierarchyNodeIndexedList
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " CacheStructCount = %d\n" \
-    " CacheStructOffset = 0x%x\n" \
-    " CacheStructIndexedList = 0x%p\n",
-    Generator->CacheStructCount,
-    CacheStructOffset,
-    Generator->CacheStructIndexedList
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " IdStructCount = %d\n" \
-    " IdStructOffset = 0x%x\n" \
-    " IdStructIndexedList = 0x%p\n",
-    Generator->IdStructCount,
-    IdStructOffset,
-    Generator->IdStructIndexedList
-    ));
-
   // Allocate the Buffer for the PPTT table
   *Table = (EFI_ACPI_DESCRIPTION_HEADER*)AllocateZeroPool (TableSize);
   if (*Table == NULL) {
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 15/19] DynamicTablesPkg: Remove erroneous use of EFIAPI

[Sami Mujawar]

The Dynamic Tables Factory protocol has an erroneous
EFIAPI calling convention macro in the function
pointer declaration.

Remove the erroneous EFIAPI calling convention macro
from the interface declarations.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h b/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
index d07d0cac36dedb0568f6f604daa22429565aafd3..ff2331b060019976319fc2707f65252130a5326c 100644
--- a/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
+++ b/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -53,7 +53,6 @@ typedef struct DynamicTableFactoryInfo EDKII_DYNAMIC_TABLE_FACTORY_INFO;
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_ACPI_TABLE_GENERATOR) (
   IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
   IN  CONST ACPI_TABLE_GENERATOR_ID                       GeneratorId,
@@ -73,7 +72,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_ACPI_TABLE_GENERATOR) (
   IN  CONST ACPI_TABLE_GENERATOR                * CONST Generator
   );
@@ -89,7 +87,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_ACPI_TABLE_GENERATOR) (
   IN  CONST ACPI_TABLE_GENERATOR                * CONST Generator
   );
@@ -109,7 +106,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_SMBIOS_TABLE_GENERATOR) (
   IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
   IN  CONST SMBIOS_TABLE_GENERATOR_ID                     GeneratorId,
@@ -129,7 +125,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_SMBIOS_TABLE_GENERATOR) (
   IN  CONST SMBIOS_TABLE_GENERATOR              * CONST Generator
   );
@@ -145,7 +140,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_SMBIOS_TABLE_GENERATOR) (
   IN  CONST SMBIOS_TABLE_GENERATOR              * CONST Generator
   );
@@ -165,7 +159,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_DT_TABLE_GENERATOR) (
   IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
   IN  CONST DT_TABLE_GENERATOR_ID                         GeneratorId,
@@ -185,7 +178,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_DT_TABLE_GENERATOR) (
   IN  CONST DT_TABLE_GENERATOR                * CONST Generator
   );
@@ -204,7 +196,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_DT_TABLE_GENERATOR) (
   IN  CONST DT_TABLE_GENERATOR                * CONST Generator
   );
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 16/19] DynamicTablesPkg: Option for VS2017 static code analysis

[Sami Mujawar]

Add build option STATIC_ANALYSIS to enable VS2017 static
code analysis.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/DynamicTablesPkg.dsc | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/DynamicTablesPkg.dsc b/DynamicTablesPkg/DynamicTablesPkg.dsc
index dfe6c07111059cdd3eed23becf5000c04270bd9c..6ff6ff019e080f355ca24c9d05f71f8b85ab9077 100644
--- a/DynamicTablesPkg/DynamicTablesPkg.dsc
+++ b/DynamicTablesPkg/DynamicTablesPkg.dsc
@@ -4,7 +4,7 @@
 #  Copyright (c) 2019, Linaro Limited. All rights reserved.<BR>
 #  Copyright (c) 2019, ARM Limited. All rights reserved.<BR>
 #
-#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
 
@@ -39,3 +39,10 @@ [Components.common]
 
 [BuildOptions]
   *_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
+
+!ifdef STATIC_ANALYSIS
+  # Check all rules
+  # Inhibit C6305: Potential mismatch between sizeof and countof quantities.
+  *_VS2017_*_CC_FLAGS = /wd6305 /analyze
+!endif
+
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 17/19] ArmPlatformPkg: Fix UART divisor warning

[Sami Mujawar]

The VS2017 compiler reports 'warning C4244: '=': conversion
from 'UINT64' to 'UINT32', possible loss of data' for the
calculation of the UART Divisor value.

Fix this warning by adding appropriate typecast and a validation
that ensures that the UART divisor value generated does not exceed
MAX_UINT32.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
index 801990d9551a638c17d560d4226137b8a3ee47bb..2d3c279cce49304959953ec4a34b50e09a7d0045 100644
--- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
+++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
@@ -2,7 +2,7 @@
   Serial I/O Port library functions with no library constructor/destructor
 
   Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
-  Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -78,6 +78,7 @@ PL011UartInitializePort (
   UINT32      Integer;
   UINT32      Fractional;
   UINT32      HardwareFifoDepth;
+  UINT64      DivisorValue;
 
   HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
                        > PL011_VER_R1P4) \
@@ -188,7 +189,12 @@ PL011UartInitializePort (
       return RETURN_INVALID_PARAMETER;
     }
 
-    Divisor = (UartClkInHz * 4) / *BaudRate;
+    DivisorValue = (((UINT64)UartClkInHz * 4) / *BaudRate);
+    if (DivisorValue > MAX_UINT32) {
+      return RETURN_INVALID_PARAMETER;
+    }
+
+    Divisor = (UINT32)DivisorValue;
     Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
     Fractional = Divisor & FRACTION_PART_MASK;
   }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Sami Mujawar]

The VS2017 compiler reports 'warning C6326: potential
comparison of a constant with another constant' when
a fixed PCD value is compared with a constant value.

The faulting code is as marked by '-->' below:

--> if (FixedPcdGet32 (PL011UartInteger) != 0) {
      Integer = FixedPcdGet32 (PL011UartInteger);
      Fractional = FixedPcdGet32 (PL011UartFractional);
    } else {
    ...

The macro FixedPcdGet32 (PL011UartInteger) evaluates
to a macro _PCD_VALUE_PL011UartInteger that is defined
by the build system to represent the UART Integer
value. Therefore, the VS2017 compiler reports the above
warning.

Fix this warning by enclosing the code in appropriate
 #if .. #else .. #endif directives.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644
--- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
+++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
@@ -174,10 +174,10 @@ PL011UartInitializePort (
   //
 
   // If PL011 Integer value has been defined then always ignore the BAUD rate
-  if (FixedPcdGet32 (PL011UartInteger) != 0) {
+#if (FixedPcdGet32 (PL011UartInteger) != 0)
     Integer = FixedPcdGet32 (PL011UartInteger);
     Fractional = FixedPcdGet32 (PL011UartFractional);
-  } else {
+#else
     // If BAUD rate is zero then replace it with the system default value
     if (*BaudRate == 0) {
       *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);
@@ -197,7 +197,7 @@ PL011UartInitializePort (
     Divisor = (UINT32)DivisorValue;
     Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
     Fractional = Divisor & FRACTION_PART_MASK;
-  }
+#endif
 
   //
   // If PL011 is already initialized, check the current settings
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 19/19] MdePkg: Initialise VA_LIST variables before use

[Sami Mujawar]

The VS2017 compiler reports 'warning C6001: Using
uninitialized memory 'Marker'.' for VA_LIST
variables.

To fix this issue declare a VA_LIST global variable
and use this to initialise VA_LIST variables before
use.

Note: The VA_LIST cannot be assigned a NULL value
because some compilers define VA_LIST to be a
structure.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 MdePkg/Library/BaseLib/SwitchStack.c           | 9 +++++++++
 MdePkg/Library/BasePrintLib/PrintLib.c         | 5 +++++
 MdePkg/Library/BasePrintLib/PrintLibInternal.c | 9 +++++++++
 3 files changed, 23 insertions(+)

diff --git a/MdePkg/Library/BaseLib/SwitchStack.c b/MdePkg/Library/BaseLib/SwitchStack.c
index cb9f69f1eaceba690b48e9ca6b8a9af2e348bddd..e1bb524819b3de3521c5461ce681aa3a6c186f2c 100644
--- a/MdePkg/Library/BaseLib/SwitchStack.c
+++ b/MdePkg/Library/BaseLib/SwitchStack.c
@@ -2,12 +2,20 @@
   Switch Stack functions.
 
   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include "BaseLibInternals.h"
 
+//
+// Declare a VA_LIST global variable that is used to initialise VA_LIST
+// variables before use. The VA_LIST cannot be assigned a NULL value
+// because some compilers define VA_LIST to be a structure.
+//
+STATIC VA_LIST gNullVaList;
+
 /**
   Transfers control to a function starting with a new stack.
 
@@ -57,6 +65,7 @@ SwitchStack (
   //
   ASSERT (((UINTN)NewStack & (CPU_STACK_ALIGNMENT - 1)) == 0);
 
+  Marker = gNullVaList;
   VA_START (Marker, NewStack);
 
   InternalSwitchStack (EntryPoint, Context1, Context2, NewStack, Marker);
diff --git a/MdePkg/Library/BasePrintLib/PrintLib.c b/MdePkg/Library/BasePrintLib/PrintLib.c
index af771652e4b0aebd616973ba1089ae5bc2b6f0c0..67c5f3dd547cea5447075ef88d697879883ba5ab 100644
--- a/MdePkg/Library/BasePrintLib/PrintLib.c
+++ b/MdePkg/Library/BasePrintLib/PrintLib.c
@@ -3,6 +3,7 @@
 
   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
   Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -177,6 +178,7 @@ UnicodeSPrint (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;
 
+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = UnicodeVSPrint (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
@@ -337,6 +339,7 @@ UnicodeSPrintAsciiFormat (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;
 
+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = UnicodeVSPrintAsciiFormat (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
@@ -614,6 +617,7 @@ AsciiSPrint (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;
 
+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = AsciiVSPrint (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
@@ -774,6 +778,7 @@ AsciiSPrintUnicodeFormat (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;
 
+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = AsciiVSPrintUnicodeFormat (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index b6ec5ac4fbb98982f8ccaf3908c2a91ce583e31e..11392f2a5d12eb059611c3ff77b27b602f9b9a40 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -2,12 +2,20 @@
   Print Library internal worker functions.
 
   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include "PrintLibInternal.h"
 
+//
+// Declare a VA_LIST global variable that is used to initialise VA_LIST
+// variables before use. The VA_LIST cannot be assigned a NULL value
+// because some compilers define VA_LIST to be a structure.
+//
+extern VA_LIST gNullVaList;
+
 #define WARNING_STATUS_NUMBER         5
 #define ERROR_STATUS_NUMBER           33
 
@@ -1256,6 +1264,7 @@ BasePrintLibSPrint (
   VA_LIST  Marker;
   UINTN    NumberOfPrinted;
 
+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = BasePrintLibSPrintMarker (StartOfBuffer, BufferSize, Flags, FormatString, Marker, NULL);
   VA_END (Marker);
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 05/19] DynamicTablesPkg: Fix Proc node length assignment

[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 01/19] DynamicTablesPkg: Fix entry point param definition

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 01/19] DynamicTablesPkg: Fix entry point param definition

VS2017 reports 'warning C4028: formal parameter 2 different
from declaration' for the library constructor and destructor
interfaces for the Generator modules. VS2017 compiler also
reports similar warnings for the DXE entry points.

Remove the CONST qualifier for the SystemTable pointer (the
second parameter to the constructor/destructor/DXE Entry
point) to make it compatible with the formal declaration.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c | 6 +++---
 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 4 ++--
 DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c         | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c           | 8 ++++----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c         | 8 ++++----
 11 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c
index d87ea7809eae09cdb5ff06290cff8c2d7e7fa904..d1432348f05143bed8588cea54d7b9b6722b9b6d 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.c
@@ -1,7 +1,7 @@
 /** @file
   Dynamic Table Factory Dxe

-  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -60,8 +60,8 @@ EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL DynamicTableFactoryProtocol = {
 EFI_STATUS
 EFIAPI
 DynamicTableFactoryDxeInitialize (
-  IN  EFI_HANDLE                 ImageHandle,
-  IN  EFI_SYSTEM_TABLE   * CONST SystemTable
+  IN  EFI_HANDLE            ImageHandle,
+  IN  EFI_SYSTEM_TABLE   *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index 39b75996d9922d2daf953bb28809aa285f7d214c..e27dcaf37470d811c4d6218d4f40d2260f550c02 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -655,8 +655,8 @@ ProcessAcpiTables (
 EFI_STATUS
 EFIAPI
 DynamicTableManagerDxeInitialize (
-  IN CONST EFI_HANDLE                 ImageHandle,
-  IN       EFI_SYSTEM_TABLE   * CONST SystemTable
+  IN  EFI_HANDLE            ImageHandle,
+  IN  EFI_SYSTEM_TABLE   *  SystemTable
   )
 {
   EFI_STATUS                                 Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
index 0b45e15d2d1c2c1e35e801ca719d68d9ff88a65e..346ab5b22f5402bf87c385558f68f080d1b454ed 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
@@ -418,8 +418,8 @@ ACPI_TABLE_GENERATOR Dbg2Generator = {
 EFI_STATUS
 EFIAPI
 AcpiDbg2LibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -442,8 +442,8 @@ AcpiDbg2LibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiDbg2LibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
index e9e4991dc127abb97f859436df0c4ceafdb3f5ec..6cb60e898f697ce4347ad3adeb5bca1cc1f30a0c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
@@ -648,8 +648,8 @@ ACPI_TABLE_GENERATOR FadtGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiFadtLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -671,8 +671,8 @@ AcpiFadtLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiFadtLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
index a30870806d7f223fde7311e3609cca92f47731cc..7e86c30649bd36ecebac75c7e3a86a0c25cd590b 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
@@ -738,8 +738,8 @@ ACPI_TABLE_GENERATOR GtdtGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiGtdtLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -761,8 +761,8 @@ AcpiGtdtLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiGtdtLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
index 05e360c899c7563aed04c071921865929524fdc4..f303e9f29cc700634d236c08505cab91b9d62cb8 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
@@ -2040,8 +2040,8 @@ ACPI_IORT_GENERATOR IortGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiIortLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -2063,8 +2063,8 @@ AcpiIortLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiIortLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
index dc5238078376746847e7ae14258bbe35ca07ca2c..2651ca15414694c494678bcec655fc882ee2cc0c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
@@ -795,8 +795,8 @@ ACPI_TABLE_GENERATOR MadtGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiMadtLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -818,8 +818,8 @@ AcpiMadtLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiMadtLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
index d219e2f04ad6602b8157b048ae840a3094492680..1293ac8e43c8e0ed641b20ba42519172bc541b54 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
@@ -329,8 +329,8 @@ ACPI_TABLE_GENERATOR McfgGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiMcfgLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -352,8 +352,8 @@ AcpiMcfgLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiMcfgLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index c8713dec625cc18b0806659ffcdc3d9e70411e11..9e42eee9b75bb330833b0f56c98947563d9eb821 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -1492,8 +1492,8 @@ ACPI_PPTT_GENERATOR PpttGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiPpttLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -1516,8 +1516,8 @@ AcpiPpttLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiPpttLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
index 544ea06f0bcb3442b78cf6409846f70ae5360664..2facfaa048115575829c9787084876efbe60834a 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
@@ -109,8 +109,8 @@ ACPI_TABLE_GENERATOR RawGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiRawLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -132,8 +132,8 @@ AcpiRawLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiRawLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
index 7b70ba2c2c55a9a15a569680d1a57b359db4e73d..1404279f828d5c06bb7605cb2fe6d864ef7a080e 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
@@ -308,8 +308,8 @@ ACPI_TABLE_GENERATOR SpcrGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiSpcrLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -331,8 +331,8 @@ AcpiSpcrLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiSpcrLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



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

Re: [PATCH v1 19/19] MdePkg: Initialise VA_LIST variables before use

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; michael.d.kinney@intel.com <michael.d.kinney@intel.com>; liming.gao@intel.com <liming.gao@intel.com>; nd <nd@arm.com>
Subject: [PATCH v1 19/19] MdePkg: Initialise VA_LIST variables before use

The VS2017 compiler reports 'warning C6001: Using
uninitialized memory 'Marker'.' for VA_LIST
variables.

To fix this issue declare a VA_LIST global variable
and use this to initialise VA_LIST variables before
use.

Note: The VA_LIST cannot be assigned a NULL value
because some compilers define VA_LIST to be a
structure.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 MdePkg/Library/BaseLib/SwitchStack.c           | 9 +++++++++
 MdePkg/Library/BasePrintLib/PrintLib.c         | 5 +++++
 MdePkg/Library/BasePrintLib/PrintLibInternal.c | 9 +++++++++
 3 files changed, 23 insertions(+)

diff --git a/MdePkg/Library/BaseLib/SwitchStack.c b/MdePkg/Library/BaseLib/SwitchStack.c
index cb9f69f1eaceba690b48e9ca6b8a9af2e348bddd..e1bb524819b3de3521c5461ce681aa3a6c186f2c 100644
--- a/MdePkg/Library/BaseLib/SwitchStack.c
+++ b/MdePkg/Library/BaseLib/SwitchStack.c
@@ -2,12 +2,20 @@
   Switch Stack functions.

   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/

 #include "BaseLibInternals.h"

+//
+// Declare a VA_LIST global variable that is used to initialise VA_LIST
+// variables before use. The VA_LIST cannot be assigned a NULL value
+// because some compilers define VA_LIST to be a structure.
+//
+STATIC VA_LIST gNullVaList;
+
 /**
   Transfers control to a function starting with a new stack.

@@ -57,6 +65,7 @@ SwitchStack (
   //
   ASSERT (((UINTN)NewStack & (CPU_STACK_ALIGNMENT - 1)) == 0);

+  Marker = gNullVaList;
   VA_START (Marker, NewStack);

   InternalSwitchStack (EntryPoint, Context1, Context2, NewStack, Marker);
diff --git a/MdePkg/Library/BasePrintLib/PrintLib.c b/MdePkg/Library/BasePrintLib/PrintLib.c
index af771652e4b0aebd616973ba1089ae5bc2b6f0c0..67c5f3dd547cea5447075ef88d697879883ba5ab 100644
--- a/MdePkg/Library/BasePrintLib/PrintLib.c
+++ b/MdePkg/Library/BasePrintLib/PrintLib.c
@@ -3,6 +3,7 @@

   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
   Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/
@@ -177,6 +178,7 @@ UnicodeSPrint (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;

+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = UnicodeVSPrint (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
@@ -337,6 +339,7 @@ UnicodeSPrintAsciiFormat (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;

+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = UnicodeVSPrintAsciiFormat (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
@@ -614,6 +617,7 @@ AsciiSPrint (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;

+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = AsciiVSPrint (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
@@ -774,6 +778,7 @@ AsciiSPrintUnicodeFormat (
   VA_LIST Marker;
   UINTN   NumberOfPrinted;

+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = AsciiVSPrintUnicodeFormat (StartOfBuffer, BufferSize, FormatString, Marker);
   VA_END (Marker);
diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index b6ec5ac4fbb98982f8ccaf3908c2a91ce583e31e..11392f2a5d12eb059611c3ff77b27b602f9b9a40 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -2,12 +2,20 @@
   Print Library internal worker functions.

   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/

 #include "PrintLibInternal.h"

+//
+// Declare a VA_LIST global variable that is used to initialise VA_LIST
+// variables before use. The VA_LIST cannot be assigned a NULL value
+// because some compilers define VA_LIST to be a structure.
+//
+extern VA_LIST gNullVaList;
+
 #define WARNING_STATUS_NUMBER         5
 #define ERROR_STATUS_NUMBER           33

@@ -1256,6 +1264,7 @@ BasePrintLibSPrint (
   VA_LIST  Marker;
   UINTN    NumberOfPrinted;

+  Marker = gNullVaList;
   VA_START (Marker, FormatString);
   NumberOfPrinted = BasePrintLibSPrintMarker (StartOfBuffer, BufferSize, Flags, FormatString, Marker, NULL);
   VA_END (Marker);
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 17/19] ArmPlatformPkg: Fix UART divisor warning

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 17/19] ArmPlatformPkg: Fix UART divisor warning

The VS2017 compiler reports 'warning C4244: '=': conversion
from 'UINT64' to 'UINT32', possible loss of data' for the
calculation of the UART Divisor value.

Fix this warning by adding appropriate typecast and a validation
that ensures that the UART divisor value generated does not exceed
MAX_UINT32.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
index 801990d9551a638c17d560d4226137b8a3ee47bb..2d3c279cce49304959953ec4a34b50e09a7d0045 100644
--- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
+++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
@@ -2,7 +2,7 @@
   Serial I/O Port library functions with no library constructor/destructor

   Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
-  Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -78,6 +78,7 @@ PL011UartInitializePort (
   UINT32      Integer;
   UINT32      Fractional;
   UINT32      HardwareFifoDepth;
+  UINT64      DivisorValue;

   HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
                        > PL011_VER_R1P4) \
@@ -188,7 +189,12 @@ PL011UartInitializePort (
       return RETURN_INVALID_PARAMETER;
     }

-    Divisor = (UartClkInHz * 4) / *BaudRate;
+    DivisorValue = (((UINT64)UartClkInHz * 4) / *BaudRate);
+    if (DivisorValue > MAX_UINT32) {
+      return RETURN_INVALID_PARAMETER;
+    }
+
+    Divisor = (UINT32)DivisorValue;
     Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
     Fractional = Divisor & FRACTION_PART_MASK;
   }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

The VS2017 compiler reports 'warning C6326: potential
comparison of a constant with another constant' when
a fixed PCD value is compared with a constant value.

The faulting code is as marked by '-->' below:

--> if (FixedPcdGet32 (PL011UartInteger) != 0) {
      Integer = FixedPcdGet32 (PL011UartInteger);
      Fractional = FixedPcdGet32 (PL011UartFractional);
    } else {
    ...

The macro FixedPcdGet32 (PL011UartInteger) evaluates
to a macro _PCD_VALUE_PL011UartInteger that is defined
by the build system to represent the UART Integer
value. Therefore, the VS2017 compiler reports the above
warning.

Fix this warning by enclosing the code in appropriate
 #if .. #else .. #endif directives.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644
--- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
+++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
@@ -174,10 +174,10 @@ PL011UartInitializePort (
   //

   // If PL011 Integer value has been defined then always ignore the BAUD rate
-  if (FixedPcdGet32 (PL011UartInteger) != 0) {
+#if (FixedPcdGet32 (PL011UartInteger) != 0)
     Integer = FixedPcdGet32 (PL011UartInteger);
     Fractional = FixedPcdGet32 (PL011UartFractional);
-  } else {
+#else
     // If BAUD rate is zero then replace it with the system default value
     if (*BaudRate == 0) {
       *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);
@@ -197,7 +197,7 @@ PL011UartInitializePort (
     Divisor = (UINT32)DivisorValue;
     Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
     Fractional = Divisor & FRACTION_PART_MASK;
-  }
+#endif

   //
   // If PL011 is already initialized, check the current settings
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 14/19] DynamicTablesPkg: PPTT: Fix uninitialized memory usage

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 14/19] DynamicTablesPkg: PPTT: Fix uninitialized memory usage

On enabling the /analyse option the VS2017 compiler
reports: warning C6001: Using uninitialized memory.

This warning is reported as some variables that were
being logged were uninitialised. To fix this, moved
the logging code after the variables being logged are
initialised.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 59 ++++++++++----------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index 82070403ac8757f54e839fd00eb4acb3292fc60c..d70fc59e754e7d348965b8c3739822a9f1c4b7e6 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -1203,6 +1203,17 @@ BuildPpttTable (
                    Generator->ProcHierarchyNodeCount,
                    &NodeIndexer
                    );
+
+    DEBUG ((
+      DEBUG_INFO,
+      " ProcHierarchyNodeCount = %d\n" \
+      " ProcHierarchyNodeOffset = 0x%x\n" \
+      " ProcHierarchyNodeIndexedList = 0x%p\n",
+      Generator->ProcHierarchyNodeCount,
+      ProcHierarchyNodeOffset,
+      Generator->ProcHierarchyNodeIndexedList
+      ));
+
   }

   // Include the size of Cache Type Structures and index them
@@ -1215,6 +1226,15 @@ BuildPpttTable (
                    Generator->CacheStructCount,
                    &NodeIndexer
                    );
+    DEBUG ((
+      DEBUG_INFO,
+      " CacheStructCount = %d\n" \
+      " CacheStructOffset = 0x%x\n" \
+      " CacheStructIndexedList = 0x%p\n",
+      Generator->CacheStructCount,
+      CacheStructOffset,
+      Generator->CacheStructIndexedList
+      ));
   }

   // Include the size of ID Type Structures and index them
@@ -1227,6 +1247,15 @@ BuildPpttTable (
                    Generator->IdStructCount,
                    &NodeIndexer
                    );
+    DEBUG ((
+      DEBUG_INFO,
+      " IdStructCount = %d\n" \
+      " IdStructOffset = 0x%x\n" \
+      " IdStructIndexedList = 0x%p\n",
+      Generator->IdStructCount,
+      IdStructOffset,
+      Generator->IdStructIndexedList
+      ));
   }

   DEBUG ((
@@ -1238,36 +1267,6 @@ BuildPpttTable (
     TableSize
     ));

-  DEBUG ((
-    DEBUG_INFO,
-    " ProcHierarchyNodeCount = %d\n" \
-    " ProcHierarchyNodeOffset = 0x%x\n" \
-    " ProcHierarchyNodeIndexedList = 0x%p\n",
-    Generator->ProcHierarchyNodeCount,
-    ProcHierarchyNodeOffset,
-    Generator->ProcHierarchyNodeIndexedList
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " CacheStructCount = %d\n" \
-    " CacheStructOffset = 0x%x\n" \
-    " CacheStructIndexedList = 0x%p\n",
-    Generator->CacheStructCount,
-    CacheStructOffset,
-    Generator->CacheStructIndexedList
-    ));
-
-  DEBUG ((
-    DEBUG_INFO,
-    " IdStructCount = %d\n" \
-    " IdStructOffset = 0x%x\n" \
-    " IdStructIndexedList = 0x%p\n",
-    Generator->IdStructCount,
-    IdStructOffset,
-    Generator->IdStructIndexedList
-    ));
-
   // Allocate the Buffer for the PPTT table
   *Table = (EFI_ACPI_DESCRIPTION_HEADER*)AllocateZeroPool (TableSize);
   if (*Table == NULL) {
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 16/19] DynamicTablesPkg: Option for VS2017 static code analysis

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 16/19] DynamicTablesPkg: Option for VS2017 static code analysis

Add build option STATIC_ANALYSIS to enable VS2017 static
code analysis.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/DynamicTablesPkg.dsc | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/DynamicTablesPkg.dsc b/DynamicTablesPkg/DynamicTablesPkg.dsc
index dfe6c07111059cdd3eed23becf5000c04270bd9c..6ff6ff019e080f355ca24c9d05f71f8b85ab9077 100644
--- a/DynamicTablesPkg/DynamicTablesPkg.dsc
+++ b/DynamicTablesPkg/DynamicTablesPkg.dsc
@@ -4,7 +4,7 @@
 #  Copyright (c) 2019, Linaro Limited. All rights reserved.<BR>
 #  Copyright (c) 2019, ARM Limited. All rights reserved.<BR>
 #
-#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##

@@ -39,3 +39,10 @@ [Components.common]

 [BuildOptions]
   *_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
+
+!ifdef STATIC_ANALYSIS
+  # Check all rules
+  # Inhibit C6305: Potential mismatch between sizeof and countof quantities.
+  *_VS2017_*_CC_FLAGS = /wd6305 /analyze
+!endif
+
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 15/19] DynamicTablesPkg: Remove erroneous use of EFIAPI

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 15/19] DynamicTablesPkg: Remove erroneous use of EFIAPI

The Dynamic Tables Factory protocol has an erroneous
EFIAPI calling convention macro in the function
pointer declaration.

Remove the erroneous EFIAPI calling convention macro
from the interface declarations.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h b/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
index d07d0cac36dedb0568f6f604daa22429565aafd3..ff2331b060019976319fc2707f65252130a5326c 100644
--- a/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
+++ b/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
@@ -1,6 +1,6 @@
 /** @file

-  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -53,7 +53,6 @@ typedef struct DynamicTableFactoryInfo EDKII_DYNAMIC_TABLE_FACTORY_INFO;
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_ACPI_TABLE_GENERATOR) (
   IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
   IN  CONST ACPI_TABLE_GENERATOR_ID                       GeneratorId,
@@ -73,7 +72,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_ACPI_TABLE_GENERATOR) (
   IN  CONST ACPI_TABLE_GENERATOR                * CONST Generator
   );
@@ -89,7 +87,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_ACPI_TABLE_GENERATOR) (
   IN  CONST ACPI_TABLE_GENERATOR                * CONST Generator
   );
@@ -109,7 +106,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_SMBIOS_TABLE_GENERATOR) (
   IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
   IN  CONST SMBIOS_TABLE_GENERATOR_ID                     GeneratorId,
@@ -129,7 +125,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_SMBIOS_TABLE_GENERATOR) (
   IN  CONST SMBIOS_TABLE_GENERATOR              * CONST Generator
   );
@@ -145,7 +140,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_SMBIOS_TABLE_GENERATOR) (
   IN  CONST SMBIOS_TABLE_GENERATOR              * CONST Generator
   );
@@ -165,7 +159,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_DT_TABLE_GENERATOR) (
   IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
   IN  CONST DT_TABLE_GENERATOR_ID                         GeneratorId,
@@ -185,7 +178,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_DT_TABLE_GENERATOR) (
   IN  CONST DT_TABLE_GENERATOR                * CONST Generator
   );
@@ -204,7 +196,6 @@ EFIAPI
 **/
 typedef
 EFI_STATUS
-EFIAPI
 (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_DT_TABLE_GENERATOR) (
   IN  CONST DT_TABLE_GENERATOR                * CONST Generator
   );
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [edk2-devel] [PATCH v1 03/19] DynamicTablesPkg: Remove struct CM_ARM_CPU_INFO

[Alexei Fedorov]

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

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


Alexei

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Sami Mujawar via Groups.Io <sami.mujawar=arm.com@groups.io>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [edk2-devel] [PATCH v1 03/19] DynamicTablesPkg: Remove struct CM_ARM_CPU_INFO

The VS2017 compiler reports 'error C2016: C requires that
a struct or union has at least one member' for the struct
CM_ARM_CPU_INFO.

Remove struct CM_ARM_CPU_INFO as this is not in use.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
index ac451b306dfd7ba299a83209675b21696be235be..f08b8257678c8b6aacc863bbe1db09587b51a291 100644
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
@@ -67,10 +67,6 @@ typedef struct CmArmBootArchInfo {
   UINT32  BootArchFlags;
 } CM_ARM_BOOT_ARCH_INFO;

-typedef struct CmArmCpuInfo {
-  // Reserved for use when SMBIOS tables are implemented
-} CM_ARM_CPU_INFO;
-
 /** A structure that describes the
     Power Management Profile Information for the Platform.

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





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [edk2-devel] [PATCH v1 02/19] DynamicTablesPkg: Fix missing local header warning

[Alexei Fedorov]

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

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


Alexei

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Sami Mujawar via Groups.Io <sami.mujawar=arm.com@groups.io>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [edk2-devel] [PATCH v1 02/19] DynamicTablesPkg: Fix missing local header warning

The edk2 BaseTools report a warning if a local header file
is not listed under the [Sources] section in the INF file.

Add header files to the [Sources] section in the respective
INF files to fix the warnings.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf | 1 +
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf        | 3 ++-
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf        | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
index b47a690dd72702a2d7157cbb21249ba1d01d08cd..56b69591ac52ca3ba51250dc01a0846b22442ce9 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
@@ -25,6 +25,7 @@ [Sources]
   DeviceTreeTableFactory/DeviceTreeTableFactory.c
   DynamicTableFactoryDxe.c
   SmbiosTableFactory/SmbiosTableFactory.c
+  DynamicTableFactory.h

 [Packages]
   MdePkg/MdePkg.dec
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
index 3bbd0e7818c6172f9178b12a3d0960e2d298bca1..2f987e3b6d339d4b5f342cb52b8dc712cf4e585c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
@@ -1,7 +1,7 @@
 ## @file
 #  IORT Table Generator
 #
-#  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+#  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 ##
@@ -18,6 +18,7 @@ [Defines]

 [Sources]
   IortGenerator.c
+  IortGenerator.h

 [Packages]
   MdePkg/MdePkg.dec
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
index 3cb13d7d8fd3df04f3e16cc6161d9736f7579a80..2c7d19513d6ceea29fd9f271f983a6cf6d2c1046 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
@@ -18,6 +18,7 @@ [Defines]

 [Sources]
   PpttGenerator.c
+  PpttGenerator.h

 [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 12/19] DynamicTablesPkg: Fix IORT node length assignment

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 12/19] DynamicTablesPkg: Fix IORT node length assignment

The VS2017 compiler reports 'warning C4267: 'return': conversion
from 'size_t' to 'UINT32', possible loss of data' for a number of
functions that compute the IORT node length. Similarly, it reports
warnings for IORT node length field assignments as the length
field is 16-bit wide.

This patch adds type casts at appropriate places and also implements
validations to ensure that the max width of the respective fields
is not exceeded.

This patch also fixes a typo in one of the local variable names.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c | 352 ++++++++++++++------
 1 file changed, 253 insertions(+), 99 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
index f303e9f29cc700634d236c08505cab91b9d62cb8..bd71220ca19679792de2bb6d88eed8d1913a7600 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
@@ -140,8 +140,8 @@ GetItsGroupNodeSize (
   /* Size of ITS Group Node +
      Size of ITS Identifier array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE) +
-           (Node->ItsIdCount * sizeof (UINT32));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE) +
+                    (Node->ItsIdCount * sizeof (UINT32)));
 }

 /** Returns the total size required for the ITS Group nodes and
@@ -160,7 +160,7 @@ GetItsGroupNodeSize (
     @retval Total size of the ITS Group Nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofItsGroupNodes (
   IN      CONST UINT32                         NodeStartOffset,
   IN      CONST CM_ARM_ITS_GROUP_NODE  *       NodeList,
@@ -168,7 +168,7 @@ GetSizeofItsGroupNodes (
   IN OUT        IORT_NODE_INDEXER     ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;

   ASSERT (NodeList != NULL);

@@ -176,7 +176,7 @@ GetSizeofItsGroupNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -211,10 +211,10 @@ GetNamedComponentNodeSize (
      Size of ID mapping array +
      Size of ASCII string + 'padding to 32-bit word aligned'.
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
-            (Node->IdMappingCount *
-             sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
-            ALIGN_VALUE (AsciiStrSize (Node->ObjectName), 4);
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
+                    ALIGN_VALUE (AsciiStrSize (Node->ObjectName), 4));
 }

 /** Returns the total size required for the Named Component nodes and
@@ -233,7 +233,7 @@ GetNamedComponentNodeSize (
     @retval Total size of the Named Component nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofNamedComponentNodes (
   IN      CONST UINT32                              NodeStartOffset,
   IN      CONST CM_ARM_NAMED_COMPONENT_NODE *       NodeList,
@@ -241,7 +241,7 @@ GetSizeofNamedComponentNodes (
   IN OUT        IORT_NODE_INDEXER          ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;

   ASSERT (NodeList != NULL);

@@ -249,7 +249,7 @@ GetSizeofNamedComponentNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -284,9 +284,9 @@ GetRootComplexNodeSize (
   /* Size of Root Complex node +
      Size of ID mapping array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_RC_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_RC_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)));
 }

 /** Returns the total size required for the Root Complex nodes and
@@ -305,7 +305,7 @@ GetRootComplexNodeSize (
     @retval Total size of the Root Complex nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofRootComplexNodes (
   IN      CONST UINT32                              NodeStartOffset,
   IN      CONST CM_ARM_ROOT_COMPLEX_NODE    *       NodeList,
@@ -313,7 +313,7 @@ GetSizeofRootComplexNodes (
   IN OUT        IORT_NODE_INDEXER          ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;

   ASSERT (NodeList != NULL);

@@ -321,7 +321,7 @@ GetSizeofRootComplexNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -358,13 +358,13 @@ GetSmmuV1V2NodeSize (
      Size of context interrupt array +
      Size of PMU interrupt array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
-           (Node->ContextInterruptCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT)) +
-           (Node->PmuInterruptCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)) +
+                    (Node->ContextInterruptCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT)) +
+                    (Node->PmuInterruptCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT)));
 }

 /** Returns the total size required for the SMMUv1/SMMUv2 nodes and
@@ -383,7 +383,7 @@ GetSmmuV1V2NodeSize (
     @retval Total size of the SMMUv1/SMMUv2 nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofSmmuV1V2Nodes (
   IN      CONST UINT32                              NodeStartOffset,
   IN      CONST CM_ARM_SMMUV1_SMMUV2_NODE   *       NodeList,
@@ -391,7 +391,7 @@ GetSizeofSmmuV1V2Nodes (
   IN OUT        IORT_NODE_INDEXER          ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;

   ASSERT (NodeList != NULL);

@@ -399,7 +399,7 @@ GetSizeofSmmuV1V2Nodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -433,9 +433,9 @@ GetSmmuV3NodeSize (
   /* Size of SMMU v1/SMMU v2 node +
      Size of ID mapping array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)));
 }

 /** Returns the total size required for the SMMUv3 nodes and
@@ -454,7 +454,7 @@ GetSmmuV3NodeSize (
     @retval Total size of the SMMUv3 nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofSmmuV3Nodes (
   IN      CONST UINT32                       NodeStartOffset,
   IN      CONST CM_ARM_SMMUV3_NODE   *       NodeList,
@@ -462,7 +462,7 @@ GetSizeofSmmuV3Nodes (
   IN OUT        IORT_NODE_INDEXER   ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;

   ASSERT (NodeList != NULL);

@@ -470,7 +470,7 @@ GetSizeofSmmuV3Nodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -504,9 +504,9 @@ GetPmcgNodeSize (
   /* Size of PMCG node +
      Size of ID mapping array
   */
-  return sizeof (EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE) +
-           (Node->IdMappingCount *
-            sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE));
+  return (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE) +
+                    (Node->IdMappingCount *
+                     sizeof (EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE)));
 }

 /** Returns the total size required for the PMCG nodes and
@@ -525,7 +525,7 @@ GetPmcgNodeSize (
     @retval Total size of the PMCG nodes.
 **/
 STATIC
-UINT32
+UINT64
 GetSizeofPmcgNodes (
   IN      CONST UINT32                     NodeStartOffset,
   IN      CONST CM_ARM_PMCG_NODE   *       NodeList,
@@ -533,7 +533,7 @@ GetSizeofPmcgNodes (
   IN OUT        IORT_NODE_INDEXER ** CONST NodeIndexer
   )
 {
-  UINT32  Size;
+  UINT64  Size;

   ASSERT (NodeList != NULL);

@@ -541,7 +541,7 @@ GetSizeofPmcgNodes (
   while (NodeCount-- != 0) {
     (*NodeIndexer)->Token = NodeList->Token;
     (*NodeIndexer)->Object = (VOID*)NodeList;
-    (*NodeIndexer)->Offset = Size + NodeStartOffset;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
     DEBUG ((
       DEBUG_INFO,
       "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
@@ -735,6 +735,7 @@ AddItsGroupNodes (
   CM_ARM_ITS_IDENTIFIER               * ItsIdentifier;
   UINT32                                ItsIdentifierCount;
   UINT32                                IdIndex;
+  UINT64                                NodeLength;

   ASSERT (Iort != NULL);

@@ -742,9 +743,22 @@ AddItsGroupNodes (
                   NodesStartOffset);

   while (NodeCount-- != 0) {
+    NodeLength = GetItsGroupNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: ITS Id Array Node length 0x%lx > MAX_UINT16."
+        " Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     ItsGroupNode->Node.Type = EFI_ACPI_IORT_TYPE_ITS_GROUP;
-    ItsGroupNode->Node.Length = GetItsGroupNodeSize (NodeList);
+    ItsGroupNode->Node.Length = (UINT16)NodeLength;
     ItsGroupNode->Node.Revision = 0;
     ItsGroupNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     ItsGroupNode->Node.NumIdMappings = 0;
@@ -825,8 +839,9 @@ AddNamedComponentNodes (
   EFI_STATUS                                   Status;
   EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE  * NcNode;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE         * IdMapArray;
-  UINT32                                       ObjectNameLenght;
   CHAR8                                      * ObjectName;
+  UINTN                                        ObjectNameLength;
+  UINT64                                       NodeLength;

   ASSERT (Iort != NULL);

@@ -834,18 +849,30 @@ AddNamedComponentNodes (
             NodesStartOffset);

   while (NodeCount-- != 0) {
+    NodeLength = GetNamedComponentNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Named Component Node length 0x%lx > MAX_UINT16."
+        " Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     NcNode->Node.Type = EFI_ACPI_IORT_TYPE_NAMED_COMP;
-    NcNode->Node.Length =
-      GetNamedComponentNodeSize (NodeList);
+    NcNode->Node.Length = (UINT16)NodeLength;
     NcNode->Node.Revision = 2;
     NcNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     NcNode->Node.NumIdMappings = NodeList->IdMappingCount;

-    ObjectNameLenght = AsciiStrLen (NodeList->ObjectName) + 1;
+    ObjectNameLength = AsciiStrLen (NodeList->ObjectName) + 1;
     NcNode->Node.IdReference =
-      sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
-      (ALIGN_VALUE (ObjectNameLenght, 4));
+      (UINT32)(sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE) +
+        (ALIGN_VALUE (ObjectNameLength, 4)));

     // Named Component specific data
     NcNode->Flags = NodeList->Flags;
@@ -860,7 +887,7 @@ AddNamedComponentNodes (
       sizeof (EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE));
     Status = AsciiStrCpyS (
                ObjectName,
-               ObjectNameLenght,
+               ObjectNameLength,
                NodeList->ObjectName
                );
     if (EFI_ERROR (Status)) {
@@ -936,6 +963,7 @@ AddRootComplexNodes (
   EFI_STATUS                           Status;
   EFI_ACPI_6_0_IO_REMAPPING_RC_NODE  * RcNode;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE * IdMapArray;
+  UINT64                               NodeLength;

   ASSERT (Iort != NULL);

@@ -943,9 +971,22 @@ AddRootComplexNodes (
             NodesStartOffset);

   while (NodeCount-- != 0) {
+    NodeLength = GetRootComplexNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Root Complex Node length 0x%lx > MAX_UINT16."
+        " Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     RcNode->Node.Type = EFI_ACPI_IORT_TYPE_ROOT_COMPLEX;
-    RcNode->Node.Length = GetRootComplexNodeSize (NodeList);
+    RcNode->Node.Length = (UINT16)NodeLength;
     RcNode->Node.Revision = 1;
     RcNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     RcNode->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1093,6 +1134,7 @@ AddSmmuV1V2Nodes (

   EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT  * ContextInterruptArray;
   EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT  * PmuInterruptArray;
+  UINT64                                NodeLength;

   ASSERT (Iort != NULL);

@@ -1100,9 +1142,21 @@ AddSmmuV1V2Nodes (
               NodesStartOffset);

   while (NodeCount-- != 0) {
+    NodeLength = GetSmmuV1V2NodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: SMMU V1/V2 Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     SmmuNode->Node.Type = EFI_ACPI_IORT_TYPE_SMMUv1v2;
-    SmmuNode->Node.Length = GetSmmuV1V2NodeSize (NodeList);
+    SmmuNode->Node.Length = (UINT16)NodeLength;
     SmmuNode->Node.Revision = 0;
     SmmuNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     SmmuNode->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1239,6 +1293,7 @@ AddSmmuV3Nodes (
   EFI_STATUS                             Status;
   EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE * SmmuV3Node;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE   * IdMapArray;
+  UINT64                                 NodeLength;

   ASSERT (Iort != NULL);

@@ -1246,9 +1301,21 @@ AddSmmuV3Nodes (
                 NodesStartOffset);

   while (NodeCount-- != 0) {
+    NodeLength = GetSmmuV3NodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: SMMU V3 Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     SmmuV3Node->Node.Type = EFI_ACPI_IORT_TYPE_SMMUv3;
-    SmmuV3Node->Node.Length = GetSmmuV3NodeSize (NodeList);
+    SmmuV3Node->Node.Length = (UINT16)NodeLength;
     SmmuV3Node->Node.Revision = 2;
     SmmuV3Node->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     SmmuV3Node->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1344,6 +1411,7 @@ AddPmcgNodes (
   EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE  * PmcgNode;
   EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE   * IdMapArray;
   ACPI_IORT_GENERATOR                  * Generator;
+  UINT64                                 NodeLength;

   ASSERT (Iort != NULL);

@@ -1352,9 +1420,21 @@ AddPmcgNodes (
               NodesStartOffset);

   while (NodeCount-- != 0) {
+    NodeLength = GetPmcgNodeSize (NodeList);
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: PMCG Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
     // Populate the node header
     PmcgNode->Node.Type = EFI_ACPI_IORT_TYPE_PMCG;
-    PmcgNode->Node.Length = GetPmcgNodeSize (NodeList);
+    PmcgNode->Node.Length = (UINT16)NodeLength;
     PmcgNode->Node.Revision = 1;
     PmcgNode->Node.Reserved = EFI_ACPI_RESERVED_DWORD;
     PmcgNode->Node.NumIdMappings = NodeList->IdMappingCount;
@@ -1448,9 +1528,11 @@ BuildIortTable (
   )
 {
   EFI_STATUS                             Status;
-  UINT32                                 TableSize;
+
+  UINT64                                 TableSize;
+  UINT64                                 NodeSize;
+
   UINT32                                 IortNodeCount;
-
   UINT32                                 ItsGroupNodeCount;
   UINT32                                 NamedComponentNodeCount;
   UINT32                                 RootComplexNodeCount;
@@ -1638,81 +1720,141 @@ BuildIortTable (

   // ITS Group Nodes
   if (ItsGroupNodeCount > 0) {
-    ItsGroupOffset = TableSize;
+    ItsGroupOffset = (UINT32)TableSize;
     // Size of ITS Group node list.
-    TableSize += GetSizeofItsGroupNodes (
-                   ItsGroupOffset,
-                   ItsGroupNodeList,
-                   ItsGroupNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofItsGroupNodes (
+                 ItsGroupOffset,
+                 ItsGroupNodeList,
+                 ItsGroupNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of Group Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }

   // Named Component Nodes
   if (NamedComponentNodeCount > 0) {
-    NamedComponentOffset = TableSize;
+    NamedComponentOffset = (UINT32)TableSize;
     // Size of Named Component node list.
-    TableSize += GetSizeofNamedComponentNodes (
-                   NamedComponentOffset,
-                   NamedComponentNodeList,
-                   NamedComponentNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofNamedComponentNodes (
+                 NamedComponentOffset,
+                 NamedComponentNodeList,
+                 NamedComponentNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of Named Component Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }

   // Root Complex Nodes
   if (RootComplexNodeCount > 0) {
-    RootComplexOffset = TableSize;
+    RootComplexOffset = (UINT32)TableSize;
     // Size of Root Complex node list.
-    TableSize += GetSizeofRootComplexNodes (
-                   RootComplexOffset,
-                   RootComplexNodeList,
-                   RootComplexNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofRootComplexNodes (
+                 RootComplexOffset,
+                 RootComplexNodeList,
+                 RootComplexNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of Root Complex Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }

   // SMMUv1/SMMUv2 Nodes
   if (SmmuV1V2NodeCount > 0) {
-    SmmuV1V2Offset = TableSize;
+    SmmuV1V2Offset = (UINT32)TableSize;
     // Size of SMMUv1/SMMUv2 node list.
-    TableSize += GetSizeofSmmuV1V2Nodes (
-                   SmmuV1V2Offset,
-                   SmmuV1V2NodeList,
-                   SmmuV1V2NodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofSmmuV1V2Nodes (
+                 SmmuV1V2Offset,
+                 SmmuV1V2NodeList,
+                 SmmuV1V2NodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of SMMUv1/v2 Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }

   // SMMUv3 Nodes
   if (SmmuV3NodeCount > 0) {
-    SmmuV3Offset = TableSize;
+    SmmuV3Offset = (UINT32)TableSize;
     // Size of SMMUv3 node list.
-    TableSize += GetSizeofSmmuV3Nodes (
-                   SmmuV3Offset,
-                   SmmuV3NodeList,
-                   SmmuV3NodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofSmmuV3Nodes (
+                 SmmuV3Offset,
+                 SmmuV3NodeList,
+                 SmmuV3NodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of SMMUv3 Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }

   // PMCG Nodes
   if (PmcgNodeCount > 0) {
-    PmcgOffset = TableSize;
+    PmcgOffset = (UINT32)TableSize;
     // Size of PMCG node list.
-    TableSize += GetSizeofPmcgNodes (
-                   PmcgOffset,
-                   PmcgNodeList,
-                   PmcgNodeCount,
-                   &NodeIndexer
-                   );
+    NodeSize = GetSizeofPmcgNodes (
+                 PmcgOffset,
+                 PmcgNodeList,
+                 PmcgNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: IORT: Invalid Size of PMCG Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+    TableSize += NodeSize;
   }

   DEBUG ((
     DEBUG_INFO,
     "INFO: IORT:\n" \
     " IortNodeCount = %d\n" \
-    " TableSize = %d\n",
+    " TableSize = 0x%lx\n",
     IortNodeCount,
     TableSize
     ));
@@ -1765,6 +1907,18 @@ BuildIortTable (
     PmcgOffset
     ));

+  if (TableSize > MAX_UINT32) {
+    Status = EFI_INVALID_PARAMETER;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: IORT: IORT Table Size 0x%lx > MAX_UINT32," \
+      " Status = %r\n",
+      TableSize,
+      Status
+      ));
+    goto error_handler;
+  }
+
   // Allocate the Buffer for IORT table
   *Table = (EFI_ACPI_DESCRIPTION_HEADER*)AllocateZeroPool (TableSize);
   if (*Table == NULL) {
@@ -1783,7 +1937,7 @@ BuildIortTable (

   DEBUG ((
     DEBUG_INFO,
-    "IORT: Iort = 0x%p TableSize = 0x%x\n",
+    "IORT: Iort = 0x%p TableSize = 0x%lx\n",
     Iort,
     TableSize
     ));
@@ -1793,7 +1947,7 @@ BuildIortTable (
              This,
              &Iort->Header,
              AcpiTableInfo,
-             TableSize
+             (UINT32)TableSize
              );
   if (EFI_ERROR (Status)) {
     DEBUG ((
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 11/19] DynamicTablesPkg: Remove redundant frame count check

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 11/19] DynamicTablesPkg: Remove redundant frame count check

Removing GT Block frame count check from AddGTBlockTimerFrames()
as this is already validated in BuildGtdtTable().

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
index 0e996698887aefca8a7240fc27fe3cb9324fd3e2..c109f2ac207311646e5a4ad3aa465b1c2883e465 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
@@ -259,16 +259,6 @@ AddGTBlockTimerFrames (
   ASSERT (GtBlockFrame != NULL);
   ASSERT (GTBlockTimerFrameList != NULL);

-  if (GTBlockFrameCount > 8) {
-    DEBUG ((
-      DEBUG_ERROR,
-      "ERROR: GTDT: GT Block Frame Count %d is greater than 8\n",
-      GTBlockFrameCount
-      ));
-    ASSERT (GTBlockFrameCount <= 8);
-    return EFI_INVALID_PARAMETER;
-  }
-
   IsFrameNumberDuplicated = FindDuplicateValue (
                               GTBlockTimerFrameList,
                               GTBlockFrameCount,
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [edk2-devel] [PATCH v1 06/19] DynamicTablesPkg: Fix GT Block length assignment

[Alexei Fedorov]

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

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


Alexei

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Sami Mujawar via Groups.Io <sami.mujawar=arm.com@groups.io>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [edk2-devel] [PATCH v1 06/19] DynamicTablesPkg: Fix GT Block length assignment

The VS2017 compiler reports 'warning C4267: '=': conversion from
'size_t' to 'UINT16', possible loss of data'.

The sizeof() operator is used to calculate the size of the
GT Block structure. The length field in the GT Block structure
is 16-bit wide. Since the return type of sizeof() operator
is size_t the VS2017 compiler reports the above warning.

To fix the warning, an explicit type cast is added. An additional
check is also performed to ensure that the calculated GT Block
length does not exceed MAX_UINT16.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c | 25 ++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
index 7e86c30649bd36ecebac75c7e3a86a0c25cd590b..0e996698887aefca8a7240fc27fe3cb9324fd3e2 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
@@ -350,6 +350,7 @@ AddGTBlockList (
   EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE  * GtBlockFrame;
   CM_ARM_GTBLOCK_TIMER_FRAME_INFO             * GTBlockTimerFrameList;
   UINT32                                        GTBlockTimerFrameCount;
+  UINTN                                         Length;

   ASSERT (Gtdt != NULL);
   ASSERT (GTBlockInfo != NULL);
@@ -376,11 +377,27 @@ AddGTBlockList (
       return Status;
     }

+    Length = sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_STRUCTURE) +
+               (sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE) *
+                GTBlockInfo->GTBlockTimerFrameCount);
+
+    // Check that the length of the GT block does not
+    // exceed MAX_UINT16
+    if (Length > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: GTDT: Too many GT Frames. Count = %d. " \
+        "Maximum supported GT Block size exceeded. " \
+        "Status = %r\n",
+        GTBlockInfo->GTBlockTimerFrameCount,
+        Status
+        ));
+      return Status;
+    }
+
     GTBlock->Type = EFI_ACPI_6_3_GTDT_GT_BLOCK;
-    GTBlock->Length = sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_STRUCTURE) +
-                        (sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE) *
-                          GTBlockInfo->GTBlockTimerFrameCount);
-
+    GTBlock->Length = (UINT16)Length;
     GTBlock->Reserved = EFI_ACPI_RESERVED_BYTE;
     GTBlock->CntCtlBase = GTBlockInfo->GTBlockPhysicalAddress;
     GTBlock->GTBlockTimerCount = GTBlockInfo->GTBlockTimerFrameCount;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 08/19] DynamicTablesPkg: Fix ACPI table rev field width

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 08/19] DynamicTablesPkg: Fix ACPI table rev field width

The VS2017 compiler reports 'warning C4244: '=': conversion from
'const UINT32' to 'UINT8', possible loss of data' when the ACPI
table revision field is being updated.

The width of the revision field in the EFI_ACPI_DESCRIPTION_HEADER
struct is 8-bit wide. Therefore, to fix the above warning make the
ACPI Table revision field usage 8-bit wide across Dynamic Tables
Framework.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Include/AcpiTableGenerator.h       | 4 ++--
 DynamicTablesPkg/Include/StandardNameSpaceObjects.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h
index 7d6d3442276db7b4abaeb3b053ba489258adea0b..d7a1209c4d40c2e6ffb3dac786c276a30d70b6df 100644
--- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
+++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
@@ -284,10 +284,10 @@ typedef struct AcpiTableGenerator {
   UINT32                                 AcpiTableSignature;

   /// The ACPI table revision.
-  UINT32                                 AcpiTableRevision;
+  UINT8                                  AcpiTableRevision;

   /// The minimum supported ACPI table revision.
-  UINT32                                 MinAcpiTableRevision;
+  UINT8                                  MinAcpiTableRevision;

   /// The ACPI table creator ID.
   UINT32                                 CreatorId;
diff --git a/DynamicTablesPkg/Include/StandardNameSpaceObjects.h b/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
index 21bb7de4044ffd6f97156f761e30fc1790f11c0c..0ba6b163691ea127ae7a7d57fb5f9fdf84789338 100644
--- a/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
@@ -79,7 +79,7 @@ typedef struct CmAStdObjAcpiTableInfo {
   UINT32                         AcpiTableSignature;

   /// The ACPI table revision
-  UINT32                         AcpiTableRevision;
+  UINT8                          AcpiTableRevision;

   /// The ACPI Table Generator ID
   ACPI_TABLE_GENERATOR_ID        TableGeneratorId;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [edk2-devel] [PATCH v1 04/19] DynamicTablesPkg: Fix serial port subtype warning

[Alexei Fedorov]

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

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


Alexei

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Sami Mujawar via Groups.Io <sami.mujawar=arm.com@groups.io>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; ard.biesheuvel@linaro.org <ard.biesheuvel@linaro.org>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [edk2-devel] [PATCH v1 04/19] DynamicTablesPkg: Fix serial port subtype warning

The VS2017 compiler reports 'warning C4244: '=': conversion
from 'UINT16' to 'UINT8', possible loss of data' for the
SPCR InterfaceType field assignment.

The SPCR InterfaceType field uses the same encoding as that
of the DBG2 table Port Subtype field. However SPCR.InterfaceType
is 8-bit while the Port Subtype field in DBG2 table is 16-bit.

Since the Configuration Manager represents the Serial port
information using the struct CM_ARM_SERIAL_PORT_INFO, the
PortSubtype member in this struct is 16-bit.

To fix the warning an explicit type case is added. A validation
is also added to ensure that the Serial Port Subtype value
provided by the Configuration Manager is within the 8-bit
range (less than 256).

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
index 1404279f828d5c06bb7605cb2fe6d864ef7a080e..4b2580da7df9b4472ae9bcc3df2138a9e13004d0 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
@@ -217,8 +217,21 @@ BuildSpcrTable (
     goto error_handler;
   }

+  // The SPCR InterfaceType uses the same encoding as that of the
+  // DBG2 table Port Subtype field. However InterfaceType is 8-bit
+  // while the Port Subtype field in the DBG2 table is 16-bit.
+  if ((SerialPortInfo->PortSubtype & 0xFF00) != 0) {
+    Status = EFI_INVALID_PARAMETER;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: SPCR: Invalid Port Sybtype (must be < 256). Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
   // Update the serial port subtype
-  AcpiSpcr.InterfaceType = SerialPortInfo->PortSubtype;
+  AcpiSpcr.InterfaceType = (UINT8)SerialPortInfo->PortSubtype;

   // Update the base address
   AcpiSpcr.BaseAddress.Address = SerialPortInfo->BaseAddress;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 09/19] DynamicTablesPkg: Fix unaligned pointers usage

[Alexei Fedorov]

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

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


Alexei

________________________________
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: 23 August 2019 11:55
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; leif.lindholm@linaro.org <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 09/19] DynamicTablesPkg: Fix unaligned pointers usage

The VS2017 compiler reports 'warning C4366: The result of
the unary '&' operator may be unaligned' if an address of
an unaligned structure member is passed as an argument to
a function.

Fix this warning by using local variables in place of
unaligned structure members.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 24 +++++++++++++++-----
 DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h | 13 +++++------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
index 40699ce113caa8530c89ac20562cf5abda26b88e..82070403ac8757f54e839fd00eb4acb3292fc60c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c
@@ -1066,6 +1066,9 @@ BuildPpttTable (
   EFI_STATUS                      Status;
   UINT32                          TableSize;
   UINT32                          ProcTopologyStructCount;
+  UINT32                          ProcHierarchyNodeCount;
+  UINT32                          CacheStructCount;
+  UINT32                          IdStructCount;

   UINT32                          ProcHierarchyNodeOffset;
   UINT32                          CacheStructOffset;
@@ -1113,7 +1116,7 @@ BuildPpttTable (
              CfgMgrProtocol,
              CM_NULL_TOKEN,
              &ProcHierarchyNodeList,
-             &Generator->ProcHierarchyNodeCount
+             &ProcHierarchyNodeCount
              );
   if (EFI_ERROR (Status)) {
     DEBUG ((
@@ -1124,7 +1127,8 @@ BuildPpttTable (
     goto error_handler;
   }

-  ProcTopologyStructCount = Generator->ProcHierarchyNodeCount;
+  ProcTopologyStructCount = ProcHierarchyNodeCount;
+  Generator->ProcHierarchyNodeCount = ProcHierarchyNodeCount;

   // Get the cache info and update the processor topology structure count with
   // Cache Type Structures (Type 1)
@@ -1132,7 +1136,7 @@ BuildPpttTable (
              CfgMgrProtocol,
              CM_NULL_TOKEN,
              &CacheStructList,
-             &Generator->CacheStructCount
+             &CacheStructCount
              );
   if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
     DEBUG ((
@@ -1143,7 +1147,8 @@ BuildPpttTable (
     goto error_handler;
   }

-  ProcTopologyStructCount += Generator->CacheStructCount;
+  ProcTopologyStructCount += CacheStructCount;
+  Generator->CacheStructCount = CacheStructCount;

   // Get the processor hierarchy node ID info and update the processor topology
   // structure count with ID Structures (Type 2)
@@ -1151,7 +1156,7 @@ BuildPpttTable (
              CfgMgrProtocol,
              CM_NULL_TOKEN,
              &IdStructList,
-             &Generator->IdStructCount
+             &IdStructCount
              );
   if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
     DEBUG ((
@@ -1163,7 +1168,8 @@ BuildPpttTable (
     goto error_handler;
   }

-  ProcTopologyStructCount += Generator->IdStructCount;
+  ProcTopologyStructCount += IdStructCount;
+  Generator->IdStructCount = IdStructCount;

   // Allocate Node Indexer array
   NodeIndexer = (PPTT_NODE_INDEXER*)AllocateZeroPool (
@@ -1475,6 +1481,12 @@ ACPI_PPTT_GENERATOR PpttGenerator = {

   // Processor topology node count
   0,
+  // Count of Processor Hierarchy Nodes
+  0,
+  // Count of Cache Structures
+  0,
+  // Count of Id Structures
+  0,
   // Pointer to PPTT Node Indexer
   NULL
 };
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h
index 6a0fdd08e1533c57285f420555586314c70a5ed5..0a14da502d595e27d87262b1bac681318f1d9ced 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.h
@@ -167,6 +167,12 @@ typedef struct AcpiPpttGenerator {
   ACPI_TABLE_GENERATOR  Header;
   /// PPTT structure count
   UINT32                ProcTopologyStructCount;
+  /// Count of Processor Hierarchy Nodes
+  UINT32                ProcHierarchyNodeCount;
+  /// Count of Cache Structures
+  UINT32                CacheStructCount;
+  /// Count of Id Structures
+  UINT32                IdStructCount;
   /// List of indexed CM objects for PPTT generation
   PPTT_NODE_INDEXER   * NodeIndexer;
   /// Pointer to the start of Processor Hierarchy nodes in
@@ -176,13 +182,6 @@ typedef struct AcpiPpttGenerator {
   PPTT_NODE_INDEXER   * CacheStructIndexedList;
   /// Pointer to the start of Id Structures in the Node Indexer array
   PPTT_NODE_INDEXER   * IdStructIndexedList;
-  /// Count of Processor Hierarchy Nodes
-  UINT32                ProcHierarchyNodeCount;
-  /// Count of Cache Structures
-  UINT32                CacheStructCount;
-  /// Count of Id Structures
-  UINT32                IdStructCount;
-
 } ACPI_PPTT_GENERATOR;

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


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

Re: [PATCH v1 19/19] MdePkg: Initialise VA_LIST variables before use

[Liming Gao]

Is this warning reported on ARM arch?

In Base.h, VA_START is defined as below for ARM arch. Do you mean VS2017 report the warning for below macro? 
If so, can you propose the change in VA_START macro to fix this warning, then doesn't need to update consumer source code. 

#define VA_START(Marker, Parameter)     __va_start (&Marker, &Parameter, _INT_SIZE_OF (Parameter), __alignof(Parameter), &Parameter)

> -----Original Message-----
> From: Sami Mujawar [mailto:sami.mujawar@arm.com]
> Sent: Friday, August 23, 2019 6:56 PM
> To: devel@edk2.groups.io
> Cc: Sami Mujawar <sami.mujawar@arm.com>; Alexei.Fedorov@arm.com; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org;
> Matteo.Carlini@arm.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; nd@arm.com
> Subject: [PATCH v1 19/19] MdePkg: Initialise VA_LIST variables before use
> 
> The VS2017 compiler reports 'warning C6001: Using
> uninitialized memory 'Marker'.' for VA_LIST
> variables.
> 
> To fix this issue declare a VA_LIST global variable
> and use this to initialise VA_LIST variables before
> use.
> 
> Note: The VA_LIST cannot be assigned a NULL value
> because some compilers define VA_LIST to be a
> structure.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  MdePkg/Library/BaseLib/SwitchStack.c           | 9 +++++++++
>  MdePkg/Library/BasePrintLib/PrintLib.c         | 5 +++++
>  MdePkg/Library/BasePrintLib/PrintLibInternal.c | 9 +++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/MdePkg/Library/BaseLib/SwitchStack.c b/MdePkg/Library/BaseLib/SwitchStack.c
> index cb9f69f1eaceba690b48e9ca6b8a9af2e348bddd..e1bb524819b3de3521c5461ce681aa3a6c186f2c 100644
> --- a/MdePkg/Library/BaseLib/SwitchStack.c
> +++ b/MdePkg/Library/BaseLib/SwitchStack.c
> @@ -2,12 +2,20 @@
>    Switch Stack functions.
> 
>    Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
>  #include "BaseLibInternals.h"
> 
> +//
> +// Declare a VA_LIST global variable that is used to initialise VA_LIST
> +// variables before use. The VA_LIST cannot be assigned a NULL value
> +// because some compilers define VA_LIST to be a structure.
> +//
> +STATIC VA_LIST gNullVaList;
> +
>  /**
>    Transfers control to a function starting with a new stack.
> 
> @@ -57,6 +65,7 @@ SwitchStack (
>    //
>    ASSERT (((UINTN)NewStack & (CPU_STACK_ALIGNMENT - 1)) == 0);
> 
> +  Marker = gNullVaList;
>    VA_START (Marker, NewStack);
> 
>    InternalSwitchStack (EntryPoint, Context1, Context2, NewStack, Marker);
> diff --git a/MdePkg/Library/BasePrintLib/PrintLib.c b/MdePkg/Library/BasePrintLib/PrintLib.c
> index af771652e4b0aebd616973ba1089ae5bc2b6f0c0..67c5f3dd547cea5447075ef88d697879883ba5ab 100644
> --- a/MdePkg/Library/BasePrintLib/PrintLib.c
> +++ b/MdePkg/Library/BasePrintLib/PrintLib.c
> @@ -3,6 +3,7 @@
> 
>    Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>    Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> +  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -177,6 +178,7 @@ UnicodeSPrint (
>    VA_LIST Marker;
>    UINTN   NumberOfPrinted;
> 
> +  Marker = gNullVaList;
>    VA_START (Marker, FormatString);
>    NumberOfPrinted = UnicodeVSPrint (StartOfBuffer, BufferSize, FormatString, Marker);
>    VA_END (Marker);
> @@ -337,6 +339,7 @@ UnicodeSPrintAsciiFormat (
>    VA_LIST Marker;
>    UINTN   NumberOfPrinted;
> 
> +  Marker = gNullVaList;
>    VA_START (Marker, FormatString);
>    NumberOfPrinted = UnicodeVSPrintAsciiFormat (StartOfBuffer, BufferSize, FormatString, Marker);
>    VA_END (Marker);
> @@ -614,6 +617,7 @@ AsciiSPrint (
>    VA_LIST Marker;
>    UINTN   NumberOfPrinted;
> 
> +  Marker = gNullVaList;
>    VA_START (Marker, FormatString);
>    NumberOfPrinted = AsciiVSPrint (StartOfBuffer, BufferSize, FormatString, Marker);
>    VA_END (Marker);
> @@ -774,6 +778,7 @@ AsciiSPrintUnicodeFormat (
>    VA_LIST Marker;
>    UINTN   NumberOfPrinted;
> 
> +  Marker = gNullVaList;
>    VA_START (Marker, FormatString);
>    NumberOfPrinted = AsciiVSPrintUnicodeFormat (StartOfBuffer, BufferSize, FormatString, Marker);
>    VA_END (Marker);
> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> index b6ec5ac4fbb98982f8ccaf3908c2a91ce583e31e..11392f2a5d12eb059611c3ff77b27b602f9b9a40 100644
> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> @@ -2,12 +2,20 @@
>    Print Library internal worker functions.
> 
>    Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2019, ARM Ltd. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
>  #include "PrintLibInternal.h"
> 
> +//
> +// Declare a VA_LIST global variable that is used to initialise VA_LIST
> +// variables before use. The VA_LIST cannot be assigned a NULL value
> +// because some compilers define VA_LIST to be a structure.
> +//
> +extern VA_LIST gNullVaList;
> +
>  #define WARNING_STATUS_NUMBER         5
>  #define ERROR_STATUS_NUMBER           33
> 
> @@ -1256,6 +1264,7 @@ BasePrintLibSPrint (
>    VA_LIST  Marker;
>    UINTN    NumberOfPrinted;
> 
> +  Marker = gNullVaList;
>    VA_START (Marker, FormatString);
>    NumberOfPrinted = BasePrintLibSPrintMarker (StartOfBuffer, BufferSize, Flags, FormatString, Marker, NULL);
>    VA_END (Marker);
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

Re: [PATCH v1 17/19] ArmPlatformPkg: Fix UART divisor warning

[Leif Lindholm]

On Fri, Aug 23, 2019 at 11:55:37 +0100, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C4244: '=': conversion
> from 'UINT64' to 'UINT32', possible loss of data' for the
> calculation of the UART Divisor value.
> 
> Fix this warning by adding appropriate typecast and a validation
> that ensures that the UART divisor value generated does not exceed
> MAX_UINT32.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Apologies for missing this set.

> ---
>  ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> index 801990d9551a638c17d560d4226137b8a3ee47bb..2d3c279cce49304959953ec4a34b50e09a7d0045 100644
> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> @@ -2,7 +2,7 @@
>    Serial I/O Port library functions with no library constructor/destructor
>  
>    Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -  Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -78,6 +78,7 @@ PL011UartInitializePort (
>    UINT32      Integer;
>    UINT32      Fractional;
>    UINT32      HardwareFifoDepth;
> +  UINT64      DivisorValue;
>  
>    HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
>                         > PL011_VER_R1P4) \
> @@ -188,7 +189,12 @@ PL011UartInitializePort (
>        return RETURN_INVALID_PARAMETER;
>      }
>  
> -    Divisor = (UartClkInHz * 4) / *BaudRate;
> +    DivisorValue = (((UINT64)UartClkInHz * 4) / *BaudRate);
> +    if (DivisorValue > MAX_UINT32) {
> +      return RETURN_INVALID_PARAMETER;
> +    }
> +
> +    Divisor = (UINT32)DivisorValue;
>      Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
>      Fractional = Divisor & FRACTION_PART_MASK;
>    }
> -- 
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Leif Lindholm]

On Fri, Aug 23, 2019 at 11:55:38 +0100, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C6326: potential
> comparison of a constant with another constant' when
> a fixed PCD value is compared with a constant value.
> 
> The faulting code is as marked by '-->' below:
> 
> --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
>       Integer = FixedPcdGet32 (PL011UartInteger);
>       Fractional = FixedPcdGet32 (PL011UartFractional);
>     } else {
>     ...
> 
> The macro FixedPcdGet32 (PL011UartInteger) evaluates
> to a macro _PCD_VALUE_PL011UartInteger that is defined
> by the build system to represent the UART Integer
> value. Therefore, the VS2017 compiler reports the above
> warning.
> 
> Fix this warning by enclosing the code in appropriate
>  #if .. #else .. #endif directives.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644
> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> @@ -174,10 +174,10 @@ PL011UartInitializePort (
>    //
>  
>    // If PL011 Integer value has been defined then always ignore the BAUD rate
> -  if (FixedPcdGet32 (PL011UartInteger) != 0) {
> +#if (FixedPcdGet32 (PL011UartInteger) != 0)
>      Integer = FixedPcdGet32 (PL011UartInteger);
>      Fractional = FixedPcdGet32 (PL011UartFractional);
> -  } else {
> +#else
>      // If BAUD rate is zero then replace it with the system default value
>      if (*BaudRate == 0) {
>        *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);
> @@ -197,7 +197,7 @@ PL011UartInitializePort (
>      Divisor = (UINT32)DivisorValue;
>      Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
>      Fractional = Divisor & FRACTION_PART_MASK;
> -  }
> +#endif
>  
>    //
>    // If PL011 is already initialized, check the current settings
> -- 
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Ard Biesheuvel]

On Thu, 21 Nov 2019 at 13:35, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Fri, Aug 23, 2019 at 11:55:38 +0100, Sami Mujawar wrote:
> > The VS2017 compiler reports 'warning C6326: potential
> > comparison of a constant with another constant' when
> > a fixed PCD value is compared with a constant value.
> >
> > The faulting code is as marked by '-->' below:
> >
> > --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
> >       Integer = FixedPcdGet32 (PL011UartInteger);
> >       Fractional = FixedPcdGet32 (PL011UartFractional);
> >     } else {
> >     ...
> >
> > The macro FixedPcdGet32 (PL011UartInteger) evaluates
> > to a macro _PCD_VALUE_PL011UartInteger that is defined
> > by the build system to represent the UART Integer
> > value. Therefore, the VS2017 compiler reports the above
> > warning.
> >
> > Fix this warning by enclosing the code in appropriate
> >  #if .. #else .. #endif directives.
> >
> > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

I don't like this at all.

If two expressions happen to evaluate to the same constant, we should
be able to compare them using C code.

I think we should disable the warning instead.


> > ---
> >  ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644
> > --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > @@ -174,10 +174,10 @@ PL011UartInitializePort (
> >    //
> >
> >    // If PL011 Integer value has been defined then always ignore the BAUD rate
> > -  if (FixedPcdGet32 (PL011UartInteger) != 0) {
> > +#if (FixedPcdGet32 (PL011UartInteger) != 0)
> >      Integer = FixedPcdGet32 (PL011UartInteger);
> >      Fractional = FixedPcdGet32 (PL011UartFractional);
> > -  } else {
> > +#else
> >      // If BAUD rate is zero then replace it with the system default value
> >      if (*BaudRate == 0) {
> >        *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);
> > @@ -197,7 +197,7 @@ PL011UartInitializePort (
> >      Divisor = (UINT32)DivisorValue;
> >      Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
> >      Fractional = Divisor & FRACTION_PART_MASK;
> > -  }
> > +#endif
> >
> >    //
> >    // If PL011 is already initialized, check the current settings
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Leif Lindholm]

On Thu, Nov 21, 2019 at 13:39:55 +0100, Ard Biesheuvel wrote:
> On Thu, 21 Nov 2019 at 13:35, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Fri, Aug 23, 2019 at 11:55:38 +0100, Sami Mujawar wrote:
> > > The VS2017 compiler reports 'warning C6326: potential
> > > comparison of a constant with another constant' when
> > > a fixed PCD value is compared with a constant value.
> > >
> > > The faulting code is as marked by '-->' below:
> > >
> > > --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
> > >       Integer = FixedPcdGet32 (PL011UartInteger);
> > >       Fractional = FixedPcdGet32 (PL011UartFractional);
> > >     } else {
> > >     ...
> > >
> > > The macro FixedPcdGet32 (PL011UartInteger) evaluates
> > > to a macro _PCD_VALUE_PL011UartInteger that is defined
> > > by the build system to represent the UART Integer
> > > value. Therefore, the VS2017 compiler reports the above
> > > warning.
> > >
> > > Fix this warning by enclosing the code in appropriate
> > >  #if .. #else .. #endif directives.
> > >
> > > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> >
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> 
> I don't like this at all.
> 
> If two expressions happen to evaluate to the same constant, we should
> be able to compare them using C code.
>
> I think we should disable the warning instead.

I agree with that as a general rule, and agree that disabling the
warning may be preferable in other contexts.

But I somewhat disagree with referring to *this instance* as two
expressions evaluating to the same constant. This is entirely a
preprocessor event, somewhat obscured by the Pcd syntax.
The test is "has the default baudrate been requested or has a specific
one been requested?".

Now, that might be more cleanly described as a macro than inline like
this - but I think the compiler has usefully warned us about weirdly
written code. So I would really not want to disable the warning on a
global level, and would prefer not to do so here.

/
    Leif
 
> 
> > > ---
> > >  ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > > index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644
> > > --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > > +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > > @@ -174,10 +174,10 @@ PL011UartInitializePort (
> > >    //
> > >
> > >    // If PL011 Integer value has been defined then always ignore the BAUD rate
> > > -  if (FixedPcdGet32 (PL011UartInteger) != 0) {
> > > +#if (FixedPcdGet32 (PL011UartInteger) != 0)
> > >      Integer = FixedPcdGet32 (PL011UartInteger);
> > >      Fractional = FixedPcdGet32 (PL011UartFractional);
> > > -  } else {
> > > +#else
> > >      // If BAUD rate is zero then replace it with the system default value
> > >      if (*BaudRate == 0) {
> > >        *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);
> > > @@ -197,7 +197,7 @@ PL011UartInitializePort (
> > >      Divisor = (UINT32)DivisorValue;
> > >      Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
> > >      Fractional = Divisor & FRACTION_PART_MASK;
> > > -  }
> > > +#endif
> > >
> > >    //
> > >    // If PL011 is already initialized, check the current settings
> > > --
> > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> > >
> > >

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Ard Biesheuvel]

On Thu, 21 Nov 2019 at 13:53, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 21, 2019 at 13:39:55 +0100, Ard Biesheuvel wrote:
> > On Thu, 21 Nov 2019 at 13:35, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Fri, Aug 23, 2019 at 11:55:38 +0100, Sami Mujawar wrote:
> > > > The VS2017 compiler reports 'warning C6326: potential
> > > > comparison of a constant with another constant' when
> > > > a fixed PCD value is compared with a constant value.
> > > >
> > > > The faulting code is as marked by '-->' below:
> > > >
> > > > --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
> > > >       Integer = FixedPcdGet32 (PL011UartInteger);
> > > >       Fractional = FixedPcdGet32 (PL011UartFractional);
> > > >     } else {
> > > >     ...
> > > >
> > > > The macro FixedPcdGet32 (PL011UartInteger) evaluates
> > > > to a macro _PCD_VALUE_PL011UartInteger that is defined
> > > > by the build system to represent the UART Integer
> > > > value. Therefore, the VS2017 compiler reports the above
> > > > warning.
> > > >
> > > > Fix this warning by enclosing the code in appropriate
> > > >  #if .. #else .. #endif directives.
> > > >
> > > > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > >
> > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> > >
> >
> > I don't like this at all.
> >
> > If two expressions happen to evaluate to the same constant, we should
> > be able to compare them using C code.
> >
> > I think we should disable the warning instead.
>
> I agree with that as a general rule, and agree that disabling the
> warning may be preferable in other contexts.
>
> But I somewhat disagree with referring to *this instance* as two
> expressions evaluating to the same constant. This is entirely a
> preprocessor event, somewhat obscured by the Pcd syntax.
> The test is "has the default baudrate been requested or has a specific
> one been requested?".
>

That may be true, but moving from C conditionals to CPP conditionals
obscures the code, and reduces test coverage, so I think it should be
avoided when possible.

> Now, that might be more cleanly described as a macro than inline like
> this - but I think the compiler has usefully warned us about weirdly
> written code. So I would really not want to disable the warning on a
> global level, and would prefer not to do so here.
>
> /
>     Leif
>
> >
> > > > ---
> > > >  ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > > > index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644
> > > > --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > > > +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> > > > @@ -174,10 +174,10 @@ PL011UartInitializePort (
> > > >    //
> > > >
> > > >    // If PL011 Integer value has been defined then always ignore the BAUD rate
> > > > -  if (FixedPcdGet32 (PL011UartInteger) != 0) {
> > > > +#if (FixedPcdGet32 (PL011UartInteger) != 0)
> > > >      Integer = FixedPcdGet32 (PL011UartInteger);
> > > >      Fractional = FixedPcdGet32 (PL011UartFractional);
> > > > -  } else {
> > > > +#else
> > > >      // If BAUD rate is zero then replace it with the system default value
> > > >      if (*BaudRate == 0) {
> > > >        *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);
> > > > @@ -197,7 +197,7 @@ PL011UartInitializePort (
> > > >      Divisor = (UINT32)DivisorValue;
> > > >      Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
> > > >      Fractional = Divisor & FRACTION_PART_MASK;
> > > > -  }
> > > > +#endif
> > > >
> > > >    //
> > > >    // If PL011 is already initialized, check the current settings
> > > > --
> > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> > > >
> > > >

Re: [edk2-devel] [PATCH v1 17/19] ArmPlatformPkg: Fix UART divisor warning

[Philippe Mathieu-Daudé]

On 8/23/19 12:55 PM, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C4244: '=': conversion
> from 'UINT64' to 'UINT32', possible loss of data' for the
> calculation of the UART Divisor value.
> 
> Fix this warning by adding appropriate typecast and a validation
> that ensures that the UART divisor value generated does not exceed
> MAX_UINT32.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> index 801990d9551a638c17d560d4226137b8a3ee47bb..2d3c279cce49304959953ec4a34b50e09a7d0045 100644
> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> @@ -2,7 +2,7 @@
>     Serial I/O Port library functions with no library constructor/destructor
>   
>     Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -  Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -78,6 +78,7 @@ PL011UartInitializePort (
>     UINT32      Integer;
>     UINT32      Fractional;
>     UINT32      HardwareFifoDepth;
> +  UINT64      DivisorValue;
>   
>     HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
>                          > PL011_VER_R1P4) \
> @@ -188,7 +189,12 @@ PL011UartInitializePort (
>         return RETURN_INVALID_PARAMETER;
>       }
>   
> -    Divisor = (UartClkInHz * 4) / *BaudRate;
> +    DivisorValue = (((UINT64)UartClkInHz * 4) / *BaudRate);
> +    if (DivisorValue > MAX_UINT32) {
> +      return RETURN_INVALID_PARAMETER;
> +    }
> +
> +    Divisor = (UINT32)DivisorValue;
>       Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
>       Fractional = Divisor & FRACTION_PART_MASK;
>     }
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Leif Lindholm]

On Thu, Nov 21, 2019 at 14:09:41 +0100, Ard Biesheuvel wrote:
> > > I don't like this at all.
> > >
> > > If two expressions happen to evaluate to the same constant, we should
> > > be able to compare them using C code.
> > >
> > > I think we should disable the warning instead.
> >
> > I agree with that as a general rule, and agree that disabling the
> > warning may be preferable in other contexts.
> >
> > But I somewhat disagree with referring to *this instance* as two
> > expressions evaluating to the same constant. This is entirely a
> > preprocessor event, somewhat obscured by the Pcd syntax.
> > The test is "has the default baudrate been requested or has a specific
> > one been requested?".
> 
> That may be true, but moving from C conditionals to CPP conditionals
> obscures the code, and reduces test coverage, so I think it should be
> avoided when possible.

I don't see what test coverage this would change. Even in the NOOPT
case, only the codegen would be affected - only one of the two cases
would be reachable. In all other cases, the compiler would go
  if (115200 != 0) {
  /* Oh, I'll just ignore any else clauses then ... */

I think the pre-existing code obscures what is actually going on and
that the change clarifies it (but could be prettified).

Now, of course, why we have a platform-specific (and platform-global)
override variable to make a library ignore its inputs is another
interesting question.

So. Rock-paper-scissors or coin-toss?

/
    Leif

Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

[Ard Biesheuvel]

On Thu, 21 Nov 2019 at 16:15, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 21, 2019 at 14:09:41 +0100, Ard Biesheuvel wrote:
> > > > I don't like this at all.
> > > >
> > > > If two expressions happen to evaluate to the same constant, we should
> > > > be able to compare them using C code.
> > > >
> > > > I think we should disable the warning instead.
> > >
> > > I agree with that as a general rule, and agree that disabling the
> > > warning may be preferable in other contexts.
> > >
> > > But I somewhat disagree with referring to *this instance* as two
> > > expressions evaluating to the same constant. This is entirely a
> > > preprocessor event, somewhat obscured by the Pcd syntax.
> > > The test is "has the default baudrate been requested or has a specific
> > > one been requested?".
> >
> > That may be true, but moving from C conditionals to CPP conditionals
> > obscures the code, and reduces test coverage, so I think it should be
> > avoided when possible.
>
> I don't see what test coverage this would change. Even in the NOOPT
> case, only the codegen would be affected - only one of the two cases
> would be reachable. In all other cases, the compiler would go
>   if (115200 != 0) {
>   /* Oh, I'll just ignore any else clauses then ... */
>
> I think the pre-existing code obscures what is actually going on and
> that the change clarifies it (but could be prettified).
>
> Now, of course, why we have a platform-specific (and platform-global)
> override variable to make a library ignore its inputs is another
> interesting question.
>
> So. Rock-paper-scissors or coin-toss?
>

Yes!

Re: [edk2-devel] [PATCH v1 04/19] DynamicTablesPkg: Fix serial port subtype warning

[Philippe Mathieu-Daudé]

On 8/23/19 12:55 PM, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C4244: '=': conversion
> from 'UINT16' to 'UINT8', possible loss of data' for the
> SPCR InterfaceType field assignment.
> 
> The SPCR InterfaceType field uses the same encoding as that
> of the DBG2 table Port Subtype field. However SPCR.InterfaceType
> is 8-bit while the Port Subtype field in DBG2 table is 16-bit.
> 
> Since the Configuration Manager represents the Serial port
> information using the struct CM_ARM_SERIAL_PORT_INFO, the
> PortSubtype member in this struct is 16-bit.
> 
> To fix the warning an explicit type case is added. A validation
> is also added to ensure that the Serial Port Subtype value
> provided by the Configuration Manager is within the 8-bit
> range (less than 256).
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
> index 1404279f828d5c06bb7605cb2fe6d864ef7a080e..4b2580da7df9b4472ae9bcc3df2138a9e13004d0 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
> @@ -217,8 +217,21 @@ BuildSpcrTable (
>       goto error_handler;
>     }
>   
> +  // The SPCR InterfaceType uses the same encoding as that of the
> +  // DBG2 table Port Subtype field. However InterfaceType is 8-bit
> +  // while the Port Subtype field in the DBG2 table is 16-bit.
> +  if ((SerialPortInfo->PortSubtype & 0xFF00) != 0) {
> +    Status = EFI_INVALID_PARAMETER;
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: SPCR: Invalid Port Sybtype (must be < 256). Status = %r\n",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
>     // Update the serial port subtype
> -  AcpiSpcr.InterfaceType = SerialPortInfo->PortSubtype;
> +  AcpiSpcr.InterfaceType = (UINT8)SerialPortInfo->PortSubtype;
>   
>     // Update the base address
>     AcpiSpcr.BaseAddress.Address = SerialPortInfo->BaseAddress;
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

[Philippe Mathieu-Daudé]

On 8/23/19 12:55 PM, Sami Mujawar wrote:
> The ARM DCC serial port subtype is an option that is
> supported by the DBG2 generator. However, the serial
> port initialisation should only be done for PL011/SBSA
> compatible UARTs.
> 
> Add check to conditionally initialise the serial port.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
>   1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
>     CM_ARM_SERIAL_PORT_INFO
>     );
>   
> -/** Initialize the PL011 UART with the parameters obtained from
> +/** Initialize the PL011/SBSA UART with the parameters obtained from
>       the Configuration Manager.

Isn't the SBSA UART a PL011?

>   
>     @param [in]  SerialPortInfo Pointer to the Serial Port Information.
> @@ -353,15 +353,22 @@ BuildDbg2Table (
>     AcpiDbg2.Dbg2DeviceInfo[DBG_PORT_INDEX_PORT1].Dbg2Device.PortSubtype =
>       SerialPortInfo->PortSubtype;
>   
> -  // Initialize the serial port
> -  Status = SetupDebugUart (SerialPortInfo);
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((
> -      DEBUG_ERROR,
> -      "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
> -      Status
> -      ));
> -    goto error_handler;
> +  if ((SerialPortInfo->PortSubtype ==
> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_PL011_UART)           ||
> +      (SerialPortInfo->PortSubtype ==
> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART_2X) ||
> +      (SerialPortInfo->PortSubtype ==
> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART)) {
> +    // Initialize the serial port
> +    Status = SetupDebugUart (SerialPortInfo);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
> +        Status
> +        ));
> +      goto error_handler;
> +    }
>     }
>   
>     *Table = (EFI_ACPI_DESCRIPTION_HEADER*)&AcpiDbg2;
> 

Re: [edk2-devel] [PATCH v1 02/19] DynamicTablesPkg: Fix missing local header warning

[Philippe Mathieu-Daudé]

On 8/23/19 12:55 PM, Sami Mujawar wrote:
> The edk2 BaseTools report a warning if a local header file
> is not listed under the [Sources] section in the INF file.
> 
> Add header files to the [Sources] section in the respective
> INF files to fix the warnings.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf | 1 +
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf        | 3 ++-
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf        | 1 +
>   3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
> index b47a690dd72702a2d7157cbb21249ba1d01d08cd..56b69591ac52ca3ba51250dc01a0846b22442ce9 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
> +++ b/DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf
> @@ -25,6 +25,7 @@ [Sources]
>     DeviceTreeTableFactory/DeviceTreeTableFactory.c
>     DynamicTableFactoryDxe.c
>     SmbiosTableFactory/SmbiosTableFactory.c
> +  DynamicTableFactory.h
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
> index 3bbd0e7818c6172f9178b12a3d0960e2d298bca1..2f987e3b6d339d4b5f342cb52b8dc712cf4e585c 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
> @@ -1,7 +1,7 @@
>   ## @file
>   #  IORT Table Generator
>   #
> -#  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
> +#  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
>   #
>   #  SPDX-License-Identifier: BSD-2-Clause-Patent
>   ##
> @@ -18,6 +18,7 @@ [Defines]
>   
>   [Sources]
>     IortGenerator.c
> +  IortGenerator.h
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
> index 3cb13d7d8fd3df04f3e16cc6161d9736f7579a80..2c7d19513d6ceea29fd9f271f983a6cf6d2c1046 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
> @@ -18,6 +18,7 @@ [Defines]
>   
>   [Sources]
>     PpttGenerator.c
> +  PpttGenerator.h
>   
>   [Packages]
>     EmbeddedPkg/EmbeddedPkg.dec
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

[Leif Lindholm]

On Thu, Nov 21, 2019 at 16:20:31 +0100, Philippe Mathieu-Daudé wrote:
> On 8/23/19 12:55 PM, Sami Mujawar wrote:
> > The ARM DCC serial port subtype is an option that is
> > supported by the DBG2 generator. However, the serial
> > port initialisation should only be done for PL011/SBSA
> > compatible UARTs.
> > 
> > Add check to conditionally initialise the serial port.
> > 
> > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > ---
> >   DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
> >   1 file changed, 17 insertions(+), 10 deletions(-)
> > 
> > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
> > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
> >     CM_ARM_SERIAL_PORT_INFO
> >     );
> > -/** Initialize the PL011 UART with the parameters obtained from
> > +/** Initialize the PL011/SBSA UART with the parameters obtained from
> >       the Configuration Manager.
> 
> Isn't the SBSA UART a PL011?

No. It's a compatible subset.
So a PL011 can be used as an SBSA UART.

/
    Leif

> >     @param [in]  SerialPortInfo Pointer to the Serial Port Information.
> > @@ -353,15 +353,22 @@ BuildDbg2Table (
> >     AcpiDbg2.Dbg2DeviceInfo[DBG_PORT_INDEX_PORT1].Dbg2Device.PortSubtype =
> >       SerialPortInfo->PortSubtype;
> > -  // Initialize the serial port
> > -  Status = SetupDebugUart (SerialPortInfo);
> > -  if (EFI_ERROR (Status)) {
> > -    DEBUG ((
> > -      DEBUG_ERROR,
> > -      "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
> > -      Status
> > -      ));
> > -    goto error_handler;
> > +  if ((SerialPortInfo->PortSubtype ==
> > +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_PL011_UART)           ||
> > +      (SerialPortInfo->PortSubtype ==
> > +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART_2X) ||
> > +      (SerialPortInfo->PortSubtype ==
> > +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART)) {
> > +    // Initialize the serial port
> > +    Status = SetupDebugUart (SerialPortInfo);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
> > +        Status
> > +        ));
> > +      goto error_handler;
> > +    }
> >     }
> >     *Table = (EFI_ACPI_DESCRIPTION_HEADER*)&AcpiDbg2;
> > 
> 

Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

[Philippe Mathieu-Daudé]

On 11/21/19 4:23 PM, Leif Lindholm wrote:
> On Thu, Nov 21, 2019 at 16:20:31 +0100, Philippe Mathieu-Daudé wrote:
>> On 8/23/19 12:55 PM, Sami Mujawar wrote:
>>> The ARM DCC serial port subtype is an option that is
>>> supported by the DBG2 generator. However, the serial
>>> port initialisation should only be done for PL011/SBSA
>>> compatible UARTs.
>>>
>>> Add check to conditionally initialise the serial port.
>>>
>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>> ---
>>>    DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
>>>    1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>> index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
>>> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>> @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
>>>      CM_ARM_SERIAL_PORT_INFO
>>>      );
>>> -/** Initialize the PL011 UART with the parameters obtained from
>>> +/** Initialize the PL011/SBSA UART with the parameters obtained from
>>>        the Configuration Manager.
>>
>> Isn't the SBSA UART a PL011?
> 
> No. It's a compatible subset.
> So a PL011 can be used as an SBSA UART.

OK thanks.

Can you update the comment? Maybe:

"Initialize the PL011 compatible UART with the parameters ..."

Regardless:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> /
>      Leif
> 
>>>      @param [in]  SerialPortInfo Pointer to the Serial Port Information.
>>> @@ -353,15 +353,22 @@ BuildDbg2Table (
>>>      AcpiDbg2.Dbg2DeviceInfo[DBG_PORT_INDEX_PORT1].Dbg2Device.PortSubtype =
>>>        SerialPortInfo->PortSubtype;
>>> -  // Initialize the serial port
>>> -  Status = SetupDebugUart (SerialPortInfo);
>>> -  if (EFI_ERROR (Status)) {
>>> -    DEBUG ((
>>> -      DEBUG_ERROR,
>>> -      "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
>>> -      Status
>>> -      ));
>>> -    goto error_handler;
>>> +  if ((SerialPortInfo->PortSubtype ==
>>> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_PL011_UART)           ||
>>> +      (SerialPortInfo->PortSubtype ==
>>> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART_2X) ||
>>> +      (SerialPortInfo->PortSubtype ==
>>> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART)) {
>>> +    // Initialize the serial port
>>> +    Status = SetupDebugUart (SerialPortInfo);
>>> +    if (EFI_ERROR (Status)) {
>>> +      DEBUG ((
>>> +        DEBUG_ERROR,
>>> +        "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
>>> +        Status
>>> +        ));
>>> +      goto error_handler;
>>> +    }
>>>      }
>>>      *Table = (EFI_ACPI_DESCRIPTION_HEADER*)&AcpiDbg2;
>>>
>>
> 

Re: [edk2-devel] [PATCH v1 15/19] DynamicTablesPkg: Remove erroneous use of EFIAPI

[Philippe Mathieu-Daudé]

On 8/23/19 12:55 PM, Sami Mujawar wrote:
> The Dynamic Tables Factory protocol has an erroneous
> EFIAPI calling convention macro in the function
> pointer declaration.
> 
> Remove the erroneous EFIAPI calling convention macro
> from the interface declarations.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h b/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
> index d07d0cac36dedb0568f6f604daa22429565aafd3..ff2331b060019976319fc2707f65252130a5326c 100644
> --- a/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
> +++ b/DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
> @@ -1,6 +1,6 @@
>   /** @file
>   
> -  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
> +  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -53,7 +53,6 @@ typedef struct DynamicTableFactoryInfo EDKII_DYNAMIC_TABLE_FACTORY_INFO;
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_ACPI_TABLE_GENERATOR) (
>     IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
>     IN  CONST ACPI_TABLE_GENERATOR_ID                       GeneratorId,
> @@ -73,7 +72,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_ACPI_TABLE_GENERATOR) (
>     IN  CONST ACPI_TABLE_GENERATOR                * CONST Generator
>     );
> @@ -89,7 +87,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_ACPI_TABLE_GENERATOR) (
>     IN  CONST ACPI_TABLE_GENERATOR                * CONST Generator
>     );
> @@ -109,7 +106,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_SMBIOS_TABLE_GENERATOR) (
>     IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
>     IN  CONST SMBIOS_TABLE_GENERATOR_ID                     GeneratorId,
> @@ -129,7 +125,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_SMBIOS_TABLE_GENERATOR) (
>     IN  CONST SMBIOS_TABLE_GENERATOR              * CONST Generator
>     );
> @@ -145,7 +140,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_SMBIOS_TABLE_GENERATOR) (
>     IN  CONST SMBIOS_TABLE_GENERATOR              * CONST Generator
>     );
> @@ -165,7 +159,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_GET_DT_TABLE_GENERATOR) (
>     IN  CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * CONST This,
>     IN  CONST DT_TABLE_GENERATOR_ID                         GeneratorId,
> @@ -185,7 +178,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_REGISTER_DT_TABLE_GENERATOR) (
>     IN  CONST DT_TABLE_GENERATOR                * CONST Generator
>     );
> @@ -204,7 +196,6 @@ EFIAPI
>   **/
>   typedef
>   EFI_STATUS
> -EFIAPI
>   (EFIAPI * EDKII_DYNAMIC_TABLE_FACTORY_DEREGISTER_DT_TABLE_GENERATOR) (
>     IN  CONST DT_TABLE_GENERATOR                * CONST Generator
>     );
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

[Leif Lindholm]

On Thu, Nov 21, 2019 at 16:29:16 +0100, Philippe Mathieu-Daudé wrote:
> On 11/21/19 4:23 PM, Leif Lindholm wrote:
> > On Thu, Nov 21, 2019 at 16:20:31 +0100, Philippe Mathieu-Daudé wrote:
> > > On 8/23/19 12:55 PM, Sami Mujawar wrote:
> > > > The ARM DCC serial port subtype is an option that is
> > > > supported by the DBG2 generator. However, the serial
> > > > port initialisation should only be done for PL011/SBSA
> > > > compatible UARTs.
> > > > 
> > > > Add check to conditionally initialise the serial port.
> > > > 
> > > > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > > > ---
> > > >    DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
> > > >    1 file changed, 17 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > > > index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
> > > > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > > > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > > > @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
> > > >      CM_ARM_SERIAL_PORT_INFO
> > > >      );
> > > > -/** Initialize the PL011 UART with the parameters obtained from
> > > > +/** Initialize the PL011/SBSA UART with the parameters obtained from
> > > >        the Configuration Manager.
> > > 
> > > Isn't the SBSA UART a PL011?
> > 
> > No. It's a compatible subset.
> > So a PL011 can be used as an SBSA UART.
> 
> OK thanks.
> 
> Can you update the comment? Maybe:
> 
> "Initialize the PL011 compatible UART with the parameters ..."

The original is correct, the suggested alternative is not (an SBSA
UART is a subset, so not fully compatible).

If the comment was to change, I would suggest that dropping the model
name completely and simply refer to it as "the UART" would be
preferable.

/
    Leif

> Regardless:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
> > 
> > /
> >      Leif

Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

[Philippe Mathieu-Daudé]

On 11/21/19 4:48 PM, Leif Lindholm wrote:
> On Thu, Nov 21, 2019 at 16:29:16 +0100, Philippe Mathieu-Daudé wrote:
>> On 11/21/19 4:23 PM, Leif Lindholm wrote:
>>> On Thu, Nov 21, 2019 at 16:20:31 +0100, Philippe Mathieu-Daudé wrote:
>>>> On 8/23/19 12:55 PM, Sami Mujawar wrote:
>>>>> The ARM DCC serial port subtype is an option that is
>>>>> supported by the DBG2 generator. However, the serial
>>>>> port initialisation should only be done for PL011/SBSA
>>>>> compatible UARTs.
>>>>>
>>>>> Add check to conditionally initialise the serial port.
>>>>>
>>>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>> ---
>>>>>     DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
>>>>>     1 file changed, 17 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>>>> index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
>>>>> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>>>> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>>>> @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
>>>>>       CM_ARM_SERIAL_PORT_INFO
>>>>>       );
>>>>> -/** Initialize the PL011 UART with the parameters obtained from
>>>>> +/** Initialize the PL011/SBSA UART with the parameters obtained from
>>>>>         the Configuration Manager.
>>>>
>>>> Isn't the SBSA UART a PL011?
>>>
>>> No. It's a compatible subset.
>>> So a PL011 can be used as an SBSA UART.
>>
>> OK thanks.
>>
>> Can you update the comment? Maybe:
>>
>> "Initialize the PL011 compatible UART with the parameters ..."
> 
> The original is correct, the suggested alternative is not (an SBSA
> UART is a subset, so not fully compatible).

OK, thanks for clarifying.

> 
> If the comment was to change, I would suggest that dropping the model
> name completely and simply refer to it as "the UART" would be
> preferable.
> 
> /
>      Leif
> 
>> Regardless:
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>>>
>>> /
>>>       Leif
> 

Re: [edk2-devel] [PATCH v1 06/19] DynamicTablesPkg: Fix GT Block length assignment

[Philippe Mathieu-Daudé]

On 8/23/19 12:55 PM, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C4267: '=': conversion from
> 'size_t' to 'UINT16', possible loss of data'.
> 
> The sizeof() operator is used to calculate the size of the
> GT Block structure. The length field in the GT Block structure
> is 16-bit wide. Since the return type of sizeof() operator
> is size_t the VS2017 compiler reports the above warning.
> 
> To fix the warning, an explicit type cast is added. An additional
> check is also performed to ensure that the calculated GT Block
> length does not exceed MAX_UINT16.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c | 25 ++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
> index 7e86c30649bd36ecebac75c7e3a86a0c25cd590b..0e996698887aefca8a7240fc27fe3cb9324fd3e2 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
> @@ -350,6 +350,7 @@ AddGTBlockList (
>     EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE  * GtBlockFrame;
>     CM_ARM_GTBLOCK_TIMER_FRAME_INFO             * GTBlockTimerFrameList;
>     UINT32                                        GTBlockTimerFrameCount;
> +  UINTN                                         Length;
>   
>     ASSERT (Gtdt != NULL);
>     ASSERT (GTBlockInfo != NULL);
> @@ -376,11 +377,27 @@ AddGTBlockList (
>         return Status;
>       }
>   
> +    Length = sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_STRUCTURE) +
> +               (sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE) *
> +                GTBlockInfo->GTBlockTimerFrameCount);
> +
> +    // Check that the length of the GT block does not
> +    // exceed MAX_UINT16
> +    if (Length > MAX_UINT16) {
> +      Status = EFI_INVALID_PARAMETER;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: GTDT: Too many GT Frames. Count = %d. " \
> +        "Maximum supported GT Block size exceeded. " \
> +        "Status = %r\n",
> +        GTBlockInfo->GTBlockTimerFrameCount,
> +        Status
> +        ));
> +      return Status;
> +    }
> +
>       GTBlock->Type = EFI_ACPI_6_3_GTDT_GT_BLOCK;
> -    GTBlock->Length = sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_STRUCTURE) +
> -                        (sizeof (EFI_ACPI_6_3_GTDT_GT_BLOCK_TIMER_STRUCTURE) *
> -                          GTBlockInfo->GTBlockTimerFrameCount);
> -
> +    GTBlock->Length = (UINT16)Length;
>       GTBlock->Reserved = EFI_ACPI_RESERVED_BYTE;
>       GTBlock->CntCtlBase = GTBlockInfo->GTBlockPhysicalAddress;
>       GTBlock->GTBlockTimerCount = GTBlockInfo->GTBlockTimerFrameCount;
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v1 07/19] DynamicTablesPkg: Fix Boot arch flag width

[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 10/19] DynamicTablesPkg: Serial debug port initialisation

[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 13/19] DynamicTablesPkg: IORT: Fix uninitialized memory usage

[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 --]