From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched
with -bios parameter, UEFI variables will be partially emulated, and
non-volatile variables may lose their contents after a reboot. This makes
the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this point the
variable store mapping is still private/encrypted, OVMF will see
ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in
SEV guest.

Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Michael Roth <michael.roth@...>
Cc: Gerd Hoffmann <kraxel@...>
Reported-by: Joey Lee <jlee@...>
Tested-by: Joey Lee <jlee@...>
Signed-off-by: Min Xu <min.m.xu@...>
---
OvmfPkg/PlatformPei/Platform.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 148240342b4b..be9ba3e00124 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -223,7 +223,20 @@ ReserveEmuVariableNvStore (
PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);

#ifdef SECURE_BOOT_FEATURE_ENABLED
- PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
+ //
+ // PlatformInitEmuVariableNvStore is called to initialize the EmuVariableNvStore
+ // with the content pointed by PcdOvmfFlashNvStorageVariableBase. This is because
+ // when OVMF is launched with -bios parameter, UEFI variables will be partially emulated,
+ // and non-volatile variables may lose their contents after a reboot. This makes the secure
+ // boot feature not working.
+ // But in SEV guest, this design doesn't work. Because at this point the variable store
+ // mapping is still private/encrypted, OVMF will see ciphertext. So we skip the call
+ // of PlatformInitEmuVariableNvStore in SEV guest.
+ //
+ if (!MemEncryptSevIsEnabled ()) {
+ PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
+ }
+
#endif

ASSERT_RETURN_ERROR (PcdStatus);
--
2.39.1.windows.1

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:

From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched
with -bios parameter, UEFI variables will be partially emulated, and
non-volatile variables may lose their contents after a reboot. This makes
the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this point the
variable store mapping is still private/encrypted, OVMF will see
ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in
SEV guest.

I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead.
Without initializing the emu var store you will not get a functional
secure boot setup anyway.

take care,
Gerd

Hi Gerd,

On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote:

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:

From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched
with -bios parameter, UEFI variables will be partially emulated, and
non-volatile variables may lose their contents after a reboot. This makes
the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this point the
variable store mapping is still private/encrypted, OVMF will see
ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in
SEV guest.

I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead.
Without initializing the emu var store you will not get a functional
secure boot setup anyway.

In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a couple
of versions. Removing it will causes problem in VM live migration. I will prefer
Min M's solution, until SEV experts found better solution.

Thank!
Joey Lee

On Fri, Mar 31, 2023 at 03:59:56PM +0800, joeyli wrote:

Hi Gerd,

On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote:

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:

From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched
with -bios parameter, UEFI variables will be partially emulated, and
non-volatile variables may lose their contents after a reboot. This makes
the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this point the
variable store mapping is still private/encrypted, OVMF will see
ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in
SEV guest.

I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead.
Without initializing the emu var store you will not get a functional
secure boot setup anyway.

In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a couple
of versions. Removing it will causes problem in VM live migration.

Hmm? qemu live-migrates the rom image too. Only after poweroff and
reboot the guest will see an updated firmware image.

I will prefer Min M's solution, until SEV experts found better
solution.

I'd prefer to not poke holes into secure boot. Re-Initializing the emu
var store from rom on each reset is also needed for security reasons in
case the efi variable store is not in smm-protected flash memory.

take care,
Gerd

On Fri, Mar 31, 2023 at 10:25:09AM +0200, Gerd Hoffmann wrote:

On Fri, Mar 31, 2023 at 03:59:56PM +0800, joeyli wrote:

Hi Gerd,

On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote:

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:

From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is launched
with -bios parameter, UEFI variables will be partially emulated, and
non-volatile variables may lose their contents after a reboot. This makes
the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this point the
variable store mapping is still private/encrypted, OVMF will see
ciphertext. So we skip the call of PlatformInitEmuVariableNvStore in
SEV guest.

I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead.
Without initializing the emu var store you will not get a functional
secure boot setup anyway.

In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a couple
of versions. Removing it will causes problem in VM live migration.

Hmm? qemu live-migrates the rom image too. Only after poweroff and
reboot the guest will see an updated firmware image.

Thanks for your explanation. Understood.

I will prefer Min M's solution, until SEV experts found better
solution.

I'd prefer to not poke holes into secure boot. Re-Initializing the emu
var store from rom on each reset is also needed for security reasons in
case the efi variable store is not in smm-protected flash memory.

