On Tue, Jul 09, 2024 at 11:03:19PM -0500, Michael Roth wrote:
> On Thu, Jul 04, 2024 at 11:53:33AM +0200, Paolo Bonzini wrote:
> > On Thu, Jul 4, 2024 at 11:39 AM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:
> > > > The debug_swap parameter simply could not be enabled in the old API
> > > > without breaking measurements. The new API *is the fix* to allow using
> > > > it (though QEMU doesn't have the option plumbed in yet). There is no
> > > > extensibility.
> > > >
> > > > Enabling debug_swap by default is also a thorny problem; it cannot be
> > > > enabled by default because not all CPUs support it, and also we'd have
> > > > the same problem that we cannot enable debug_swap on new machine types
> > > > without requiring a new kernel. Tying the default to the -cpu model
> > > > would work but it is confusing.
> > >
> > > Presumably we can tie it to '-cpu host' without much problem, and
> > > then just leave it as an opt-in feature flag for named CPU models.
> >
> > '-cpu host' for SEV-SNP is also problematic, since CPUID is part of
> > the measurement. It's okay for starting guests in a quick and dirty
> > manner, but it cannot be used if measurement is in use.
> >
> > It's weird to have "-cpu" provide the default for "-object", since
> > -object is created much earlier than CPUs. But then "-cpu" itself is
> > weird because it's a kind of "factory" for future objects. Maybe we
> > should redo the same exercise I did to streamline machine
> > initialization, this time focusing on -cpu/-machine/-accel, to
> > understand the various phases and where sev-{,snp-}guest
> > initialization fits.
> >
> > > > I think it's reasonable if the fix is displayed right into the error
> > > > message. It's only needed for SEV-ES though, SEV can use the old and
> > > > new APIs interchangeably.
> > >
> > > FYI currently it is proposed to unconditionally force set legacy-vm-type=true
> > > in libvirt, so QEMU guests would *never* use the new ioctl, to fix what we
> > > consider to be a QEMU / kernel guest ABI regression.
> >
> > Ok, so it's probably best to apply both this and your patch for now.
> > Later debug-swap can be enabled and will automatically disable
> > legacy-vm-type if the user left it to the default.
>
> I think this seems like the ideal default behavior, where QEMU will
> continue to stick with legacy interface unless the user specifically
> enables a new option like debug-swap that relies on KVM_SEV_INIT2.
> So I reworked this patch to make legacy-vm-type an OnOffAuto field,
> where 'auto' implements the above semantics, and 'on'/'off' continue
> to behave as they do here in v1.
>
> At the moment, since QEMU doesn't actually expose anything that requires
> KVM_SEV_INIT2 for SEV-ES, setting legacy-vm-type to 'auto' or 'on' end
> up being equivalent, but by defaulting to 'auto' things will continue to
> "just work" on the libvirt side even when new features are enabled. And
> by adding a bit of that infrastructure now it's less likely that some
> option gets exposed in a way that doesn't abide by the above semantics.
>
> So as part of v2 I switched the default for 9.1+ to 'auto'. But if
> v1+Daniel's patch is still preferred that should be fine too.
I think your v2 patch is good on its own. It avoids the issues that
concerned me and has sensible future behaviour too.
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 :|
