Re: [PATCH v4 05/17] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM

[Daniel P . Berrangé]

On Wed, Jul 03, 2024 at 12:05:43PM +0100, Roy Hopkins wrote:
> When using an IGVM file the configuration of the system firmware is
> defined by IGVM directives contained in the file. In this case the user
> should not configure any pflash devices.
> 
> This commit skips initialization of the ROM mode when pflash0 is not set
> then checks to ensure no pflash devices have been configured when using
> IGVM, exiting with an error message if this is not the case.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  hw/i386/pc_sysfw.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index ef80281d28..f5e40b3ef6 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -219,7 +219,13 @@ void pc_system_firmware_init(PCMachineState *pcms,
>      BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
>  
>      if (!pcmc->pci_enabled) {
> -        x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, true);
> +        /*
> +         * If an IGVM file is specified then the firmware must be provided
> +         * in the IGVM file.
> +         */
> +        if (!X86_MACHINE(pcms)->igvm) {
> +            x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, 
> true);
> +        }

IIUC from looking at x86_bios_rom_init, the 'firmware' machine property
will be NULL if no -bios arg is given, and non-NULL if -bios is set,
so we can give an error message is -bios is set, while doing the right
thing if unset.

IOW I think this could look more like

        X86MachineState *x86ms = X86_MACHINE(pcms);
        if (x86ms->igvm) {
            if (x86ms->firmware) {
                error_report("Firmware ROM cannot be configured when "
                             "using IGVM");
                exit(1);
            }
        } else {
            x86_bios_rom_init(x86ms, "bios.bin", rom_memory, true);
        }

>          return;
>      }
>  
> @@ -239,8 +245,13 @@ void pc_system_firmware_init(PCMachineState *pcms,
>      }
>  
>      if (!pflash_blk[0]) {
> -        /* Machine property pflash0 not set, use ROM mode */
> -        x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, false);
> +        /*
> +         * Machine property pflash0 not set, use ROM mode unless using IGVM,
> +         * in which case the firmware must be provided by the IGVM file.
> +         */
> +        if (!X86_MACHINE(pcms)->igvm) {
> +            x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, 
> false);
> +        }

Same as earlier

>      } else {
>          if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
>              /*
> @@ -256,6 +267,20 @@ void pc_system_firmware_init(PCMachineState *pcms,
>      }
>  
>      pc_system_flash_cleanup_unused(pcms);
> +
> +    /*
> +     * The user should not have specified any pflash devices when using IGVM
> +     * to configure the guest.
> +     */
> +    if (X86_MACHINE(pcms)->igvm) {
> +        for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +            if (pcms->flash[i]) {
> +                error_report("pflash devices cannot be configured when "
> +                             "using IGVM");
> +                exit(1);
> +            }
> +        }
> +    }
>  }
>  
>  void x86_firmware_configure(hwaddr gpa, void *ptr, int size)
> -- 
> 2.43.0
> 

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