[PATCH v1 0/4] Bug fixes for DynamicTablesPkg and ArmVirtPkg/kvmtool

[PATCH v1 0/4] Bug fixes for DynamicTablesPkg and ArmVirtPkg/kvmtool

[Sami Mujawar]

This patch series has the following fixes for DynamicTablesPkg and
ArmVirtPkg/Kvmtool:

  1. Reduces the log output from TableHelperLib in DynamicTablesPkg.
  2. Fixes issue with parsing of the serial port node in
     ArmSerialPortParser in DynamicTablesPkg and
     EarlyFdt16550SerialPortHookLib in ArmVirtPkg. This issue
     was not observed until another entry was added in the 'chosen'
     node in the DT when doing some experiments.
  3. Fixes the DEPEX in the Kvmtool guest RTC library as the missing
     dependency on CPU Architecture protocol would cause occasional
     failures.

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/2596_bug_fix_dynamictables_kvmtool_v1

Sami Mujawar (4):
  DynamicTablesPkg: Reduce log output from TableHelperLib
  DynamicTablesPkg: Fix parsing of serial port node
  ArmVirtPkg: Fix parsing of serial port node
  ArmVirtPkg: Fix depex in kvmtool guest Rtc library

 ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c     |  4 +--
 ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf              |  4 +--
 DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c | 28 ++++++++++----------
 DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c          |  4 +--
 4 files changed, 20 insertions(+), 20 deletions(-)

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

[PATCH v1 1/4] DynamicTablesPkg: Reduce log output from TableHelperLib

[Sami Mujawar]

