[PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Min Xu]

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

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@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Reported-by: Joey Lee <jlee@suse.com>
Tested-by: Joey Lee <jlee@suse.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

On Wed, Mar 29, 2023 at 01:23:10PM +0800, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[joeyli]

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@intel.com>
> > 
> > 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 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

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@intel.com>
> > > 
> > > 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[joeyli]

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@intel.com>
> > > > 
> > > > 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Min Xu]

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@intel.com>
> > > > >
> > > > > 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

  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 {

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Min Xu]

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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Min Xu]

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

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[joeyli]

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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

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@redhat.com>
Date: Tue, 11 Apr 2023 11:12:37 +0200
Subject: [PATCH 1/1] OvmfPkg: call ReserveEmuVariableNvStore after
 AmdSevInitialize

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

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@redhat.com>
> Date: Tue, 11 Apr 2023 11:12:37 +0200
> Subject: [PATCH 1/1] OvmfPkg: call ReserveEmuVariableNvStore after
>   AmdSevInitialize
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   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 {

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

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
> 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

  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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

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
> 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

  Hi,

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

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

>    -drive if=pflash,format=raw,unit=0,file=/root/kernels/qemu-install/OVMF.fd,readonly=on

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

> > Can AmdSevInitialize() setup the mappings?
> 
> Is there a way to tell when OVMF.fd vs OVMF_VARS.fd/OVMF_CODE.fd is used?

Hmm, good question.  Can the guest figure what memory ranges are part
of the launch measurement?

I have a patch here (attached below) which refines flash detection and
can detect whenever varstore flash is writable or not.  I suspect that
doesn't help much though as flash probing requires mappings already
being correct.

take care,
  Gerd

commit fdab276a9f8a25f505b083b5e15180d093f515e3
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Tue Apr 4 11:25:37 2023 +0200

    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
index 82b2b70441bf..c088d560f829 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
@@ -118,8 +118,17 @@ QemuFlashDetected (
       *Ptr = OriginalUint8;
     } else if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
       DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH\n"));
-      FlashDetected = TRUE;
-      *Ptr          = READ_ARRAY_CMD;
+      *Ptr = WRITE_BYTE_CMD;
+      *Ptr = OriginalUint8;
+      *Ptr = READ_STATUS_CMD;
+      ProbeUint8 = *Ptr;
+      if (ProbeUint8 & 0x10 /* programming error */) {
+        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FLASH is readonly\n"));
+      } else {
+        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FLASH is writable\n"));
+        FlashDetected = TRUE;
+      }
+      *Ptr = READ_ARRAY_CMD;
     }
   }
 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

On 4/14/23 05:20, Gerd Hoffmann wrote:
>    Hi,
> 
>>     -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
> 
>>   In this case, only OVMF_CODE.fd will be encrypted.
>>   The fedora.fd (OVMF_VARS.fd) will be unencrypted.
> 
>>     -drive if=pflash,format=raw,unit=0,file=/root/kernels/qemu-install/OVMF.fd,readonly=on
> 
>>   In this case, OVMF.fd will be encrypted, which includes the now memory
>>   backed variable store.
> 
>>> Can AmdSevInitialize() setup the mappings?
>>
>> Is there a way to tell when OVMF.fd vs OVMF_VARS.fd/OVMF_CODE.fd is used?
> 
> Hmm, good question.  Can the guest figure what memory ranges are part
> of the launch measurement?
> 
> I have a patch here (attached below) which refines flash detection and
> can detect whenever varstore flash is writable or not.  I suspect that
> doesn't help much though as flash probing requires mappings already
> being correct.

Sorry for the delay, but, yeah, doesn't help. SEV and SEV-ES assert and 
SEV-SNP terminates because of accessing a shared page (in the RMP) as a 
private page (we don't support the generated 0x404 error code in the #VC 
handler).

Thanks,
Tom

> 
> take care,
>    Gerd
> 
> commit fdab276a9f8a25f505b083b5e15180d093f515e3
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Tue Apr 4 11:25:37 2023 +0200
> 
>      OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 82b2b70441bf..c088d560f829 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -118,8 +118,17 @@ QemuFlashDetected (
>         *Ptr = OriginalUint8;
>       } else if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
>         DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH\n"));
> -      FlashDetected = TRUE;
> -      *Ptr          = READ_ARRAY_CMD;
> +      *Ptr = WRITE_BYTE_CMD;
> +      *Ptr = OriginalUint8;
> +      *Ptr = READ_STATUS_CMD;
> +      ProbeUint8 = *Ptr;
> +      if (ProbeUint8 & 0x10 /* programming error */) {
> +        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FLASH is readonly\n"));
> +      } else {
> +        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FLASH is writable\n"));
> +        FlashDetected = TRUE;
> +      }
> +      *Ptr = READ_ARRAY_CMD;
>       }
>     }
>   
> 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