I agree that the efi variable store is not secure without smm. But after
58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't work with
SEV. System just hangs in "NvVarStore FV headers were invalid."

If secure boot can not work with SEV (even it is not really secure), why
not just block the building process when SEV with SECURE_BOOT_ENABLE? At
least the issue will not happen at runtime.

Thanks
Joey Lee

On Friday, March 31, 2023 10:49 PM, Joeyli wrote:

On Fri, Mar 31, 2023 at 10:25:09AM +0200, Gerd Hoffmann wrote:

On Fri, Mar 31, 2023 at 03:59:56PM +0800, joeyli wrote:

Hi Gerd,

On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote:

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:

From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is
launched with -bios parameter, UEFI variables will be partially
emulated, and non-volatile variables may lose their contents
after a reboot. This makes the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this
point the variable store mapping is still private/encrypted,
OVMF will see ciphertext. So we skip the call of
PlatformInitEmuVariableNvStore in SEV guest.

I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead.
Without initializing the emu var store you will not get a
functional secure boot setup anyway.

In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a
couple of versions. Removing it will causes problem in VM live migration.

Hmm? qemu live-migrates the rom image too. Only after poweroff and
reboot the guest will see an updated firmware image.

Thanks for your explanation. Understood.

I will prefer Min M's solution, until SEV experts found better
solution.

I'd prefer to not poke holes into secure boot. Re-Initializing the
emu var store from rom on each reset is also needed for security
reasons in case the efi variable store is not in smm-protected flash memory.

I agree that the efi variable store is not secure without smm. But after
58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't work
with SEV. System just hangs in "NvVarStore FV headers were invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped and an error code is returned. So system will not hang.
So another solution is simply remove the ASSERT. Then an error message is dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Thanks
Min

Hi,

I agree that the efi variable store is not secure without smm. But after
58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't work
with SEV. System just hangs in "NvVarStore FV headers were invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped and an error code is returned. So system will not hang.
So another solution is simply remove the ASSERT. Then an error message is dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Maybe we just need to call ReserveEmuVariableNvStore a bit later?

