Re: [PATCH 00/10] KVM: SEV: allow customizing VMSA features

[Sean Christopherson <seanjc@google.com>]

On Tue, Feb 13, 2024, Paolo Bonzini wrote:
> On Tue, Feb 13, 2024 at 3:46 AM Sean Christopherson <seanjc@google.com> wrote:
> >   __u32 flags;
> >   __u32 vm_type;
> >   union {
> >         struct tdx;
> >         struct sev;
> >         struct sev_es;
> >         struct sev_snp;
> >         __u8 pad[<big size>]
> >   };
> >
> > Rinse and repeat for APIs that have a common purpose, but different payloads.
> >
> > Similar to KVM_{SET,GET}_NESTED_STATE, where the data is wildly different, and
> > there's very little overlap between {svm,vmx}_set_nested_state(), I find it quite
> > valuable to have a single set of APIs.  E.g. I don't have to translate between
> > VMX and SVM terminology when thinking about the APIs, when discussing them, etc.
> >
> > That's especially true for all this CoCo goo, where the names are ridiculously
> > divergent, and often not exactly intuitive.  E.g. LAUNCH_MEASURE reads like
> > "measure the launch", but surprise, it's "get the measurement".
> 
> I agree, but then you'd have to do things like "CPUID data is passed
> via UPDATE_DATA for SEV and INIT_VM for TDX (and probably not at all
> for pKVM)". And in one case the firmware may prefer to encrypt in
> place, in the other you cannot do that at all.
> 
> There was a reason why SVM support was not added from the beginning.
> Before adding nested get/set support for SVM, the whole nested
> virtualization was made as similar as possible in design and
> functionality to VMX. Of course it cannot be entirely the same, but
> for example they share the overall idea that pending events and L2
> state are taken from vCPU state; kvm_nested_state only stores global
> processor state (VMXON/VMCS pointers on VMX, and GIF on SVM) and,
> while in guest mode, L1 state and control bits. This ensures that the
> same userspace flow can work for both VMX and SVM. However, in this
> case we can't really control what is done in firmware.
> 
> > The effort doesn't seem huge, so long as we don't try to make the parameters
> > common across vendor code.  The list of APIs doesn't seem insurmountable (note,
> > I'm not entirely sure these are correct mappings):
> 
> While the effort isn't huge, the benefit is also pretty small, which
> comes to a second big difference with GET/SET_NESTED_STATE: because
> there is a GET ioctl, we have the possibility of retrieving the "black
> box" and passing it back. With CoCo it's anyway userspace's task to
> fill in the parameter structs. I just don't see the possibility of
> sharing any code except the final ioctl, which to be honest is not
> much to show. And the higher price might be in re-reviewing code that
> has already been reviewed, both in KVM and in userspace.

Yeah, I realize I'm probably grasping at straws.  *sigh*