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

["joeyli" <jlee@suse.com>]

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