take care,
Gerd

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 148240342b4b..99d40636431f 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -377,10 +377,6 @@ InitializePlatform (
InitializeRamRegions (PlatformInfoHob);

if (PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME) {
- if (!PlatformInfoHob->SmmSmramRequire) {
- ReserveEmuVariableNvStore ();
- }
-
PeiFvInitialization (PlatformInfoHob);
MemTypeInfoInitialization (PlatformInfoHob);
MemMapInitialization (PlatformInfoHob);
@@ -389,6 +385,12 @@ InitializePlatform (

InstallClearCacheCallback ();
AmdSevInitialize (PlatformInfoHob);
+
+ if (PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME &&
+ !PlatformInfoHob->SmmSmramRequire) {
+ ReserveEmuVariableNvStore ();
+ }
+
if (PlatformInfoHob->HostBridgeDevId == 0xffff) {
MiscInitializationForMicrovm (PlatformInfoHob);
} else {

On April 3, 2023 7:21 PM, Gerd Hoffmann wrote:

I agree that the efi variable store is not secure without smm. But
after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't
work with SEV. System just hangs in "NvVarStore FV headers were invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped

and an error code is returned. So system will not hang.

So another solution is simply remove the ASSERT. Then an error message is

dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Maybe we just need to call ReserveEmuVariableNvStore a bit later?

I think we can still call ReserveEmuVariableNvStore at PEI phase, but move the initialization of EmuVariableNvStore to https://github.com/tianocore/edk2/blob/master/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c#L780-L783
@Tom Lendacky At this moment, is SEV guest available to read the content from VarStore?

Thanks
Min

On 4/5/23 20:42, Xu, Min M wrote:

On April 3, 2023 7:21 PM, Gerd Hoffmann wrote:

I agree that the efi variable store is not secure without smm. But
after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't
work with SEV. System just hangs in "NvVarStore FV headers were invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped

and an error code is returned. So system will not hang.

So another solution is simply remove the ASSERT. Then an error message is

dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Maybe we just need to call ReserveEmuVariableNvStore a bit later?

I think we can still call ReserveEmuVariableNvStore at PEI phase, but move the initialization of EmuVariableNvStore to https://github.com/tianocore/edk2/blob/master/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c#L780-L783
@Tom Lendacky At this moment, is SEV guest available to read the content from VarStore?

It's quite possible. If you can work up a quick patch, I'll test it out.

Thanks,
Tom

Thanks
Min

On Friday, April 7, 2023 4:29 AM, Tom Lendacky wrote:

On 4/5/23 20:42, Xu, Min M wrote:

On April 3, 2023 7:21 PM, Gerd Hoffmann wrote:

I agree that the efi variable store is not secure without smm. But
after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE

doesn't

work with SEV. System just hangs in "NvVarStore FV headers were

invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is
skipped

and an error code is returned. So system will not hang.

So another solution is simply remove the ASSERT. Then an error
message is

dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Maybe we just need to call ReserveEmuVariableNvStore a bit later?

I think we can still call ReserveEmuVariableNvStore at PEI phase, but
move the initialization of EmuVariableNvStore to

https://github.com/tianocore/edk2/blob/master/OvmfPkg/EmuVariableFvbR
u

ntimeDxe/Fvb.c#L780-L783 @Tom Lendacky At this moment, is SEV guest
available to read the content from VarStore?

It's quite possible. If you can work up a quick patch, I'll test it out.

Yes, the patch is uploaded here https://bugzilla.tianocore.org/show_bug.cgi?id=4379#c17

Thanks
Min

On Mon, Apr 03, 2023 at 12:21:38AM +0000, Xu, Min M wrote:

On Friday, March 31, 2023 10:49 PM, Joeyli wrote:

On Fri, Mar 31, 2023 at 10:25:09AM +0200, Gerd Hoffmann wrote:

On Fri, Mar 31, 2023 at 03:59:56PM +0800, joeyli wrote:

Hi Gerd,

On Thu, Mar 30, 2023 at 09:50:53AM +0200, Gerd Hoffmann wrote:

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:

From: Min M Xu <min.m.xu@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4379

PlatformInitEmuVariableNvStore is called to initialize the
EmuVariableNvStore with the content pointed by
PcdOvmfFlashNvStorageVariableBase. This is because when OVMF is
launched with -bios parameter, UEFI variables will be partially
emulated, and non-volatile variables may lose their contents
after a reboot. This makes the secure boot feature not working.

But in SEV guest, this design doesn't work. Because at this
point the variable store mapping is still private/encrypted,
OVMF will see ciphertext. So we skip the call of
PlatformInitEmuVariableNvStore in SEV guest.

I'd suggest to simply build without -D SECURE_BOOT_ENABLE instead.
Without initializing the emu var store you will not get a
functional secure boot setup anyway.

In our case, we already shipped ovmf with -D SECURE_BOOT_ENABLE in a
couple of versions. Removing it will causes problem in VM live migration.

Hmm? qemu live-migrates the rom image too. Only after poweroff and
reboot the guest will see an updated firmware image.

Thanks for your explanation. Understood.

I will prefer Min M's solution, until SEV experts found better
solution.

I'd prefer to not poke holes into secure boot. Re-Initializing the
emu var store from rom on each reset is also needed for security
reasons in case the efi variable store is not in smm-protected flash memory.

I agree that the efi variable store is not secure without smm. But after
58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE doesn't work
with SEV. System just hangs in "NvVarStore FV headers were invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped and an error code is returned. So system will not hang.
So another solution is simply remove the ASSERT. Then an error message is dumped out and system continues.

Ah! You are right. I forgot that I enabled debug mode.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Removing ASSERT in debug mode can workaround problem. Looks that it just hide a problem.

Thanks!
Joey Lee

On April 7, 2023 5:41 PM, joeyli wrote:

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is skipped

and an error code is returned. So system will not hang.

So another solution is simply remove the ASSERT. Then an error message is

dumped out and system continues.

Ah! You are right. I forgot that I enabled debug mode.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Removing ASSERT in debug mode can workaround problem. Looks that it just
hide a problem.

@joeyli Based on Gerd's suggestion, there is another patch to fix this issue. If you can test it in your side, that will be great.
https://bugzilla.tianocore.org/attachment.cgi?id=1353&action=diff

Thanks
Min

Hi Min Xu,

On Fri, Apr 07, 2023 at 01:56:05AM +0000, Min Xu via groups.io wrote:

On Friday, April 7, 2023 4:29 AM, Tom Lendacky wrote:

On 4/5/23 20:42, Xu, Min M wrote:

On April 3, 2023 7:21 PM, Gerd Hoffmann wrote:

I agree that the efi variable store is not secure without smm. But
after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE

doesn't

work with SEV. System just hangs in "NvVarStore FV headers were

invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is
skipped

and an error code is returned. So system will not hang.

So another solution is simply remove the ASSERT. Then an error
message is

dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Maybe we just need to call ReserveEmuVariableNvStore a bit later?

I think we can still call ReserveEmuVariableNvStore at PEI phase, but
move the initialization of EmuVariableNvStore to

https://github.com/tianocore/edk2/blob/master/OvmfPkg/EmuVariableFvbR
u

ntimeDxe/Fvb.c#L780-L783 @Tom Lendacky At this moment, is SEV guest
available to read the content from VarStore?

It's quite possible. If you can work up a quick patch, I'll test it out.

Yes, the patch is uploaded here https://bugzilla.tianocore.org/show_bug.cgi?id=4379#c17

I have tested new patch. The issue is not produced, but after I applied debug
patch. Looks that the InitializeFvAndVariableStoreForSecureBoot() not be
called. I have put detail log on bugzilla.

Thanks a lot!
Joey Lee

On 4/6/23 20:56, Xu, Min M wrote:

On Friday, April 7, 2023 4:29 AM, Tom Lendacky wrote:

On 4/5/23 20:42, Xu, Min M wrote:

On April 3, 2023 7:21 PM, Gerd Hoffmann wrote:

I agree that the efi variable store is not secure without smm. But
after 58eb8517ad7b be introduced, the -D SECURE_BOOT_ENABLE

doesn't

work with SEV. System just hangs in "NvVarStore FV headers were

invalid."

Hi, Joeyli
ASSERT is triggered in DEBUG version. In RELEASE version ASSERT is
skipped

and an error code is returned. So system will not hang.

So another solution is simply remove the ASSERT. Then an error
message is

dumped out and system continues.

@Gerd Hoffmann @Tom Lendacky @joeyli What's your thought?

Maybe we just need to call ReserveEmuVariableNvStore a bit later?

I think we can still call ReserveEmuVariableNvStore at PEI phase, but
move the initialization of EmuVariableNvStore to

https://github.com/tianocore/edk2/blob/master/OvmfPkg/EmuVariableFvbR
u

ntimeDxe/Fvb.c#L780-L783 @Tom Lendacky At this moment, is SEV guest
available to read the content from VarStore?

It's quite possible. If you can work up a quick patch, I'll test it out.

Yes, the patch is uploaded here https://bugzilla.tianocore.org/show_bug.cgi?id=4379#c17

Hi Min,

Thanks for the quick turn-around, but that patch didn't work for me. I've update the bugzilla.

Thanks,
Tom

Thanks
Min

On Fri, Apr 07, 2023 at 12:00:46PM -0500, Tom Lendacky wrote:

Thanks for the quick turn-around, but that patch didn't work for me. I've
update the bugzilla.

Can you try the patch below?

thanks,
Gerd

From a9179864523d12c3dcc137f36f6ed1a2832ed22c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@...>
Date: Tue, 11 Apr 2023 11:12:37 +0200
Subject: [PATCH 1/1] OvmfPkg: call ReserveEmuVariableNvStore after
AmdSevInitialize

Signed-off-by: Gerd Hoffmann <kraxel@...>
---
OvmfPkg/PlatformPei/Platform.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index c56247e294f2..1e70c1920830 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -378,10 +378,6 @@ InitializePlatform (
InitializeRamRegions (PlatformInfoHob);

if (PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME) {
- if (!PlatformInfoHob->SmmSmramRequire) {
- ReserveEmuVariableNvStore ();
- }
-
PeiFvInitialization (PlatformInfoHob);
MemTypeInfoInitialization (PlatformInfoHob);
MemMapInitialization (PlatformInfoHob);
@@ -390,6 +386,12 @@ InitializePlatform (

InstallClearCacheCallback ();
AmdSevInitialize (PlatformInfoHob);
+
+ if ((PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME) &&
+ (!PlatformInfoHob->SmmSmramRequire)) {
+ ReserveEmuVariableNvStore ();
+ }
+
if (PlatformInfoHob->HostBridgeDevId == 0xffff) {
MiscInitializationForMicrovm (PlatformInfoHob);
} else {
--
2.39.2

On 4/11/23 05:04, Gerd Hoffmann wrote:

On Fri, Apr 07, 2023 at 12:00:46PM -0500, Tom Lendacky wrote:

Thanks for the quick turn-around, but that patch didn't work for me. I've
update the bugzilla.

Can you try the patch below?

That doesn't work either.

Specifying both OVMF_CODE.fd and OVMF_VARS.fd generates an ASSERT.
Specifying just OVMF_CODE.fd causes VMRUN failure (triple fault)
Specifying just OVMF.fd boots successfully

Thanks,
Tom

thanks,
Gerd
From a9179864523d12c3dcc137f36f6ed1a2832ed22c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@...>
Date: Tue, 11 Apr 2023 11:12:37 +0200
Subject: [PATCH 1/1] OvmfPkg: call ReserveEmuVariableNvStore after
AmdSevInitialize
Signed-off-by: Gerd Hoffmann <kraxel@...>
---
OvmfPkg/PlatformPei/Platform.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index c56247e294f2..1e70c1920830 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -378,10 +378,6 @@ InitializePlatform (
InitializeRamRegions (PlatformInfoHob);
if (PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME) {
- if (!PlatformInfoHob->SmmSmramRequire) {
- ReserveEmuVariableNvStore ();
- }
-
PeiFvInitialization (PlatformInfoHob);
MemTypeInfoInitialization (PlatformInfoHob);
MemMapInitialization (PlatformInfoHob);
@@ -390,6 +386,12 @@ InitializePlatform (
InstallClearCacheCallback ();
AmdSevInitialize (PlatformInfoHob);
+
+ if ((PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME) &&
+ (!PlatformInfoHob->SmmSmramRequire)) {
+ ReserveEmuVariableNvStore ();
+ }
+
if (PlatformInfoHob->HostBridgeDevId == 0xffff) {
MiscInitializationForMicrovm (PlatformInfoHob);
} else {

On Tue, Apr 11, 2023 at 01:03:28PM -0500, Tom Lendacky wrote:

On 4/11/23 05:04, Gerd Hoffmann wrote:

On Fri, Apr 07, 2023 at 12:00:46PM -0500, Tom Lendacky wrote:

Thanks for the quick turn-around, but that patch didn't work for me. I've
update the bugzilla.

Can you try the patch below?

That doesn't work either.

Specifying both OVMF_CODE.fd and OVMF_VARS.fd generates an ASSERT.

Both as pflash I assume? Which assert?

Specifying just OVMF_CODE.fd causes VMRUN failure (triple fault)

That's not a valid configuration anyway.

Specifying just OVMF.fd boots successfully

pflash or -bios or both?

For which cases does the patch change behavior?

take care,
Gerd

On 4/12/23 02:24, Gerd Hoffmann wrote:

On Tue, Apr 11, 2023 at 01:03:28PM -0500, Tom Lendacky wrote:

On 4/11/23 05:04, Gerd Hoffmann wrote:

On Fri, Apr 07, 2023 at 12:00:46PM -0500, Tom Lendacky wrote:

Thanks for the quick turn-around, but that patch didn't work for me. I've
update the bugzilla.

Can you try the patch below?

That doesn't work either.

Specifying both OVMF_CODE.fd and OVMF_VARS.fd generates an ASSERT.

Both as pflash I assume? Which assert?

Yes, both as pflash. I've never attempted to run an SEV guest using the
-bios option.

The assert is:
ASSERT [PlatformPei] /root/kernels/ovmf-build-X64/OvmfPkg/Library/PlatformInitLib/Platform.c(930): ((BOOLEAN)(0==1))

That happens for SEV and SEV-ES.

For SEV-SNP, it causes a VMRUN failure with a strange exit code - but
I believe it is because of accessing a page marked as shared in the RMP,
but accessed as private by the guest.

Specifying just OVMF_CODE.fd causes VMRUN failure (triple fault)

That's not a valid configuration anyway.

Right, but it has worked in the past. IIUC, it effectively ends up
creating a memory based variable store.

An SEV guest triple faults.

An SEV-ES and SEV-SNP guest asserts:
Invalid MMIO opcode (AF)
ASSERT [SecMain] /root/kernels/ovmf-build-X64/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c(507): ((BOOLEAN)(0==1))

Specifying just OVMF.fd boots successfully

pflash or -bios or both?

Just pflash. We don't support running OVMF under SEV using the -bios
option. If I try to run an SEV guest with -bios OVMF.fd, both SEV and
SEV-ES hang, while SEV-SNP returns an -EFAULT on a launch update.

I believe none of the mappings are setup properly at this point. I
think just eliminating the call for an SEV guest is fine.

Thanks,
Tom

For which cases does the patch change behavior?
take care,
Gerd

Hi,

Specifying both OVMF_CODE.fd and OVMF_VARS.fd generates an ASSERT.

Both as pflash I assume? Which assert?

Yes, both as pflash. I've never attempted to run an SEV guest using the
-bios option.

The assert is:
ASSERT [PlatformPei] /root/kernels/ovmf-build-X64/OvmfPkg/Library/PlatformInitLib/Platform.c(930): ((BOOLEAN)(0==1))

Ok, so wrong encryption settings.

Specifying just OVMF.fd boots successfully

pflash or -bios or both?

Just pflash.

/me looks surprised. It should not make a difference whenever you use
the separate OVMF_CODE.fd + OVMF_VARS.fd files or the combined OVMF.fd.

What are the exact qemu command lines for both cases?

I believe none of the mappings are setup properly at this point. I
think just eliminating the call for an SEV guest is fine.

Can AmdSevInitialize() setup the mappings?

take care,
Gerd

On 4/13/23 01:05, Gerd Hoffmann wrote:

Hi,

Specifying both OVMF_CODE.fd and OVMF_VARS.fd generates an ASSERT.

Both as pflash I assume? Which assert?

Yes, both as pflash. I've never attempted to run an SEV guest using the
-bios option.

The assert is:
ASSERT [PlatformPei] /root/kernels/ovmf-build-X64/OvmfPkg/Library/PlatformInitLib/Platform.c(930): ((BOOLEAN)(0==1))

Ok, so wrong encryption settings.

Specifying just OVMF.fd boots successfully

pflash or -bios or both?

Just pflash.

/me looks surprised. It should not make a difference whenever you use
the separate OVMF_CODE.fd + OVMF_VARS.fd files or the combined OVMF.fd.
What are the exact qemu command lines for both cases?

For the OVMF_CODE.fd/OVMF_VARS.fd case:
qemu-system-x86_64 -enable-kvm -cpu EPYC,host-phys-bits=true -smp 1
-machine type=q35,confidential-guest-support=sev0,vmport=off -m 1G
-object sev-guest,id=sev0,policy=0,cbitpos=51,reduced-phys-bits=1
-drive if=pflash,format=raw,unit=0,file=/root/kernels/qemu-install/OVMF_CODE.fd,readonly=on
-drive if=pflash,format=raw,unit=1,file=./fedora.fd
-drive file=./fedora.img,if=none,id=disk0,format=raw
-device virtio-scsi-pci,id=scsi0,disable-legacy=on,iommu_platform=true
-device scsi-hd,drive=disk0
-nographic -monitor pty -monitor unix:monitor,server,nowait
-gdb tcp::1234 -qmp tcp::4444,server,wait=off

In this case, only OVMF_CODE.fd will be encrypted.
The fedora.fd (OVMF_VARS.fd) will be unencrypted.

For the OVMF.fd case:
qemu-system-x86_64 -enable-kvm -cpu EPYC,host-phys-bits=true -smp 1
-machine type=q35,confidential-guest-support=sev0,vmport=off -m 1G
-object sev-guest,id=sev0,policy=0,cbitpos=51,reduced-phys-bits=1
-drive if=pflash,format=raw,unit=0,file=/root/kernels/qemu-install/OVMF.fd,readonly=on
-drive file=./fedora.img,if=none,id=disk0,format=raw
-device virtio-scsi-pci,id=scsi0,disable-legacy=on,iommu_platform=true
-device scsi-hd,drive=disk0
-nographic -monitor pty -monitor unix:monitor,server,nowait
-gdb tcp::1234 -qmp tcp::4444,server,wait=off

In this case, OVMF.fd will be encrypted, which includes the now memory
backed variable store.

I believe none of the mappings are setup properly at this point. I
think just eliminating the call for an SEV guest is fine.

Can AmdSevInitialize() setup the mappings?

Is there a way to tell when OVMF.fd vs OVMF_VARS.fd/OVMF_CODE.fd is used?
The reason being that the variable store of OVMF.fd must be accessed
encrypted since the whole binary was used in the LAUNCH_UPDATE. But with
the split mode, only the OVMF_CODE.fd was encrypted in the LAUNCH_UPDATE,
so the variable store must be accessed unencrypted.

If we can make that determination, then I think we can setup the mappings.

Thanks,
Tom

take care,
Gerd