> > Hmm, good question.  Can the guest figure what memory ranges are part
> > of the launch measurement?
> > 
> > I have a patch here (attached below) which refines flash detection and
> > can detect whenever varstore flash is writable or not.  I suspect that
> > doesn't help much though as flash probing requires mappings already
> > being correct.
> 
> Sorry for the delay, but, yeah, doesn't help. SEV and SEV-ES assert and
> SEV-SNP terminates because of accessing a shared page (in the RMP) as a
> private page (we don't support the generated 0x404 error code in the #VC
> handler).

Can you try this?
https://github.com/kraxel/edk2/commits/devel/secure-boot-pcd

It moves the varstore copy from platform init to emu variable driver,
which should be late enough that sev setup should be complete.

take care,
  Gerd

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

On 4/21/23 04:18, Gerd Hoffmann wrote:
>>> Hmm, good question.  Can the guest figure what memory ranges are part
>>> of the launch measurement?
>>>
>>> I have a patch here (attached below) which refines flash detection and
>>> can detect whenever varstore flash is writable or not.  I suspect that
>>> doesn't help much though as flash probing requires mappings already
>>> being correct.
>>
>> Sorry for the delay, but, yeah, doesn't help. SEV and SEV-ES assert and
>> SEV-SNP terminates because of accessing a shared page (in the RMP) as a
>> private page (we don't support the generated 0x404 error code in the #VC
>> handler).
> 
> Can you try this?
> https://github.com/kraxel/edk2/commits/devel/secure-boot-pcd

It works for the split vars/code launch, but fails for the combined
vars/code launch:

EMU Variable FVB Started
EMU Variable FVB: Using pre-reserved block at 7FE7C000
EMU Variable FVB: Basic FV headers were invalid
EMU Variable FVB: SecureBoot: restore FV from ROM
EMU Variable FVB: Basic FV headers were invalid
ASSERT [EmuVariableFvbRuntimeDxe] /root/kernels/ovmf-gerd-build-X64/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c(781): ((BOOLEAN)(0==1))

So the mapping isn't correct at this point either.

Thanks,
Tom

> 
> It moves the varstore copy from platform init to emu variable driver,
> which should be late enough that sev setup should be complete.
> 
> take care,
>    Gerd
> 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

On Fri, Apr 21, 2023 at 03:49:27PM -0500, Tom Lendacky wrote:
> On 4/21/23 04:18, Gerd Hoffmann wrote:
> > > > Hmm, good question.  Can the guest figure what memory ranges are part
> > > > of the launch measurement?
> > > > 
> > > > I have a patch here (attached below) which refines flash detection and
> > > > can detect whenever varstore flash is writable or not.  I suspect that
> > > > doesn't help much though as flash probing requires mappings already
> > > > being correct.
> > > 
> > > Sorry for the delay, but, yeah, doesn't help. SEV and SEV-ES assert and
> > > SEV-SNP terminates because of accessing a shared page (in the RMP) as a
> > > private page (we don't support the generated 0x404 error code in the #VC
> > > handler).
> > 
> > Can you try this?
> > https://github.com/kraxel/edk2/commits/devel/secure-boot-pcd
> 
> It works for the split vars/code launch, but fails for the combined
> vars/code launch:
> 
> EMU Variable FVB Started
> EMU Variable FVB: Using pre-reserved block at 7FE7C000
> EMU Variable FVB: Basic FV headers were invalid
> EMU Variable FVB: SecureBoot: restore FV from ROM
> EMU Variable FVB: Basic FV headers were invalid
> ASSERT [EmuVariableFvbRuntimeDxe] /root/kernels/ovmf-gerd-build-X64/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c(781): ((BOOLEAN)(0==1))
> 
> So the mapping isn't correct at this point either.

Log looks like this for me, using grep -Ei '(fvb|flash|amdsev)'

Loading driver at 0x0003F022000 EntryPoint=0x0003F0245B5 AmdSevDxe.efi
Loading driver at 0x0003F6E4000 EntryPoint=0x0003F6E7035 FvbServicesRuntimeDxe.efi
QEMU Flash: Attempting flash detection at FFC00010
QemuFlashDetected => FD behaves as ROM
QemuFlashDetected => No
QEMU flash was not detected. Writable FVB is not being installed.
Loading driver at 0x0003F6D3000 EntryPoint=0x0003F6D55B9 EmuVariableFvbRuntimeDxe.efi
EMU Variable FVB Started
EMU Variable FVB: Using pre-reserved block at 3FEF4000
EMU Variable FVB: Basic FV headers were invalid
Installing FVB for EMU Variable support

