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

[Roy Hopkins]

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