On Mon, Jun 03, 2024 at 12:55:43PM +0100, Daniel P. Berrangé wrote:
> On Thu, May 30, 2024 at 06:16:41AM -0500, Pankaj Gupta wrote:
> > From: Michael Roth <michael.roth@xxxxxxx>
> >
> > SEV-ES and SEV-SNP support OVMF images with non-volatile storage in
> > cases where the storage area is generated as a separate image as part
> > of the OVMF build process.
>
> IIUC, right now all OVMF builds for SEV/-ES/-SNP should be done as so
> called "stateless" image. ie *without* any separate NVRAM image, because
> that image will not be covered by the VM boot measurement and thus the
> NVRAM state is liable to undermine trust of the VM.
Technically both stateless and stateful are supported for SEV-ES, so
this is mainly to provide similar support for SNP. Unfortunately we
can't re-use the existing QEMU topology because of the limitations are
using read-only ROMs.
But I'm not sure it's something we have a hard requirement for, and even
then maybe there are other alternatives to consider that would offer
better security.
In any case, I think it makes total sense to decouple this from the
series and re-submit this as a separate patchset if we determine that
it's truly necessary (and if so, address Paolo's comments, and handle the
64K-alignment thing in the context of that work).
So for now maybe we should plan to drop it from qemu-coco-queue and
focus on the stateless builds for the initial code merge.
-Mike
>
> Using NVRAM for SNP is theoretically possible in future but would be
> reliant on SVSM providing a secure encryption mechanism on the storage.
>
>
>
> >
> > Currently these are exposed with unit=0 corresponding to the actual BIOS
> > image, and unit=1 corresponding to the storage image. However, pflash
> > images are mapped guest memory using read-only memslots, which are not
> > allowed in conjunction with guest_memfd-backed ranges. This makes that
> > approach unusable for SEV-SNP, where the BIOS range will be encrypted
> > and mapped as private guest_memfd-backed memory. For this reason,
> > SEV-SNP will instead rely on -bios to handle loading the BIOS image.
> >
> > To allow for pflash to still be used for the storage image, rework the
> > existing logic to remove assumptions that unit=0 contains the BIOS image
> > when SEV-SNP, so that it can instead be used to handle only the storage
> > image.
>
> Mixing both BIOS and pflash is pretty undesirable, not least because
> that setup cannot be currently represented by the firmware descriptor
> format described by docs/interop/firmware.json.
>
> So at the very least this patch is incomplete, as it would need to
> propose changes to the firmware.json to allow this setup to be expressed.
>
> I really wish we didn't have to introduce this though - is there really
> no way to make it possible to use pflash for both CODE & VARS with SNP,
> as is done with traditional VMs, so we don't diverge in setup, needing
> yet more changes up the mgmt stack ?
>
> >
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> > ---
> > hw/i386/pc_sysfw.c | 47 +++++++++++++++++++++++++++++-----------------
> > 1 file changed, 30 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index def77a442d..7f97e62b16 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -125,21 +125,10 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > }
> > }
> >
> > -/*
> > - * Map the pcms->flash[] from 4GiB downward, and realize.
> > - * Map them in descending order, i.e. pcms->flash[0] at the top,
> > - * without gaps.
> > - * Stop at the first pcms->flash[0] lacking a block backend.
> > - * Set each flash's size from its block backend. Fatal error if the
> > - * size isn't a non-zero multiple of 4KiB, or the total size exceeds
> > - * pcms->max_fw_size.
> > - *
> > - * If pcms->flash[0] has a block backend, its memory is passed to
> > - * pc_isa_bios_init(). Merging several flash devices for isa-bios is
> > - * not supported.
> > - */
> > -static void pc_system_flash_map(PCMachineState *pcms,
> > - MemoryRegion *rom_memory)
> > +static void pc_system_flash_map_partial(PCMachineState *pcms,
> > + MemoryRegion *rom_memory,
> > + hwaddr offset,
> > + bool storage_only)
> > {
> > X86MachineState *x86ms = X86_MACHINE(pcms);
> > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > @@ -154,6 +143,8 @@ static void pc_system_flash_map(PCMachineState *pcms,
> >
> > assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> >
> > + total_size = offset;
> > +
> > for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> > hwaddr gpa;
> >
> > @@ -192,7 +183,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(system_flash), &error_fatal);
> > sysbus_mmio_map(SYS_BUS_DEVICE(system_flash), 0, gpa);
> >
> > - if (i == 0) {
> > + if (i == 0 && !storage_only) {
> > flash_mem = pflash_cfi01_get_memory(system_flash);
> > if (pcmc->isa_bios_alias) {
> > x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem,
> > @@ -211,6 +202,25 @@ static void pc_system_flash_map(PCMachineState *pcms,
> > }
> > }
> >
> > +/*
> > + * Map the pcms->flash[] from 4GiB downward, and realize.
> > + * Map them in descending order, i.e. pcms->flash[0] at the top,
> > + * without gaps.
> > + * Stop at the first pcms->flash[0] lacking a block backend.
> > + * Set each flash's size from its block backend. Fatal error if the
> > + * size isn't a non-zero multiple of 4KiB, or the total size exceeds
> > + * pcms->max_fw_size.
> > + *
> > + * If pcms->flash[0] has a block backend, its memory is passed to
> > + * pc_isa_bios_init(). Merging several flash devices for isa-bios is
> > + * not supported.
> > + */
> > +static void pc_system_flash_map(PCMachineState *pcms,
> > + MemoryRegion *rom_memory)
> > +{
> > + pc_system_flash_map_partial(pcms, rom_memory, 0, false);
> > +}
> > +
> > void pc_system_firmware_init(PCMachineState *pcms,
> > MemoryRegion *rom_memory)
> > {
> > @@ -238,9 +248,12 @@ void pc_system_firmware_init(PCMachineState *pcms,
> > }
> > }
> >
> > - if (!pflash_blk[0]) {
> > + if (!pflash_blk[0] || sev_snp_enabled()) {
> > /* Machine property pflash0 not set, use ROM mode */
> > x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, false);
> > + if (sev_snp_enabled()) {
> > + pc_system_flash_map_partial(pcms, rom_memory, 3653632, true);
> > + }
> > } else {
> > if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> > /*
> > --
> > 2.34.1
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