So AmdSevDxe is loaded first, next comes FvbServicesRuntimeDxe, finally
EmuVariableFvbRuntimeDxe.

So AmdSev should have (in theory, in practice obviously not ...) setup
everything at that point I assume?

Failing that FvbServicesRuntimeDxe might do it as well, there actually
is some code doing so to fixup things after calling
SetMemorySpaceAttributes (see MarkIoMemoryRangeForRuntimeAccess).

Maybe that should also be called before QemuFlashInitialize() so the SEV
settings are correct when OVMF goes do flash detection?

take care,
  Gerd

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

On 4/24/23 04:45, Gerd Hoffmann wrote:
> On Fri, Apr 21, 2023 at 03:49:27PM -0500, Tom Lendacky wrote:
>> On 4/21/23 04:18, Gerd Hoffmann wrote:
>>>>> Hmm, good question.  Can the guest figure what memory ranges are part
>>>>> of the launch measurement?
>>>>>
>>>>> I have a patch here (attached below) which refines flash detection and
>>>>> can detect whenever varstore flash is writable or not.  I suspect that
>>>>> doesn't help much though as flash probing requires mappings already
>>>>> being correct.
>>>>
>>>> Sorry for the delay, but, yeah, doesn't help. SEV and SEV-ES assert and
>>>> SEV-SNP terminates because of accessing a shared page (in the RMP) as a
>>>> private page (we don't support the generated 0x404 error code in the #VC
>>>> handler).
>>>
>>> Can you try this?
>>> https://github.com/kraxel/edk2/commits/devel/secure-boot-pcd
>>
>> It works for the split vars/code launch, but fails for the combined
>> vars/code launch:
>>
>> EMU Variable FVB Started
>> EMU Variable FVB: Using pre-reserved block at 7FE7C000
>> EMU Variable FVB: Basic FV headers were invalid
>> EMU Variable FVB: SecureBoot: restore FV from ROM
>> EMU Variable FVB: Basic FV headers were invalid
>> ASSERT [EmuVariableFvbRuntimeDxe] /root/kernels/ovmf-gerd-build-X64/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c(781): ((BOOLEAN)(0==1))
>>
>> So the mapping isn't correct at this point either.
> 
> Log looks like this for me, using grep -Ei '(fvb|flash|amdsev)'
> 
> Loading driver at 0x0003F022000 EntryPoint=0x0003F0245B5 AmdSevDxe.efi
> Loading driver at 0x0003F6E4000 EntryPoint=0x0003F6E7035 FvbServicesRuntimeDxe.efi
> QEMU Flash: Attempting flash detection at FFC00010
> QemuFlashDetected => FD behaves as ROM
> QemuFlashDetected => No
> QEMU flash was not detected. Writable FVB is not being installed.
> Loading driver at 0x0003F6D3000 EntryPoint=0x0003F6D55B9 EmuVariableFvbRuntimeDxe.efi
> EMU Variable FVB Started
> EMU Variable FVB: Using pre-reserved block at 3FEF4000
> EMU Variable FVB: Basic FV headers were invalid
> Installing FVB for EMU Variable support
> 
> So AmdSevDxe is loaded first, next comes FvbServicesRuntimeDxe, finally
> EmuVariableFvbRuntimeDxe.
> 
> So AmdSev should have (in theory, in practice obviously not ...) setup
> everything at that point I assume?

