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

[Roy Hopkins]

On Wed, 2024-07-24 at 18:13 +0100, Daniel P. Berrangé wrote:
> 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.
> 
> [Snip]

I looked at changing this, but the `firmware` machine property is not NULL even
if no -bios arg is provided. This is because `default_machine_opts" in pc_q35.c
and pc_piixx.c both provide a default file for the firmware if not provided.
Therefore I've left this unchanged.

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