From: Min M Xu <min.m.xu@intel.com>
QEMU command option -pflash is not supported in Tdx guest. When Tdx guest
is booted, EmuVariableFvbRuntimeDxe driver is loaded and the NvVarStore
is initialized with empty content. This patch is to initialize the
NvVarStore with the content of Configuration FV (CFV).
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 19 +++++++++++++++++++
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 2 ++
2 files changed, 21 insertions(+)
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 4fc715dc3681..96895272d806 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -717,6 +717,8 @@ FvbInitialize (
EFI_HANDLE Handle;
EFI_PHYSICAL_ADDRESS Address;
RETURN_STATUS PcdStatus;
+ UINT8 *CfvBase;
+ UINT32 CfvSize;
DEBUG ((DEBUG_INFO, "EMU Variable FVB Started\n"));
@@ -774,6 +776,23 @@ FvbInitialize (
mEmuVarsFvb.BufferPtr = Ptr;
+ //
+ // In Tdx guest the VarNvStore content should be initialized by the Configuration FV (CFV).
+ // Integrity of the CFV has been validated by TdxValidateCfv (@PlatformInitLib)
+ //
+ if (TdIsEnabled ()) {
+ CfvBase = (UINT8 *)(UINTN)PcdGet32 (PcdCfvBase);
+ CfvSize = (UINT32)PcdGet32 (PcdCfvRawDataSize);
+
+ if (CfvSize > mEmuVarsFvb.Size) {
+ DEBUG ((DEBUG_ERROR, "Size of CFV is larger than the EMU Variable FVB.\n"));
+ ASSERT (FALSE);
+ } else {
+ CopyMem (Ptr, CfvBase, CfvSize);
+ Initialize = FALSE;
+ }
+ }
+
//
// Initialize the main FV header and variable store header
//
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
index 0811545cf7b3..15e8e673e8a0 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
@@ -56,6 +56,8 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90589): https://edk2.groups.io/g/devel/message/90589
Mute This Topic: https://groups.io/mt/91835110/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-> + UINT8 *CfvBase;
> + UINT32 CfvSize;
>
> DEBUG ((DEBUG_INFO, "EMU Variable FVB Started\n"));
>
> @@ -774,6 +776,23 @@ FvbInitialize (
>
> mEmuVarsFvb.BufferPtr = Ptr;
>
> + //
> + // In Tdx guest the VarNvStore content should be initialized by the Configuration FV (CFV).
> + // Integrity of the CFV has been validated by TdxValidateCfv (@PlatformInitLib)
> + //
> + if (TdIsEnabled ()) {
> + CfvBase = (UINT8 *)(UINTN)PcdGet32 (PcdCfvBase);
> + CfvSize = (UINT32)PcdGet32 (PcdCfvRawDataSize);
> +
> + if (CfvSize > mEmuVarsFvb.Size) {
> + DEBUG ((DEBUG_ERROR, "Size of CFV is larger than the EMU Variable FVB.\n"));
> + ASSERT (FALSE);
> + } else {
> + CopyMem (Ptr, CfvBase, CfvSize);
> + Initialize = FALSE;
> + }
> + }
There is PcdEmuVariableNvStoreReserved for that. How about just
copying the store to ram, then set PcdEmuVariableNvStoreReserved
to the location and let the existing logic handle it?
Also why limit this to tdx?
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90625): https://edk2.groups.io/g/devel/message/90625
Mute This Topic: https://groups.io/mt/91835110/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On June 20, 2022 7:01 PM, Gerd Hoffman wrote:
>
> > + UINT8 *CfvBase;
> > + UINT32 CfvSize;
> >
> > DEBUG ((DEBUG_INFO, "EMU Variable FVB Started\n"));
> >
> > @@ -774,6 +776,23 @@ FvbInitialize (
> >
> > mEmuVarsFvb.BufferPtr = Ptr;
> >
> > + //
> > + // In Tdx guest the VarNvStore content should be initialized by the
> Configuration FV (CFV).
> > + // Integrity of the CFV has been validated by TdxValidateCfv
> > + (@PlatformInitLib) // if (TdIsEnabled ()) {
> > + CfvBase = (UINT8 *)(UINTN)PcdGet32 (PcdCfvBase);
> > + CfvSize = (UINT32)PcdGet32 (PcdCfvRawDataSize);
> > +
> > + if (CfvSize > mEmuVarsFvb.Size) {
> > + DEBUG ((DEBUG_ERROR, "Size of CFV is larger than the EMU Variable
> FVB.\n"));
> > + ASSERT (FALSE);
> > + } else {
> > + CopyMem (Ptr, CfvBase, CfvSize);
> > + Initialize = FALSE;
> > + }
> > + }
>
> There is PcdEmuVariableNvStoreReserved for that. How about just copying
> the store to ram, then set PcdEmuVariableNvStoreReserved to the location
> and let the existing logic handle it?
There is ReserveEmuVariableNvStore in PlatformPei/Platform.c. This function is called to allocate storage for NV Variables. PcdEmuVariableNvStoreReserved is set in that function too. So we can copy the content to that reserved storage if it is tdx guest. Then we let the exiting logic to handle it. So I would like to extract ReserveEmuVariableNvStore to PlatformReserveEmuVariableNvStore (in PlatformInitLib) and call it in both PlatformPei/Platform.c and PeilesssStartup.c.
What's your thought?
>
> Also why limit this to tdx?
Because I am not sure if other platforms need such operation. So in current stage it is limit to tdx.
Thanks
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90668): https://edk2.groups.io/g/devel/message/90668
Mute This Topic: https://groups.io/mt/91835110/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, Jun 22, 2022 at 02:02:00AM +0000, Xu, Min M wrote: > On June 20, 2022 7:01 PM, Gerd Hoffman wrote: > > > > There is PcdEmuVariableNvStoreReserved for that. How about just copying > > the store to ram, then set PcdEmuVariableNvStoreReserved to the location > > and let the existing logic handle it? > There is ReserveEmuVariableNvStore in PlatformPei/Platform.c. This > function is called to allocate storage for NV Variables. > PcdEmuVariableNvStoreReserved is set in that function too. So we can > copy the content to that reserved storage if it is tdx guest. Then we > let the exiting logic to handle it. So I would like to extract > ReserveEmuVariableNvStore to PlatformReserveEmuVariableNvStore (in > PlatformInitLib) and call it in both PlatformPei/Platform.c and > PeilesssStartup.c. Moving the ReserveEmuVariableNvStore() function to PlatformInitLib make sense. Will be a bit more than pure code motion though, we probably need a new variable in the platforminfo struct because PeilesssStartup.c can't set PCDs. Copying over the content in PlatformInitLib makes sense too, probably best as separate function. > > Also why limit this to tdx? > Because I am not sure if other platforms need such operation. So in > current stage it is limit to tdx. I think the code should copy over the varstore in case the SECURE_BOOT_ENABLE option is set. That is the actual use case and it makes sense without TDX too. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90671): https://edk2.groups.io/g/devel/message/90671 Mute This Topic: https://groups.io/mt/91835110/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On June 22, 2022 3:01 PM, Gerd Hoffmann wrote: > On Wed, Jun 22, 2022 at 02:02:00AM +0000, Xu, Min M wrote: > > On June 20, 2022 7:01 PM, Gerd Hoffman wrote: > > > > > > There is PcdEmuVariableNvStoreReserved for that. How about just > > > copying the store to ram, then set PcdEmuVariableNvStoreReserved to > > > the location and let the existing logic handle it? > > > There is ReserveEmuVariableNvStore in PlatformPei/Platform.c. This > > function is called to allocate storage for NV Variables. > > PcdEmuVariableNvStoreReserved is set in that function too. So we can > > copy the content to that reserved storage if it is tdx guest. Then we > > let the exiting logic to handle it. So I would like to extract > > ReserveEmuVariableNvStore to PlatformReserveEmuVariableNvStore (in > > PlatformInitLib) and call it in both PlatformPei/Platform.c and > > PeilesssStartup.c. > > Moving the ReserveEmuVariableNvStore() function to PlatformInitLib make > sense. Will be a bit more than pure code motion though, we probably need > a new variable in the platforminfo struct because PeilesssStartup.c can't set > PCDs. I check the current PlatformInfoHob and PcdEmuVariableNvStoreReserved has already been defined. > > Copying over the content in PlatformInitLib makes sense too, probably best > as separate function. Yes, PlatformReserveEmuVariableNvStore() will be a separated function and it returns the pointer of the allocated storage. Then this pointer can be set to either the PCD (PlatformPei) or in PlatformInfoHob (PeilessStartupLib). > > > > Also why limit this to tdx? > > Because I am not sure if other platforms need such operation. So in > > current stage it is limit to tdx. > > I think the code should copy over the varstore in case the > SECURE_BOOT_ENABLE option is set. That is the actual use case and it > makes sense without TDX too. Then we need add a build-flag in *.dsc. Do you think OvmfPkgX64.dsc and IntelTdxX64.dsc are enough? Thanks Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90672): https://edk2.groups.io/g/devel/message/90672 Mute This Topic: https://groups.io/mt/91835110/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > Copying over the content in PlatformInitLib makes sense too, probably best > > as separate function. > Yes, PlatformReserveEmuVariableNvStore() will be a separated function > and it returns the pointer of the allocated storage. Then this pointer > can be set to either the PCD (PlatformPei) or in PlatformInfoHob > (PeilessStartupLib). I mean copying over should be a separate function too, so it is up to the caller not the library itself to decide whenever the copying should happen or not. > > > > Also why limit this to tdx? > > > Because I am not sure if other platforms need such operation. So in > > > current stage it is limit to tdx. > > > > I think the code should copy over the varstore in case the > > SECURE_BOOT_ENABLE option is set. That is the actual use case and it > > makes sense without TDX too. > Then we need add a build-flag in *.dsc. Do you think OvmfPkgX64.dsc and IntelTdxX64.dsc are enough? The flag is already there ;) take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90673): https://edk2.groups.io/g/devel/message/90673 Mute This Topic: https://groups.io/mt/91835110/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On June 22, 2022 5:23 PM, Gerd Hoffmann wrote: > > > Yes, PlatformReserveEmuVariableNvStore() will be a separated function > > and it returns the pointer of the allocated storage. Then this pointer > > can be set to either the PCD (PlatformPei) or in PlatformInfoHob > > (PeilessStartupLib). > > I mean copying over should be a separate function too, so it is up to the > caller not the library itself to decide whenever the copying should happen or > not. I see. I will do that. > > > > > > Also why limit this to tdx? > > > > Because I am not sure if other platforms need such operation. So > > > > in current stage it is limit to tdx. > > > > > > I think the code should copy over the varstore in case the > > > SECURE_BOOT_ENABLE option is set. That is the actual use case and > > > it makes sense without TDX too. > > Then we need add a build-flag in *.dsc. Do you think OvmfPkgX64.dsc and > IntelTdxX64.dsc are enough? > > The flag is already there ;) SECURE_BOOT_ENABLE is not a build-flag. It can only works in .dsc files. The build-flag(SECURE_BOOT_FEATURE_ENABLED) would be defined in .dsc: [BuildOptions] !if $(SECURE_BOOT_ENABLE) == TRUE MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED !endif Then it will be used like: #ifdef SECURE_BOOT_FEATURE_ENABLED CopyOverVarStoreContent() #endif In this way, there are totally 7 dsc in OvmfPkg need updated. - IntelTdx/IntelTdxX64.dsc - CloudHv/CloudHvX64.dsc - Bhyve/BhyveX64.dsc - Microvm/MicrovmX64.dsc - OvmfPkgIa32.dsc - OvmfPkgX64.dsc - OvmfPkgIa32X64.dsc Thanks Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90706): https://edk2.groups.io/g/devel/message/90706 Mute This Topic: https://groups.io/mt/91835110/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > The flag is already there ;) > SECURE_BOOT_ENABLE is not a build-flag. It can only works in .dsc files. > The build-flag(SECURE_BOOT_FEATURE_ENABLED) would be defined in .dsc: > [BuildOptions] > !if $(SECURE_BOOT_ENABLE) == TRUE > MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED > INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED > GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED > !endif Ah, I see. I've expected that to happen automatically, similar to the the linux kernel, where all CONFIG_* variables are available to both build system and C source code. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90713): https://edk2.groups.io/g/devel/message/90713 Mute This Topic: https://groups.io/mt/91835110/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.