1. Patchew
  2. QEMU
  3. [PATCH v4 00/17] Introduce support for IGVM files
  4. View patch

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

Roy Hopkins posted 17 patches 4 months, 3 weeks ago
Maintainers: "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>, Kashyap Chamarthy <kchamart@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Thomas Huth <thuth@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH v4 05/17] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM
Posted by Roy Hopkins 4 months, 3 weeks ago 
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(-)

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);
+        }
         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);
+        }
     } 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
Re: [PATCH v4 05/17] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM
Posted by Daniel P. Berrangé 4 months ago 
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 :|
Re: [PATCH v4 05/17] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM
Posted by Roy Hopkins 3 months, 1 week ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.