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.hopk...@suse.com>
> > ---
> > hw/i386/pc_sysfw.c | 31 ++++++++++++++++++++++++++++---
> > 1 file changed, 28 insertions(+), 3 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berra...@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
