Re: [edk2-devel] [RFC PATCH v1 01/30] ArmVirtPkg: kvmtool: Add Emulated Runtime variable support

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Ard,

Thank you for the feedback.

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 10/05/2023, 12:33, "Ard Biesheuvel" <ardb@kernel.org <mailto:ardb@kernel.org>> wrote:


On Tue, 25 Apr 2023 at 18:04, Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>> wrote:
>
> Although Kvmtool supports a CFI flash interface, it is currently
> implemented using file backed support on the Host. This scenario
> requires the VMM to be within the trust boundary.
>
> In Confidential Compute Architecture the VMM is outside the trust
> boundary. For such architectures Emulated Runtime variable storage
> is desirable.
>
> Therefore, make Emulated Runtime variable storage as the default
> option and add a build flag ENABLE_CFI_FLASH to configure the
> firmware build to use the CFI Flash as the Variable storage.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>
> ---
> ArmVirtPkg/ArmVirtKvmTool.dsc | 22 +++++++++++++++++++-
> ArmVirtPkg/ArmVirtKvmTool.fdf | 4 +++-
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index d0afe1b49e250c554313c2077b89650d6f6d67cb..d2228a95726b24fe5c2edfbc84b1f5c23a85feba 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
> #


Please add a DEFINE for this variable at the start here.
And can we make it default to TRUE?
[SAMI] I have done some experiments and I think we can make this selection at runtime based on the CFI entry in the Kvmtool provided DT. I will submit a separate patch series that enables the dynamic detection of CFI flash and either uses the Nor Flash storage or Emulated Runtime variable. I can subsequently drop this patch from the RFC v2 series.
[/SAMI]

> @@ -50,7 +50,9 @@ [LibraryClasses.common]
> ArmVirtMemInfoLib|ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.inf
>
> TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
> +!ifdef ENABLE_CFI_FLASH
> VirtNorFlashPlatformLib|ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
> +!endif
>
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>
> @@ -156,6 +158,13 @@ [PcdsFixedAtBuild.common]
> #
> gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>
> +!ifndef ENABLE_CFI_FLASH


Not sure what the difference is, but we tend to use


!if $(ENABLE_CFI_FLASH) == TRUE


(and use a local DEFINE - see above)
[SAMI] I think we can drop this patch once we have dynamic detection of the CFI flash interface.





> + # Emulate Runtime Variable storage
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|TRUE
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!endif
> +
> [PcdsPatchableInModule.common]
> #
> # This will be overridden in the code
> @@ -211,6 +220,7 @@ [PcdsDynamicDefault.common]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
>
> +!ifdef ENABLE_CFI_FLASH
> # Setup Flash storage variables
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x40000
> @@ -218,6 +228,10 @@ [PcdsDynamicDefault.common]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x40000
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x40000
> +!else
> + # Emulate Runtime Variable storage
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
> +!endif
>
> ## RTC Register address in MMIO space.
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0
> @@ -263,7 +277,9 @@ [Components.common]
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
> <LibraryClasses>
> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +!ifdef ENABLE_CFI_FLASH
> NULL|EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
> +!endif
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> }
>
> @@ -271,7 +287,9 @@ [Components.common]
> MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf {
> <LibraryClasses>
> +!ifdef ENABLE_CFI_FLASH
> NULL|ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
> +!endif
> }
>
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> @@ -296,11 +314,13 @@ [Components.common]
> NULL|ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
> }
>
> +!ifdef ENABLE_CFI_FLASH
> OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf {
> <LibraryClasses>
> # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> }
> +!endif
>
> MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.fdf b/ArmVirtPkg/ArmVirtKvmTool.fdf
> index 82aff47673cb3085c91c1dd7431683c8353c16e6..8ccbccd71e134e0ea97d49380293687aca43e8b9 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.fdf
> +++ b/ArmVirtPkg/ArmVirtKvmTool.fdf
> @@ -1,5 +1,5 @@
> #
> -# 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
> #
> @@ -154,7 +154,9 @@ [FV.FvMain]
> INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
> INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> +!ifdef ENABLE_CFI_FLASH
> INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
> +!endif
>
> #
> # FAT filesystem + GPT/MBR partitioning + UDF filesystem
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
> 
>
>