[PATCH v2 0/5] ArmVirtPkg: Add dynamic CFI flash detection for Kvmtool guests

[PATCH v2 0/5] ArmVirtPkg: Add dynamic CFI flash detection for Kvmtool guests

[Sami Mujawar]

Kvmtool allows guest VMs to be launched with or without
a CFI flash device. The guest hardware configuration can
be seen in the device tree that Kvmtool hands off to the
guest firmware.

Therefore, add support to dynamically detect if a CFI
flash device is present. If CFI is present use the
NorFlashDxe driver as the backend for variable services;
otherwise use emulated runtime variables.

The last patch in this series fix a crash due to stack
overflow which is observed when running the UEFI shell
command 'dmpstore'.

The first 4 patches in this series have not been modified
and are resent with the v2 series.

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/2646_dynamic_cfi_detection_v2

Sami Mujawar (5):
  ArmPkg: Configure PcdEmuVariableNvModeEnable as a dynamic PCD
  ArmVirtPkg: Define variables for emulating runtime variables
  ArmVirtPkg: Fallback to variable emulation if no CFI is found
  ArmVirtPkg: Dispatch variable service if variable emulation is enabled
  ArmVirtPkg/PrePi: Allocate separate stack for Dxe phase

 ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 +-
 ArmVirtPkg/ArmVirtKvmTool.dsc                                    | 11 ++++--
 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c               | 13 ++++++-
 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf             |  4 ++-
 ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c          | 38 +++++++++++++++++---
 ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf     |  3 +-
 ArmVirtPkg/PrePi/PrePi.c                                         |  4 +--
 7 files changed, 63 insertions(+), 12 deletions(-)

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

[PATCH v2 1/5] ArmPkg: Configure PcdEmuVariableNvModeEnable as a dynamic PCD

[Sami Mujawar]

The PCD gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
indicates if a variable driver will emulate the variable NV mode.
This PCD is defined as [PcdsFixedAtBuild, PcdsPatchableInModule,
PcdsDynamic, PcdsDynamicEx].

Some firmware builds may define this PCD as a dynamic PCD and
initialise the value at runtime. Therefore, move the PCD declaration
from the [FixedPcd] section to the [Pcd] section in the platform
boot manager library file PlatformBootManagerLib.inf. Without this
change the build would not succeed.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 05ed46456cc482d735490ad4418aa75a1b331aa7..bc029be635f26fa29731a4139109d0f5eb177054 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -57,7 +57,6 @@ [FeaturePcd]
 
 [FixedPcd]
   gArmTokenSpaceGuid.PcdUefiShellDefaultBootEnable
-  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
   gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
@@ -68,6 +67,7 @@ [FixedPcd]
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
   gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy
+  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
 
 [Guids]
   gBootDiscoveryPolicyMgrFormsetGuid
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v2 2/5] ArmVirtPkg: Define variables for emulating runtime variables

[Sami Mujawar]

Kvmtool allows guest VMs to be launched with or without a
CFI flash device.

When the kvmtool option '--flash <flash filename>' is used to
launch a guest VM a CFI flash device maps the flash file that
was specified at the command line. The NorFlash driver uses
this flash as the variable storage backend.

However, when the above option is not specified, a CFI flash
device is not present. In such cases, the firmware can fallback
to use emulated runtime variables (which uses the VMs DRAM as
the storage backend).

Therefore, define the PCD PcdEmuVariableNvModeEnable required
to enable the emulated runtime variable support, but do not
enable it by default.

The firmware is expected to dynamically discover if the CFI
flash is present and subsequently enable NorFlash or emulate
the runtime variables.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/ArmVirtKvmTool.dsc | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index d0afe1b49e250c554313c2077b89650d6f6d67cb..25920ab4ae3cce20fdbe8e9ff7e25b8696d2c851 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -1,7 +1,7 @@
 #  @file
 #  Workspace file for KVMTool virtual platform.
 #
-#  Copyright (c) 2018 - 2022, ARM Limited. All rights reserved.
+#  Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -219,6 +219,10 @@ [PcdsDynamicDefault.common]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x40000
 
+  # Define PCD for emulating Runtime Variable storage when
+  # CFI flash is absent.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|FALSE
+
   ## RTC Register address in MMIO space.
   gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0
   gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64|0x0
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v2 3/5] ArmVirtPkg: Fallback to variable emulation if no CFI is found

[Sami Mujawar]

The kvmtool option '--flash <flash filename>' is used to launch
a guests VM with a CFI flash device that maps the flash file
specified at the command line.
However, kvmtool allows guest VMs to be launched without a CFI
flash device. In such scenarios the firmware can utilize the
emulated variable storage for UEFI variables. To support this
the PCD gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
must be set to TRUE.

