Re: [PATCH v4 29/31] hw/i386/sev: Allow use of pflash in conjunction with -bios

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, May 30, 2024 at 06:16:41AM -0500, Pankaj Gupta wrote:
> From: Michael Roth <michael.roth@amd.com>
> 
> 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.

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@amd.com>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@amd.com>
> ---
>  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 :|