I'd have to dig much deeper to see if there's a way to identify whether a 
VARS file was specified on the Qemu command line. I *think* (please 
correct me if I'm missing something) for SEV and SEV-ES it would be 
straight forward to try and access the memory as shared and check the 
headers. If they're valid, then a VARS file was specified on the command 
line and should remain mapped shared. If they aren't valid, a VARS file 
wasn't specified and you have either the full OVMF.fd file or just the 
OVMF_CODE.fd with memory backing the VARS that, in either case, should be 
mapped private.

I think the problem may come in with SNP where if the mapping isn't 
correct (shared mapping against a page that has been validated or a 
private mapping against a page that hasn't been validated) you can end up 
with #NPFs or #VCs and having to figure out what or why you are getting those.

Let me see what I can find...  I'm off the next few days so it might be a bit.

Thanks,
Tom

> 
> Failing that FvbServicesRuntimeDxe might do it as well, there actually
> is some code doing so to fixup things after calling
> SetMemorySpaceAttributes (see MarkIoMemoryRangeForRuntimeAccess).
> 
> Maybe that should also be called before QemuFlashInitialize() so the SEV
> settings are correct when OVMF goes do flash detection?
> 
> take care,
>    Gerd
> 

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Gerd Hoffmann]

  Hi,

> I'd have to dig much deeper to see if there's a way to identify whether a
> VARS file was specified on the Qemu command line. I *think* (please correct
> me if I'm missing something) for SEV and SEV-ES it would be straight forward
> to try and access the memory as shared and check the headers. If they're
> valid, then a VARS file was specified on the command line and should remain
> mapped shared. If they aren't valid, a VARS file wasn't specified and you
> have either the full OVMF.fd file or just the OVMF_CODE.fd with memory
> backing the VARS that, in either case, should be mapped private.

OVMF_CODE.fd + OVMF_VARS.fd is *identical* to just OVMF.fd, i.e. the
guest will see valid varstore headers in both cases.

The split into code part and vars part allows to (a) easily update the
code without screwing up the vars, and (b) map both with different
properties, i.e. code read-only and vars read/write.

Does the patch below help?

take care,
  Gerd

>From 3971f9453ded3032f5918dc9d181ecc0b6f97862 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 28 Apr 2023 10:34:23 +0200
Subject: [PATCH 1/1] [testing] try setup mmio in QemuFlashBeforeProbe (dxe)

---
 .../QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index d57f7ca25ccf..3a6280ab9c3a 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -37,9 +37,18 @@ QemuFlashBeforeProbe (
   IN  UINTN                 FdBlockCount
   )
 {
-  //
-  // Do nothing
-  //
+  EFI_STATUS  Status;
+
+  if (MemEncryptSevIsEnabled ()) {
+    Status = MemEncryptSevClearMmioPageEncMask (
+             0,
+             BaseAddress,
+             EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount)
+             );
+    if (EFI_ERROR(Status)) {
+      DEBUG ((DEBUG_WARN, "%a: MemEncryptSevClearMmioPageEncMask: %r\n", __func__, Status));
+    }
+  }
 }
 
 /**
-- 
2.40.0

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Lendacky, Thomas]

On 4/28/23 03:41, Gerd Hoffmann wrote:
>    Hi,
> 
>> I'd have to dig much deeper to see if there's a way to identify whether a
>> VARS file was specified on the Qemu command line. I *think* (please correct
>> me if I'm missing something) for SEV and SEV-ES it would be straight forward
>> to try and access the memory as shared and check the headers. If they're
>> valid, then a VARS file was specified on the command line and should remain
>> mapped shared. If they aren't valid, a VARS file wasn't specified and you
>> have either the full OVMF.fd file or just the OVMF_CODE.fd with memory
>> backing the VARS that, in either case, should be mapped private.
> 
> OVMF_CODE.fd + OVMF_VARS.fd is *identical* to just OVMF.fd, i.e. the
> guest will see valid varstore headers in both cases.

It is identical except in how they are mapped. With a split OVMF_CODE.fd / 
OVMF_VARS.fd, the OVMF_CODE.fd file is mapped private and the OVMF_VARS.fd 
is mapped shared because the hypervisor is updating the contents of 
OVMF_VARS.fd. With OVMF.fd, the whole file is mapped private because 
updates to the variables are not retained, so the hypervisor isn't 
updating the contents.

I'll give the patch below a try in the next day or two.

Thanks,
Tom

> 
> The split into code part and vars part allows to (a) easily update the
> code without screwing up the vars, and (b) map both with different
> properties, i.e. code read-only and vars read/write.
> 
> Does the patch below help?
> 
> take care,
>    Gerd
> 
>  From 3971f9453ded3032f5918dc9d181ecc0b6f97862 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Fri, 28 Apr 2023 10:34:23 +0200
> Subject: [PATCH 1/1] [testing] try setup mmio in QemuFlashBeforeProbe (dxe)
> 
> ---
>   .../QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index d57f7ca25ccf..3a6280ab9c3a 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -37,9 +37,18 @@ QemuFlashBeforeProbe (
>     IN  UINTN                 FdBlockCount
>     )
>   {
> -  //
> -  // Do nothing
> -  //
> +  EFI_STATUS  Status;
> +
> +  if (MemEncryptSevIsEnabled ()) {
> +    Status = MemEncryptSevClearMmioPageEncMask (
> +             0,
> +             BaseAddress,
> +             EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount)
> +             );
> +    if (EFI_ERROR(Status)) {
> +      DEBUG ((DEBUG_WARN, "%a: MemEncryptSevClearMmioPageEncMask: %r\n", __func__, Status));
> +    }
> +  }
>   }
>   
>   /**

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[joeyli]

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@intel.com>
> > > > > >
> > > > > > 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

Re: [PATCH V1 1/1] OvmfPkg/PlatformPei: Skip PlatformInitEmuVariableNvStore in SEV guest

[Min Xu]

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