[PATCH v3 04/15] hw/core/machine: Add igvm-cfg object and processing for IGVM files

Roy Hopkins posted 15 patches 5 months 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>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.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 v3 04/15] hw/core/machine: Add igvm-cfg object and processing for IGVM files
Posted by Roy Hopkins 5 months ago
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 the 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/boards.h |  2 ++
 hw/core/machine.c   | 20 ++++++++++++++++++++
 qemu-options.hx     | 25 +++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 73ad319d7d..4c1484ba0b 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -4,6 +4,7 @@
 #define HW_BOARDS_H
 
 #include "exec/memory.h"
+#include "sysemu/igvm-cfg.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/blockdev.h"
 #include "qapi/qapi-types-machine.h"
@@ -382,6 +383,7 @@ struct MachineState {
     bool suppress_vmdesc;
     bool enable_graphics;
     ConfidentialGuestSupport *cgs;
+    IgvmCfgState *igvm;
     HostMemoryBackend *memdev;
     /*
      * convenience alias to ram_memdev_id backend memory region
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 655d75c21f..f9f879172c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1094,6 +1094,16 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "confidential-guest-support",
                                           "Set confidential guest scheme to support");
 
+#if defined(CONFIG_IGVM)
+    object_class_property_add_link(oc, "igvm-cfg",
+                                   TYPE_IGVM_CFG,
+                                   offsetof(MachineState, igvm),
+                                   object_property_allow_set_link,
+                                   OBJ_PROP_LINK_STRONG);
+    object_class_property_set_description(oc, "igvm-cfg",
+                                          "Set IGVM configuration");
+#endif
+
     /* For compatibility */
     object_class_property_add_str(oc, "memory-encryption",
         machine_get_memory_encryption, machine_set_memory_encryption);
@@ -1582,6 +1592,16 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
 
     accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
     machine_class->init(machine);
+
+#if defined(CONFIG_IGVM)
+    /* Apply guest state from IGVM if supplied */
+    if (machine->igvm) {
+        if (IGVM_CFG_GET_CLASS(machine->igvm)
+                ->process(machine->igvm, machine->cgs, &error_abort) == -1) {
+            return;
+        }
+    }
+#endif
     phase_advance(PHASE_MACHINE_INITIALIZED);
 }
 
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
Re: [PATCH v3 04/15] hw/core/machine: Add igvm-cfg object and processing for IGVM files
Posted by Daniel P. Berrangé 5 months ago
On Fri, Jun 21, 2024 at 03:29:07PM +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 the 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/boards.h |  2 ++
>  hw/core/machine.c   | 20 ++++++++++++++++++++
>  qemu-options.hx     | 25 +++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 73ad319d7d..4c1484ba0b 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -4,6 +4,7 @@
>  #define HW_BOARDS_H
>  
>  #include "exec/memory.h"
> +#include "sysemu/igvm-cfg.h"
>  #include "sysemu/hostmem.h"
>  #include "sysemu/blockdev.h"
>  #include "qapi/qapi-types-machine.h"
> @@ -382,6 +383,7 @@ struct MachineState {
>      bool suppress_vmdesc;
>      bool enable_graphics;
>      ConfidentialGuestSupport *cgs;
> +    IgvmCfgState *igvm;
>      HostMemoryBackend *memdev;
>      /*
>       * convenience alias to ram_memdev_id backend memory region
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 655d75c21f..f9f879172c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1094,6 +1094,16 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_set_description(oc, "confidential-guest-support",
>                                            "Set confidential guest scheme to support");
>  
> +#if defined(CONFIG_IGVM)
> +    object_class_property_add_link(oc, "igvm-cfg",
> +                                   TYPE_IGVM_CFG,
> +                                   offsetof(MachineState, igvm),
> +                                   object_property_allow_set_link,
> +                                   OBJ_PROP_LINK_STRONG);
> +    object_class_property_set_description(oc, "igvm-cfg",
> +                                          "Set IGVM configuration");
> +#endif
> +
>      /* For compatibility */
>      object_class_property_add_str(oc, "memory-encryption",
>          machine_get_memory_encryption, machine_set_memory_encryption);
> @@ -1582,6 +1592,16 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
>  
>      accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
>      machine_class->init(machine);
> +
> +#if defined(CONFIG_IGVM)
> +    /* Apply guest state from IGVM if supplied */
> +    if (machine->igvm) {
> +        if (IGVM_CFG_GET_CLASS(machine->igvm)
> +                ->process(machine->igvm, machine->cgs, &error_abort) == -1) {

Perhaps use error_fatal rather than error_abort, since failures here are
more likely to be user errors (incompatible igvm config), rather than QEMU
programmer bugs.

> +            return;
> +        }
> +    }
> +#endif
>      phase_advance(PHASE_MACHINE_INITIALIZED);
>  }
>  

This adds igvm-cfg for all machines, regardless of architecture target.

Are igvm files fully cross-platform portable, or should we just put
this into the TYPE_X86_MACHINE base class to limit it ?