Reduce the log output from Configuration Manager Object Parser
in TableHelperLib by enabling the logs only if DEBUG_INFO is
enabled.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c | 28 ++++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 5e4b88e8cc754ea72157ea4a0c6d0f5b9eae2495..99d6032510a5e912c9189df82c4d2b4398458d2d 100644
--- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -1,7 +1,7 @@
 /** @file
   Configuration Manager Object parser.
 
-  Copyright (c) 2021 - 2022, ARM Limited. All rights reserved.<BR>
+  Copyright (c) 2021 - 2023, ARM Limited. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -787,7 +787,7 @@ PrintOemId (
   )
 {
   DEBUG ((
-    DEBUG_ERROR,
+    DEBUG_INFO,
     (Format != NULL) ? Format : "%C%C%C%C%C%C",
     Ptr[0],
     Ptr[1],
@@ -855,7 +855,7 @@ PrintCmObjDesc (
     *RemainingSize -= Parser[Index].Length;
     if (*RemainingSize < 0) {
       DEBUG ((
-        DEBUG_ERROR,
+        DEBUG_INFO,
         "\nERROR: %a: Buffer overrun\n",
         Parser[Index].NameStr
         ));
@@ -865,11 +865,11 @@ PrintCmObjDesc (
 
     // Indentation
     for (IndentIndex = 0; IndentIndex < IndentLevel; IndentIndex++) {
-      DEBUG ((DEBUG_ERROR, "  "));
+      DEBUG ((DEBUG_INFO, "  "));
     }
 
     DEBUG ((
-      DEBUG_ERROR,
+      DEBUG_INFO,
       "%-*a :",
       OUTPUT_FIELD_COLUMN_WIDTH - 2 * IndentLevel,
       Parser[Index].NameStr
@@ -879,20 +879,20 @@ PrintCmObjDesc (
     } else if (Parser[Index].Format != NULL) {
       switch (Parser[Index].Length) {
         case 1:
-          DEBUG ((DEBUG_ERROR, Parser[Index].Format, *(UINT8 *)Data));
+          DEBUG ((DEBUG_INFO, Parser[Index].Format, *(UINT8 *)Data));
           break;
         case 2:
-          DEBUG ((DEBUG_ERROR, Parser[Index].Format, *(UINT16 *)Data));
+          DEBUG ((DEBUG_INFO, Parser[Index].Format, *(UINT16 *)Data));
           break;
         case 4:
-          DEBUG ((DEBUG_ERROR, Parser[Index].Format, *(UINT32 *)Data));
+          DEBUG ((DEBUG_INFO, Parser[Index].Format, *(UINT32 *)Data));
           break;
         case 8:
-          DEBUG ((DEBUG_ERROR, Parser[Index].Format, ReadUnaligned64 (Data)));
+          DEBUG ((DEBUG_INFO, Parser[Index].Format, ReadUnaligned64 (Data)));
           break;
         default:
           DEBUG ((
-            DEBUG_ERROR,
+            DEBUG_INFO,
             "\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n",
             Parser[Index].NameStr,
             Parser[Index].Length
@@ -901,7 +901,7 @@ PrintCmObjDesc (
     } else if (Parser[Index].SubObjParser != NULL) {
       SubStructSize = Parser[Index].Length;
 
-      DEBUG ((DEBUG_ERROR, "\n"));
+      DEBUG ((DEBUG_INFO, "\n"));
       PrintCmObjDesc (
         Data,
         Parser[Index].SubObjParser,
@@ -912,14 +912,14 @@ PrintCmObjDesc (
     } else {
       ASSERT (0);
       DEBUG ((
-        DEBUG_ERROR,
+        DEBUG_INFO,
         "\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n",
         Parser[Index].NameStr,
         Parser[Index].Length
         ));
     }
 
-    DEBUG ((DEBUG_ERROR, "\n"));
+    DEBUG ((DEBUG_INFO, "\n"));
     Data = (UINT8 *)Data + Parser[Index].Length;
   } // for
 }
@@ -978,7 +978,7 @@ ParseCmObjDesc (
 
   for (ObjIndex = 0; ObjIndex < ObjectCount; ObjIndex++) {
     DEBUG ((
-      DEBUG_ERROR,
+      DEBUG_INFO,
       "\n%-*a [%d/%d]:\n",
       OUTPUT_FIELD_COLUMN_WIDTH,
       ParserArray->ObjectName,
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 2/4] DynamicTablesPkg: Fix parsing of serial port node

[Sami Mujawar]

When scanning for the Serial Port in the device
tree, the length and value parameters to ScanMem8()
are not in the right order. This results in the
serial port not being detected if the chosen node
in the device tree has additional elements.

Therefore, pass the parameters to ScanMem8() in the
correct order to fix this issue.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c b/DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c
index cfd032df4d746ad2a414a0258d99fe670f1928a2..732b482eebe3754543167e45737e48242fddf065 100644
--- a/DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c
+++ b/DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c
@@ -1,7 +1,7 @@
 /** @file
   Arm Serial Port Parser.
 
-  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+  Copyright (c) 2021 - 2023, Arm Limited. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
@@ -290,7 +290,7 @@ GetSerialConsoleNode (
   }
 
   // Determine the actual path length, as a colon terminates the path.
-  Path = ScanMem8 (Prop, ':', PropSize);
+  Path = ScanMem8 (Prop, PropSize, ':');
   if (Path == NULL) {
     PathLen = (UINT32)AsciiStrLen (Prop);
   } else {
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 3/4] ArmVirtPkg: Fix parsing of serial port node

[Sami Mujawar]

When scanning for the Serial Port in the device
tree, the length and value parameters to ScanMem8()
are not in the right order. This results in the
serial port not being detected if the chosen node
in the device tree has additional elements.

Therefore, pass the parameters to ScanMem8() in the
correct order to fix this issue.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c b/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c
index fb1daf32769c20521e93de7af0f54a6a8e2c8369..c1b81920214b16137fd7c40b8ec897031e6fe9aa 100644
--- a/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c
+++ b/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c
@@ -1,7 +1,7 @@
 /** @file
   Early Platform Hook Library instance for 16550 Uart.
 
-  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2020 - 2023, Arm Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -67,7 +67,7 @@ GetSerialConsolePortAddress (
   }
 
   // Determine the actual path length, as a colon terminates the path.
-  Path = ScanMem8 (Prop, ':', PropSize);
+  Path = ScanMem8 (Prop, PropSize, ':');
   if (Path == NULL) {
     PathLen = AsciiStrLen (Prop);
   } else {
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 4/4] ArmVirtPkg: Fix depex in kvmtool guest Rtc library

[Sami Mujawar]

The Rtc library for the kvmtool guest firmware configures the
RTC controller address range as runtime memory by calling the
gDS->SetMemorySpaceAttributes().
The SetMemorySpaceAttributes() function has a dependency on
the CPU Arch Protocol. If the CPU Arch Protocol is not
installed the call to set the memory attributes fails with
error code EFI_NOT_AVAILABLE_YET.

Therefore, set the library dependency on the CPU Arch protocol.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf b/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf
index f0a7c19ca5f4b5a8eeab08af64175ae6f3526b12..c10a6737a0736b11a8f45b661f8ac38948cb40b0 100644
--- a/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf
+++ b/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  FDT client library for motorola,mc146818 RTC driver
 #
-#  Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
+#  Copyright (c) 2020 - 2023, ARM Limited. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -40,4 +40,4 @@ [Pcd]
   gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64
 
 [Depex]
-  gFdtClientProtocolGuid
+  gFdtClientProtocolGuid AND gEfiCpuArchProtocolGuid
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 0/4] Bug fixes for DynamicTablesPkg and ArmVirtPkg/kvmtool

[PierreGondois]

Hello Sami,

Just for the record, for PATCH 3/4, a similar solution was used in:
commit 0b58c4894dad ("MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency")

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Regards,
Pierre

On 3/20/23 15:05, Sami Mujawar wrote:
> This patch series has the following fixes for DynamicTablesPkg and
> ArmVirtPkg/Kvmtool:
> 
>    1. Reduces the log output from TableHelperLib in DynamicTablesPkg.
>    2. Fixes issue with parsing of the serial port node in
>       ArmSerialPortParser in DynamicTablesPkg and
>       EarlyFdt16550SerialPortHookLib in ArmVirtPkg. This issue
>       was not observed until another entry was added in the 'chosen'
>       node in the DT when doing some experiments.
>    3. Fixes the DEPEX in the Kvmtool guest RTC library as the missing
>       dependency on CPU Architecture protocol would cause occasional
>       failures.
> 
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/2596_bug_fix_dynamictables_kvmtool_v1
> 
> Sami Mujawar (4):
>    DynamicTablesPkg: Reduce log output from TableHelperLib
>    DynamicTablesPkg: Fix parsing of serial port node
>    ArmVirtPkg: Fix parsing of serial port node
>    ArmVirtPkg: Fix depex in kvmtool guest Rtc library
> 
>   ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c     |  4 +--
>   ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf              |  4 +--
>   DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c | 28 ++++++++++----------
>   DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c          |  4 +--
>   4 files changed, 20 insertions(+), 20 deletions(-)
> 

Re: [PATCH v1 0/4] Bug fixes for DynamicTablesPkg and ArmVirtPkg/kvmtool

[Sami Mujawar]

Hi Ard,

I am occassionally seeing the following error when trying to run the 
Kvmtool guest firmware.

              add-symbol-file 
w:\workspace\Build\ArmVirtKvmTool-AARCH64\DEBUG_GCC5\AARCH64\PcAtChipsetPkg\PcatRealTimeClockRuntimeDxe\PcatRealTimeClockRuntimeDxe\DEBUG\PcRtc.dll 
0xBBF80000
              Loading driver at 0x000BBF70000 EntryPoint=0x000BBF82958 
PcRtc.efi
              Failed to set memory attributes. Status = 00000002
             Failed to map memory for motorola,mc146818. Status = 00000002

             ASSERT_EFI_ERROR (Status = 00000002)
             ASSERT [PcRtc] 
w:\workspace\Build\ArmVirtKvmTool-AARCH64\DEBUG_GCC5\AARCH64\PcAtChipsetPkg\PcatRealTimeClockRuntimeDxe\PcatRealTimeClockRuntimeDxe\DEBUG\AutoGen.c(494): 
!(((INTN)(RETURN_STATUS)(Status)) < 0)

I am not sure why this issue only surfaces sometimes (which is 
worrying). However, the fix for this is patch 4/4 in this series.

Is it possible to review the ArmVirtPkg changes and if I have your 
approval to merge this series, please?

Regards,

Sami Mujawar

On 20/03/2023 02:05 pm, Sami Mujawar wrote:
> This patch series has the following fixes for DynamicTablesPkg and
> ArmVirtPkg/Kvmtool:
>
>    1. Reduces the log output from TableHelperLib in DynamicTablesPkg.
>    2. Fixes issue with parsing of the serial port node in
>       ArmSerialPortParser in DynamicTablesPkg and
>       EarlyFdt16550SerialPortHookLib in ArmVirtPkg. This issue
>       was not observed until another entry was added in the 'chosen'
>       node in the DT when doing some experiments.
>    3. Fixes the DEPEX in the Kvmtool guest RTC library as the missing
>       dependency on CPU Architecture protocol would cause occasional
>       failures.
>
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/2596_bug_fix_dynamictables_kvmtool_v1
>
> Sami Mujawar (4):
>    DynamicTablesPkg: Reduce log output from TableHelperLib
>    DynamicTablesPkg: Fix parsing of serial port node
>    ArmVirtPkg: Fix parsing of serial port node
>    ArmVirtPkg: Fix depex in kvmtool guest Rtc library
>
>   ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c     |  4 +--
>   ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf              |  4 +--
>   DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c | 28 ++++++++++----------
>   DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c          |  4 +--
>   4 files changed, 20 insertions(+), 20 deletions(-)
>

Re: [PATCH v1 0/4] Bug fixes for DynamicTablesPkg and ArmVirtPkg/kvmtool

[Ard Biesheuvel]

On Wed, 29 Mar 2023 at 12:49, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Hi Ard,
>
> I am occassionally seeing the following error when trying to run the
> Kvmtool guest firmware.
>
>               add-symbol-file
> w:\workspace\Build\ArmVirtKvmTool-AARCH64\DEBUG_GCC5\AARCH64\PcAtChipsetPkg\PcatRealTimeClockRuntimeDxe\PcatRealTimeClockRuntimeDxe\DEBUG\PcRtc.dll
> 0xBBF80000
>               Loading driver at 0x000BBF70000 EntryPoint=0x000BBF82958
> PcRtc.efi
>               Failed to set memory attributes. Status = 00000002
>              Failed to map memory for motorola,mc146818. Status = 00000002
>
>              ASSERT_EFI_ERROR (Status = 00000002)
>              ASSERT [PcRtc]
> w:\workspace\Build\ArmVirtKvmTool-AARCH64\DEBUG_GCC5\AARCH64\PcAtChipsetPkg\PcatRealTimeClockRuntimeDxe\PcatRealTimeClockRuntimeDxe\DEBUG\AutoGen.c(494):
> !(((INTN)(RETURN_STATUS)(Status)) < 0)
>
> I am not sure why this issue only surfaces sometimes (which is
> worrying). However, the fix for this is patch 4/4 in this series.
>

I suppose the dispatch order is not fixed, which is surprising. But I
think the fix is quite appropriate in any case.

> Is it possible to review the ArmVirtPkg changes and if I have your
> approval to merge this series, please?
>

Feel free to go ahead and merge them

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>