Re: [PATCH 09/10] KVM: SEV: introduce KVM_SEV_INIT2 operation

[Michael Roth]

On Thu, Feb 15, 2024 at 02:44:47PM +0100, Paolo Bonzini wrote:
> On 2/15/24 02:34, Michael Roth wrote:
> > > +        struct struct kvm_sev_init {
> > Missing the vm_type param here.
> 
> It can go away in the struct actually.  Also, "struct struct".
> 
> > > +If the ``KVM_X86_SEV_VMSA_FEATURES`` attribute does not exist, the hypervisor only
> > > +supports KVM_SEV_INIT and KVM_SEV_ES_INIT.  In that case the set of VMSA features is
> > > +undefined.
> > 
> > It's hard to imagine userspace implementation support for querying
> > KVM_X86_SEV_VMSA_FEATURES but still insisting on KVM_SEV_INIT.
> 
> ... except for backwards compatibility with old kernels.  For example, the
> VMM could first invoke HAS_DEVICE_ATTR, and then fall back to KVM_SEV_INIT
> after checking that the user did not explicitly request or forbid one or
> more VMSA features.

What I mean is that if userspace is modified for these checks, it's
reasonable to also inform them that only VMSA features present in
those older kernels (i.e. debug-swap) will be available via KVM_SEV_INIT,
and for anything else they will need to use KVM_SEV_INIT.

That way we can provide clear documentation on what to expect regarding
VMSA features for KVM_SEV_INIT and not have to have the "undefined"
wording: it'll never use anything other than debug-swap depending on the
module param setting.

> 
> > Maybe it
> > would be better to just lock in that VMSA_FEATURES at what is currently
> > supported: DEBUG_SWAP=on/off depending on the kvm_amd module param, and
> > then for all other features require opt-in via KVM_SEV_INIT2, and then
> > bake that into the documentation. That way way they could still reference
> > this documentation to properly calculate measurements for older/existing
> > VMM implementations.
> 
> Thinking more about it, I think all features including debug_swap should be
> disabled with the old SEV_INIT/SEV_ES_INIT.  Because the features need to
> match between the VMM and the measurement calculation, they need to be added
> explicitly on e.g. the QEMU command line.

That seems reasonable, but the main thing I was hoping to avoid was
another round of VMSA features changing out from underneath the covers
again. The module param setting is something we've needed to convey
internally/externally a good bit due to the fallout and making this
change would lead to another repeat. Not the end of the world but would
be nice to avoid if possible.

-Mike

> 
> Paolo
>