Re: [PATCH v4 04/17] hw/i386: Add igvm-cfg object and processing for IGVM files

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

On Wed, Jul 03, 2024 at 12:05:42PM +0100, Roy Hopkins wrote:
> An IGVM file contains configuration of guest state that should be
> applied during configuration of the guest, before the guest is started.
> 
> This patch allows the user to add an igvm-cfg object to an X86 machine
> configuration that allows an IGVM file to be configured that will be
> applied to the guest before it is started.
> 
> If an IGVM configuration is provided then the IGVM file is processed at
> the end of the board initialization, before the state transition to
> PHASE_MACHINE_INITIALIZED.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  include/hw/i386/x86.h |  3 +++
>  hw/i386/pc.c          | 12 ++++++++++++
>  hw/i386/pc_piix.c     | 10 ++++++++++
>  hw/i386/pc_q35.c      | 10 ++++++++++
>  qemu-options.hx       | 25 +++++++++++++++++++++++++
>  5 files changed, 60 insertions(+)
> 
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index d43cb3908e..4abe3afaba 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -25,6 +25,7 @@
>  #include "hw/intc/ioapic.h"
>  #include "hw/isa/isa.h"
>  #include "qom/object.h"
> +#include "sysemu/igvm-cfg.h"
>  
>  struct X86MachineClass {
>      /*< private >*/
> @@ -97,6 +98,8 @@ struct X86MachineState {
>       * which means no limitation on the guest's bus locks.
>       */
>      uint64_t bus_lock_ratelimit;
> +
> +    IgvmCfgState *igvm;
>  };
>  
>  #define X86_MACHINE_SMM              "smm"
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 77415064c6..03d29e9e48 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1825,6 +1825,18 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_bool(oc, "fd-bootchk",
>          pc_machine_get_fd_bootchk,
>          pc_machine_set_fd_bootchk);
> +
> +#if defined(CONFIG_IGVM)
> +    object_class_property_add_link(oc, "igvm-cfg",
> +                                   TYPE_IGVM_CFG,
> +                                   offsetof(X86MachineState, igvm),
> +                                   object_property_allow_set_link,
> +                                   OBJ_PROP_LINK_STRONG);
> +    object_class_property_set_description(oc, "igvm-cfg",
> +                                          "Set IGVM configuration");
> +#endif
> +
> +
>  }
>  
>  static const TypeInfo pc_machine_info = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 9445b07b4f..d537dd50fb 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -365,6 +365,16 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>                                 x86_nvdimm_acpi_dsmio,
>                                 x86ms->fw_cfg, OBJECT(pcms));
>      }
> +
> +#if defined(CONFIG_IGVM)
> +    /* Apply guest state from IGVM if supplied */
> +    if (x86ms->igvm) {
> +        if (IGVM_CFG_GET_CLASS(x86ms->igvm)
> +                ->process(x86ms->igvm, machine->cgs, &error_fatal) == -1) {
> +            return;

g_assert_not_reached() instead of return, since &error_fatal should
ensure we never get this far.  Also suggest "< 0" rather than "== -1"
as the more common QEMU pattern.

> +        }
> +    }
> +#endif
>  }
>  
>  typedef enum PCSouthBridgeOption {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 71d3c6d122..d4d66be5a5 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -329,6 +329,16 @@ static void pc_q35_init(MachineState *machine)
>                                 x86_nvdimm_acpi_dsmio,
>                                 x86ms->fw_cfg, OBJECT(pcms));
>      }
> +
> +#if defined(CONFIG_IGVM)
> +    /* Apply guest state from IGVM if supplied */
> +    if (x86ms->igvm) {
> +        if (IGVM_CFG_GET_CLASS(x86ms->igvm)
> +                ->process(x86ms->igvm, machine->cgs, &error_fatal) == -1) {
> +            return;

Same note as for piix above.

> +        }
> +    }
> +#endif
>  }
>  
>  #define DEFINE_Q35_MACHINE(major, minor) \
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8ca7f34ef0..fd36390416 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5719,6 +5719,31 @@ SRST
>                   -machine ...,memory-encryption=sev0 \\
>                   .....
>  
> +    ``-object igvm-cfg,file=file``
> +        Create an IGVM configuration object that defines the initial state
> +        of the guest using a file in that conforms to the Independent Guest
> +        Virtual Machine (IGVM) file format.
> +
> +        The ``file`` parameter is used to specify the IGVM file to load.
> +        When provided, the IGVM file is used to populate the initial
> +        memory of the virtual machine and, depending on the platform, can
> +        define the initial processor state, memory map and parameters.
> +
> +        The IGVM file is expected to contain the firmware for the virtual
> +        machine, therefore an ``igvm-cfg`` object cannot be provided along
> +        with other ways of specifying firmware, such as the ``-bios``
> +        parameter on x86 machines.
> +
> +        e.g to launch a machine providing the firmware in an IGVM file
> +
> +        .. parsed-literal::
> +
> +             # |qemu_system_x86| \\
> +                 ...... \\
> +                 -object igvm-cfg,id=igvm0,file=bios.igvm \\
> +                 -machine ...,igvm-cfg=igvm0 \\
> +                 .....
> +
>      ``-object authz-simple,id=id,identity=string``
>          Create an authorization object that will control access to
>          network services.
> -- 
> 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 :|