It at least reports errors if I try to load an IGVM file with
qemu-system-aarch64 + virt type

$ ./build/qemu-system-aarch64 -object igvm-cfg,file=../buildigvm/ovmf-sev.igvm,id=igvm -machine virt,igvm-cfg=igvm
qemu-system-aarch64: IGVM file does not describe a compatible supported platform

so that's good.

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 v3 04/15] hw/core/machine: Add igvm-cfg object and processing for IGVM files
Posted by Roy Hopkins 4 months, 4 weeks ago
On Mon, 2024-06-24 at 15:00 +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 21, 2024 at 03:29:07PM +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 the 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/boards.h |  2 ++
> >  hw/core/machine.c   | 20 ++++++++++++++++++++
> >  qemu-options.hx     | 25 +++++++++++++++++++++++++
> >  3 files changed, 47 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 73ad319d7d..4c1484ba0b 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -4,6 +4,7 @@
> >  #define HW_BOARDS_H
> >  
> >  #include "exec/memory.h"
> > +#include "sysemu/igvm-cfg.h"
> >  #include "sysemu/hostmem.h"
> >  #include "sysemu/blockdev.h"
> >  #include "qapi/qapi-types-machine.h"
> > @@ -382,6 +383,7 @@ struct MachineState {
> >      bool suppress_vmdesc;
> >      bool enable_graphics;
> >      ConfidentialGuestSupport *cgs;
> > +    IgvmCfgState *igvm;
> >      HostMemoryBackend *memdev;
> >      /*
> >       * convenience alias to ram_memdev_id backend memory region
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 655d75c21f..f9f879172c 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1094,6 +1094,16 @@ static void machine_class_init(ObjectClass *oc, void
> > *data)
> >      object_class_property_set_description(oc, "confidential-guest-support",
> >                                            "Set confidential guest scheme to
> > support");
> >  
> > +#if defined(CONFIG_IGVM)
> > +    object_class_property_add_link(oc, "igvm-cfg",
> > +                                   TYPE_IGVM_CFG,
> > +                                   offsetof(MachineState, igvm),
> > +                                   object_property_allow_set_link,
> > +                                   OBJ_PROP_LINK_STRONG);
> > +    object_class_property_set_description(oc, "igvm-cfg",
> > +                                          "Set IGVM configuration");
> > +#endif
> > +
> >      /* For compatibility */
> >      object_class_property_add_str(oc, "memory-encryption",
> >          machine_get_memory_encryption, machine_set_memory_encryption);
> > @@ -1582,6 +1592,16 @@ void machine_run_board_init(MachineState *machine,
> > const char *mem_path, Error *
> >  
> >      accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
> >      machine_class->init(machine);
> > +
> > +#if defined(CONFIG_IGVM)
> > +    /* Apply guest state from IGVM if supplied */
> > +    if (machine->igvm) {
> > +        if (IGVM_CFG_GET_CLASS(machine->igvm)
> > +                ->process(machine->igvm, machine->cgs, &error_abort) == -1)
> > {
> 
> Perhaps use error_fatal rather than error_abort, since failures here are
> more likely to be user errors (incompatible igvm config), rather than QEMU
> programmer bugs.

Makes sense. I'll change it.

> 
> > +            return;
> > +        }
> > +    }
> > +#endif
> >      phase_advance(PHASE_MACHINE_INITIALIZED);
> >  }
> >  
> 
> This adds igvm-cfg for all machines, regardless of architecture target.
> 
> Are igvm files fully cross-platform portable, or should we just put
> this into the TYPE_X86_MACHINE base class to limit it ?
> 
> It at least reports errors if I try to load an IGVM file with
> qemu-system-aarch64 + virt type
> 
> $ ./build/qemu-system-aarch64 -object igvm-cfg,file=../buildigvm/ovmf-
> sev.igvm,id=igvm -machine virt,igvm-cfg=igvm
> qemu-system-aarch64: IGVM file does not describe a compatible supported
> platform
> 
> so that's good.

The IGVM specification is designed to support non X86 platforms, hence its
inclusion for all machines. Support for non-X86 is likely to result in changes
to the specification though that will impact the library we depend on.

There would obviously need to be some further implementation to support non-X86
machines in QEMU, in the same way that further implementation is required to
support other X86 confidential computing platforms such as TDX.

So, this poses the question: should we move it to TYPE_X86_MACHINE as the
current supported platforms are all on X86? Or should we leave it where it is
with a view to adding non X86 platform support with less impact later? I'd
appreciate your views on this.

> 
> With regards,
> Daniel
Re: [PATCH v3 04/15] hw/core/machine: Add igvm-cfg object and processing for IGVM files
Posted by Daniel P. Berrangé 4 months, 4 weeks ago
On Fri, Jun 28, 2024 at 12:09:59PM +0100, Roy Hopkins wrote:
> On Mon, 2024-06-24 at 15:00 +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 21, 2024 at 03:29:07PM +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 the 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/boards.h |  2 ++
> > >  hw/core/machine.c   | 20 ++++++++++++++++++++
> > >  qemu-options.hx     | 25 +++++++++++++++++++++++++
> > >  3 files changed, 47 insertions(+)

snip

> > This adds igvm-cfg for all machines, regardless of architecture target.
> > 
> > Are igvm files fully cross-platform portable, or should we just put
> > this into the TYPE_X86_MACHINE base class to limit it ?
> > 
> > It at least reports errors if I try to load an IGVM file with
> > qemu-system-aarch64 + virt type
> > 
> > $ ./build/qemu-system-aarch64 -object igvm-cfg,file=../buildigvm/ovmf-
> > sev.igvm,id=igvm -machine virt,igvm-cfg=igvm
> > qemu-system-aarch64: IGVM file does not describe a compatible supported
> > platform
> > 
> > so that's good.
> 
> The IGVM specification is designed to support non X86 platforms, hence its
> inclusion for all machines. Support for non-X86 is likely to result in changes
> to the specification though that will impact the library we depend on.
> 
> There would obviously need to be some further implementation to support non-X86
> machines in QEMU, in the same way that further implementation is required to
> support other X86 confidential computing platforms such as TDX.
> 
> So, this poses the question: should we move it to TYPE_X86_MACHINE as the
> current supported platforms are all on X86? Or should we leave it where it is
> with a view to adding non X86 platform support with less impact later? I'd
> appreciate your views on this.

The pro of putting it in the base machine class is that we don't need to
repeat the property creation in each machine as we broaden support to other
arches, I presume aarch64 is probably most likely future candidate.

The downside of putting it in the base machine class is that it limits
mgmt app ability to probe QEMU for support. ie it wouldn't be possible
to probe whether individual machines support it or not, as we broaden
QEMU support.

Then again, we will already face that limited ability to probe even on
x86, as we won't be able to query whether IGVM is SNP only, or has been
extended to TDX too.

With my libvirt hat on, probing is still probably the more important
factor, so would push towards putting the property just to individual
machine classes that definitely have been wired up to be able to use
it.

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 v3 04/15] hw/core/machine: Add igvm-cfg object and processing for IGVM files
Posted by Roy Hopkins 4 months, 3 weeks ago
On Fri, 2024-06-28 at 12:23 +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 28, 2024 at 12:09:59PM +0100, Roy Hopkins wrote:
> > On Mon, 2024-06-24 at 15:00 +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 21, 2024 at 03:29:07PM +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 the 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/boards.h |  2 ++
> > > >  hw/core/machine.c   | 20 ++++++++++++++++++++
> > > >  qemu-options.hx     | 25 +++++++++++++++++++++++++
> > > >  3 files changed, 47 insertions(+)
> 
> snip
> 
> > > This adds igvm-cfg for all machines, regardless of architecture target.
> > > 
> > > Are igvm files fully cross-platform portable, or should we just put
> > > this into the TYPE_X86_MACHINE base class to limit it ?
> > > 
> > > It at least reports errors if I try to load an IGVM file with
> > > qemu-system-aarch64 + virt type
> > > 
> > > $ ./build/qemu-system-aarch64 -object igvm-cfg,file=../buildigvm/ovmf-
> > > sev.igvm,id=igvm -machine virt,igvm-cfg=igvm
> > > qemu-system-aarch64: IGVM file does not describe a compatible supported
> > > platform
> > > 
> > > so that's good.
> > 
> > The IGVM specification is designed to support non X86 platforms, hence its
> > inclusion for all machines. Support for non-X86 is likely to result in
> > changes
> > to the specification though that will impact the library we depend on.
> > 
> > There would obviously need to be some further implementation to support non-
> > X86
> > machines in QEMU, in the same way that further implementation is required to
> > support other X86 confidential computing platforms such as TDX.
> > 
> > So, this poses the question: should we move it to TYPE_X86_MACHINE as the
> > current supported platforms are all on X86? Or should we leave it where it
> > is
> > with a view to adding non X86 platform support with less impact later? I'd
> > appreciate your views on this.
> 
> The pro of putting it in the base machine class is that we don't need to
> repeat the property creation in each machine as we broaden support to other
> arches, I presume aarch64 is probably most likely future candidate.
> 
> The downside of putting it in the base machine class is that it limits
> mgmt app ability to probe QEMU for support. ie it wouldn't be possible
> to probe whether individual machines support it or not, as we broaden
> QEMU support.
> 
> Then again, we will already face that limited ability to probe even on
> x86, as we won't be able to query whether IGVM is SNP only, or has been
> extended to TDX too.
> 
> With my libvirt hat on, probing is still probably the more important
> factor, so would push towards putting the property just to individual
> machine classes that definitely have been wired up to be able to use
> it.
> 
> With regards,
> Daniel

Ok, thanks. I'll move the instance to 'X86MachineState' and the call to 
'igvm->process()' into 'pc_q35_init()' and 'pc_init1()'.

Regards,
Roy