Therefore, update the NorFlashKvmtoolLib to fallback to variable
emulation if a CFI device is not detected. Also improve the error
logging.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c      | 38 +++++++++++++++++---
 ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf |  3 +-
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
index 43f5858644b1f47ada17e00fba55a670ab5862bd..2beeefdd272d6f8841f7d0b972394739b745982e 100644
--- a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
+++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
@@ -1,7 +1,7 @@
 /** @file
    An instance of the NorFlashPlatformLib for Kvmtool platform.
 
- 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
 
@@ -228,7 +228,7 @@ NorFlashPlatformLibConstructor (
   CONST CHAR8   *Label;
   UINT32        LabelLen;
 
-  if (mNorFlashDeviceCount != 0) {
+  if ((mNorFlashDeviceCount != 0) || PcdGetBool (PcdEmuVariableNvModeEnable)) {
     return EFI_SUCCESS;
   }
 
@@ -337,9 +337,39 @@ NorFlashPlatformLibConstructor (
     }
 
     if (mNorFlashDevices[UefiVarStoreIndex].DeviceBaseAddress != 0) {
-      return SetupVariableStore (&mNorFlashDevices[UefiVarStoreIndex]);
+      Status = SetupVariableStore (&mNorFlashDevices[UefiVarStoreIndex]);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((
+          DEBUG_ERROR,
+          "ERROR: Failed to setup variable store, Status = %r\n",
+          Status
+          ));
+        ASSERT (0);
+      }
+    } else {
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: Invalid Flash device Base address\n"
+        ));
+      ASSERT (0);
+      Status = EFI_NOT_FOUND;
+    }
+  } else {
+    // No Flash device found fallback to Runtime Variable Emulation.
+    DEBUG ((
+      DEBUG_INFO,
+      "INFO: No Flash device found fallback to Runtime Variable Emulation.\n"
+      ));
+    Status = PcdSetBoolS (PcdEmuVariableNvModeEnable, TRUE);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: Failed to set PcdEmuVariableNvModeEnable, Status = %r\n",
+        Status
+        ));
+      ASSERT (0);
     }
   }
 
-  return EFI_NOT_FOUND;
+  return Status;
 }
diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
index b5f35d4782896761e7975a6e5c196ff0fab0d6db..fba1245e41ec4b146db79a821b8343247377af41 100644
--- a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
+++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  Nor Flash library for Kvmtool.
 #
-#  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
 #
 ##
@@ -39,6 +39,7 @@ [Pcd]
   gArmTokenSpaceGuid.PcdFvBaseAddress
   gArmTokenSpaceGuid.PcdFvSize
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v2 4/5] ArmVirtPkg: Dispatch variable service if variable emulation is enabled

[Sami Mujawar]

The VariableRuntimeDxe links with NvVarStoreFormattedLib which is
required to establish the dependency on OvmfPkg\VirtNorFlashDxe.
The VirtNorFlashDxe installs the gEdkiiNvVarStoreFormattedGuid to
indicate it has finished initialising the flash variable storage
and that the variable service can be dispatched.

However, the kvmtool guest firmware dynamically detects if CFI
flash is absent and sets PcdEmuVariableNvModeEnable to TRUE
indicating emulated runtime variable must be used. Therefore,
in this scenario install the gEdkiiNvVarStoreFormattedGuid so
that the variable service can be dispatched.

Also link the NorFlashKvmtoolLib as a NULL library so that
it can discover if the CFI flash is absent and setup the PCD
PcdEmuVariableNvModeEnable. This is required in case the
NorFlashDxe is not yet dispatched.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/ArmVirtKvmTool.dsc                        |  5 ++++-
 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c   | 13 ++++++++++++-
 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf |  4 +++-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index 25920ab4ae3cce20fdbe8e9ff7e25b8696d2c851..4541d03d23e0d98915b3d3ada688c48d979b75d2 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -311,7 +311,10 @@ [Components.common]
   #
   # Platform Driver
   #
-  ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
+  ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf {
+    <LibraryClasses>
+    NULL|ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
+  }
   OvmfPkg/Fdt/VirtioFdtDxe/VirtioFdtDxe.inf
   EmbeddedPkg/Drivers/FdtClientDxe/FdtClientDxe.inf
   OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf
diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
index 3f5027fac4d65c4ae3f370c5349c6f410aae5b43..bf6fc1f1f070f32e3ce351f57da955c5cc849409 100644
--- a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
+++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
@@ -4,7 +4,7 @@
   - It decides if the firmware should expose ACPI or Device Tree-based
     hardware description to the operating system.
 
-  Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
+  Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -75,6 +75,17 @@ KvmtoolPlatformDxeEntryPoint (
 {
   EFI_STATUS  Status;
 
+  if (PcdGetBool (PcdEmuVariableNvModeEnable)) {
+    // The driver implementing the variable service can now be dispatched.
+    Status = gBS->InstallProtocolInterface (
+                    &gImageHandle,
+                    &gEdkiiNvVarStoreFormattedGuid,
+                    EFI_NATIVE_INTERFACE,
+                    NULL
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   Status = PlatformHasAcpiDt (ImageHandle);
   ASSERT_EFI_ERROR (Status);
 
diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
index c5bf798c3b2b7bf1f77e0c5ada9000f536123d6a..b0583d52058805aaeece31d7e3776ac498f101ad 100644
--- a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
+++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
@@ -3,7 +3,7 @@
 #  - It decides if the firmware should expose ACPI or Device Tree-based
 #    hardware description to the operating system.
 #
-#  Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
+#  Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -33,10 +33,12 @@ [LibraryClasses]
   UefiDriverEntryPoint
 
 [Guids]
+  gEdkiiNvVarStoreFormattedGuid   ## SOMETIMES_PRODUCES ## PROTOCOL
   gEdkiiPlatformHasAcpiGuid       ## SOMETIMES_PRODUCES ## PROTOCOL
   gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
 
 [Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
   gUefiOvmfPkgTokenSpaceGuid.PcdForceNoAcpi
 
 [Depex]
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v2 5/5] ArmVirtPkg/PrePi: Allocate separate stack for Dxe phase

[Sami Mujawar]

The patch "f07a9df9af60 ArmVirtPkg: Enable stack guard"
enabled stack overflow detection for ArmVirtPkg. Following
this patch, running UEFI shell command 'dmpstore' resulted
in a crash indicating a stack overflow. Invoking 'dmpstore'
results in recursive calls to CascadeProcessVariables ()
which apparently consumes the available stack space and
overflows.

Normally, SEC and PEI run off the initial stack, and the
DxeIpl PEIM is in charge of launching the DxeCore with a
full-sized stack and remapping it non-executable as well.

PrePi platforms take some shortcuts and the DXE and BDS
run off the initial stack which is relatively small. It
is therefore desirable to allocate 128 KiB worth of boot
services data memory as the stack for the Dxe phase.

The PrePiMain () in ArmVirtPkg/PrePi/PrePi.c invokes the
LoadDxeCoreFromFv () to load the Dxe core and transfers
control. The second parameter to LoadDxeCoreFromFv () is
the stack size, which is currently set to 0.
LoadDxeCoreFromFv () is implemented in PrePiLib and if the
stack size is 0, it continues to use the initial stack.
However, if a stack size is specified in the call to
LoadDxeCoreFromFv (), memory is allocated for a new stack
and the stack is switched to use the newly allocated stack
for the Dxe phase.

Therefore, specify 128 KiB as the stack size in the call to
LoadDxeCoreFromFv () so that a separate stack is allocated
and used for the Dxe phase.

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

diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
index 3d943b2138d3fe8a03322262111d5f7df3e39d39..ff51a757a21a19347c78b0936987c9f8cc283c0f 100755
--- a/ArmVirtPkg/PrePi/PrePi.c
+++ b/ArmVirtPkg/PrePi/PrePi.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2023, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -101,7 +101,7 @@ PrePiMain (
   ASSERT_EFI_ERROR (Status);
 
   // Load the DXE Core and transfer control to it
-  Status = LoadDxeCoreFromFv (NULL, 0);
+  Status = LoadDxeCoreFromFv (NULL, SIZE_128KB);
   ASSERT_EFI_ERROR (Status);
 }
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v2 0/5] ArmVirtPkg: Add dynamic CFI flash detection for Kvmtool guests

[Ard Biesheuvel]

On Fri, 19 May 2023 at 16:56, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Kvmtool allows guest VMs to be launched with or without
> a CFI flash device. The guest hardware configuration can
> be seen in the device tree that Kvmtool hands off to the
> guest firmware.
>
> Therefore, add support to dynamically detect if a CFI
> flash device is present. If CFI is present use the
> NorFlashDxe driver as the backend for variable services;
> otherwise use emulated runtime variables.
>
> The last patch in this series fix a crash due to stack
> overflow which is observed when running the UEFI shell
> command 'dmpstore'.
>
> The first 4 patches in this series have not been modified
> and are resent with the v2 series.
>
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/2646_dynamic_cfi_detection_v2
>
> Sami Mujawar (5):
>   ArmPkg: Configure PcdEmuVariableNvModeEnable as a dynamic PCD
>   ArmVirtPkg: Define variables for emulating runtime variables
>   ArmVirtPkg: Fallback to variable emulation if no CFI is found
>   ArmVirtPkg: Dispatch variable service if variable emulation is enabled
>   ArmVirtPkg/PrePi: Allocate separate stack for Dxe phase
>

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


>  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 +-
>  ArmVirtPkg/ArmVirtKvmTool.dsc                                    | 11 ++++--
>  ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c               | 13 ++++++-
>  ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf             |  4 ++-
>  ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c          | 38 +++++++++++++++++---
>  ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf     |  3 +-
>  ArmVirtPkg/PrePi/PrePi.c                                         |  4 +--
>  7 files changed, 63 insertions(+), 12 deletions(-)
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>