[PATCH v1 0/6] ArmVirtPkg: Add dynamic CFI flash detection for Kvmtool guests

[PATCH v1 0/6] 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 2 patches in this series fix a crash due to
stack overflow which is observed when running the UEFI
shell command 'dmpstore'.

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

Sami Mujawar (6):
  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: Kvmtool: Increase primary core stack size
  ArmVirtPkg: ArmVirtQemuKernel: Increase primary core stack size

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

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

[PATCH v1 1/6] 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 v1 2/6] 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 v1 3/6] 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 v1 4/6] 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 v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size

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

Therefore, increase the primary core stack size.

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

diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index 4541d03d23e0d98915b3d3ada688c48d979b75d2..664a624fd2a30bb466a3df2103482e3e6c1f303a 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -126,7 +126,7 @@ [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdVFPEnabled|1
 !endif
 
-  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
+  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x8000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 6/6] ArmVirtPkg: ArmVirtQemuKernel: Increase primary core stack size

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

Therefore, increase the primary core stack size.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 3cb9120e4e10551a2dc283f29b7d75fa8926a273..23b0296d8c7bc01b86a592e561b62c306d2fe018 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -119,7 +119,7 @@ [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdVFPEnabled|1
 !endif
 
-  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
+  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x8000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
 !if $(NETWORK_TLS_ENABLE) == TRUE
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size

[Ard Biesheuvel]

On Thu, 18 May 2023 at 11:10, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> 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.
>
> Therefore, increase the primary core stack size.
>

Thanks for the fix. I imagine diagnosing this may not have been trivial.

However, I don't think this is the right fix tbh. Normally, SEC and
PEI run off this initial stack, and the DxeIpl PEIM is in charging of
launching the DxeCore with a full sized stack, and remapping it
non-executable as well.

These PrePi platforms take some shortcuts and apparently, one of the
consequences is that DXE and BDS run off the initial stack, which
points into the firmware image IIRC.

IOW, it would be better to explicitly allocate 128 KiB worth of
bootservices data memory and let the DxeCore run off of that.


> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmVirtPkg/ArmVirtKvmTool.dsc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index 4541d03d23e0d98915b3d3ada688c48d979b75d2..664a624fd2a30bb466a3df2103482e3e6c1f303a 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -126,7 +126,7 @@ [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
>  !endif
>
> -  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
> +  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x8000
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>

Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size

[Sami Mujawar]

Hi Ard,

Thank you for the feedback and for the pointers.

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 18/05/2023 12:01 pm, Ard Biesheuvel via groups.io wrote:
> On Thu, 18 May 2023 at 11:10, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> 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.
>>
>> Therefore, increase the primary core stack size.
>>
> Thanks for the fix. I imagine diagnosing this may not have been trivial.
>
> However, I don't think this is the right fix tbh. Normally, SEC and
> PEI run off this initial stack, and the DxeIpl PEIM is in charging of
> launching the DxeCore with a full sized stack, and remapping it
> non-executable as well.
>
> These PrePi platforms take some shortcuts and apparently, one of the
> consequences is that DXE and BDS run off the initial stack, which
> points into the firmware image IIRC.
>
> IOW, it would be better to explicitly allocate 128 KiB worth of
> bootservices data memory and let the DxeCore run off of that.

[SAMI] If the stack size is passed in LoadDxeCoreFromFv() at

https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/PrePi.c#L104, 
the code at

https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Library/PrePiLib/PrePiLib.c#L158-L182

allocates the stack and switches it. I have set the stack size to 128KB 
in the call to

LoadDxeCoreFromFv (NULL, SIZE_128KB) and it fixes the issue.

However, the PrePiLib implementation lacks the code to remap the stack 
as NonExecutable as

done by  the DxeIplPeim code at

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c#L42-L45).

I have added a call to ArmSetMemoryRegionNoExec () in PrePiLib and it 
works. However, this code would

need to go in a separate Arch specific file. I am not sure what would be 
required for other architectures but

I can submit a patch that adds an arch hook function 'ArchSetStackNx 
(UINTN StackBase, UINTN StackSize)' to

set the stack as NonExecutable and provide an implementation for Arm. 
Other architectures can similarly

implement this function.

Please let me know if this approach is ok.

[/SAMI]

>
>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmVirtPkg/ArmVirtKvmTool.dsc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
>> index 4541d03d23e0d98915b3d3ada688c48d979b75d2..664a624fd2a30bb466a3df2103482e3e6c1f303a 100644
>> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
>> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
>> @@ -126,7 +126,7 @@ [PcdsFixedAtBuild.common]
>>     gArmTokenSpaceGuid.PcdVFPEnabled|1
>>   !endif
>>
>> -  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
>> +  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x8000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>
> 
>
>

Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size

[Ard Biesheuvel]

On Thu, 18 May 2023 at 17:12, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Hi Ard,
>
> Thank you for the feedback and for the pointers.
>
> Please see my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 18/05/2023 12:01 pm, Ard Biesheuvel via groups.io wrote:
> > On Thu, 18 May 2023 at 11:10, Sami Mujawar <sami.mujawar@arm.com> wrote:
> >> 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.
> >>
> >> Therefore, increase the primary core stack size.
> >>
> > Thanks for the fix. I imagine diagnosing this may not have been trivial.
> >
> > However, I don't think this is the right fix tbh. Normally, SEC and
> > PEI run off this initial stack, and the DxeIpl PEIM is in charging of
> > launching the DxeCore with a full sized stack, and remapping it
> > non-executable as well.
> >
> > These PrePi platforms take some shortcuts and apparently, one of the
> > consequences is that DXE and BDS run off the initial stack, which
> > points into the firmware image IIRC.
> >
> > IOW, it would be better to explicitly allocate 128 KiB worth of
> > bootservices data memory and let the DxeCore run off of that.
>
> [SAMI] If the stack size is passed in LoadDxeCoreFromFv() at
>
> https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/PrePi.c#L104,
> the code at
>
> https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Library/PrePiLib/PrePiLib.c#L158-L182
>
> allocates the stack and switches it. I have set the stack size to 128KB
> in the call to
>
> LoadDxeCoreFromFv (NULL, SIZE_128KB) and it fixes the issue.
>

Fantastic, so all the code we need is already there.

> However, the PrePiLib implementation lacks the code to remap the stack
> as NonExecutable as
>
> done by  the DxeIplPeim code at
>
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c#L42-L45).
>
> I have added a call to ArmSetMemoryRegionNoExec () in PrePiLib and it
> works. However, this code would
>
> need to go in a separate Arch specific file. I am not sure what would be
> required for other architectures but
>
> I can submit a patch that adds an arch hook function 'ArchSetStackNx
> (UINTN StackBase, UINTN StackSize)' to
>
> set the stack as NonExecutable and provide an implementation for Arm.
> Other architectures can similarly
>
> implement this function.
>
> Please let me know if this approach is ok.
>

Given the wider discussion we had the other day about tightening
memory protections, I think it is important that we get this fixed but
I don't think there is any urgency to it. I sent some patches a couple
of months ago to map DxeCore code and data with tightened permissions
as well, and I think we can revisit this in that context the next time
around.

So for now, just passing the stack size as you suggested above is
sufficient IMO.

Thanks,
Ard.

Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size

[Sami Mujawar]

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

Hi Ard,

On Thu, May 18, 2023 at 08:17 AM, Ard Biesheuvel wrote:

> 
> Given the wider discussion we had the other day about tightening
> memory protections, I think it is important that we get this fixed but
> I don't think there is any urgency to it. I sent some patches a couple
> of months ago to map DxeCore code and data with tightened permissions
> as well, and I think we can revisit this in that context the next time
> around.
> 
> So for now, just passing the stack size as you suggested above is
> sufficient IMO.

[SAMI] Thanks, I will submit a v2 series with the changes.

I also observed that a similar change would be needed for ArmPlatformPkg/PrePi at
https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PrePi/PrePi.c#L164
As of now we do not enable PcdCpuStackGuard in
edk2-platforms\Platform\ARM\VExpressPkg\ArmVExpress.dsc.inc
But if this is done the same stack overflow issue is seen on the FVP model.

Considering that, should I send out a patch for ArmPlatformPkg/PrePi as well?

Also, I think it be good to enable the stack guard check in
edk2-platforms\Platform\ARM\VExpressPkg\ArmVExpress.dsc.inc.
Please let me know your thoughts about this.

[/SAMI]

Regards,

Sami Mujawar

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