by Tom Lendacky

[permalink] [raw]

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

On 2/9/24 12:37, Paolo Bonzini wrote:
> The idea that no parameter would ever be necessary when enabling SEV or
> SEV-ES for a VM was decidedly optimistic. In fact, in some sense it's
> already a parameter whether SEV or SEV-ES is desired. Another possible
> source of variability is the desired set of VMSA features, as that affects
> the measurement of the VM's initial state and cannot be changed
> arbitrarily by the hypervisor.
>
> Create a new sub-operation for KVM_MEM_ENCRYPT_OP that can take a struct,
> and put the new op to work by including the VMSA features as a field of the
> struct. The existing KVM_SEV_INIT and KVM_SEV_ES_INIT use the full set of
> supported VMSA features for backwards compatibility.
>
> The struct also includes the usual bells and whistles for future
> extensibility: a flags field that must be zero for now, and some padding
> at the end.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> .../virt/kvm/x86/amd-memory-encryption.rst | 41 ++++++++++++++--
> arch/x86/include/uapi/asm/kvm.h | 10 ++++
> arch/x86/kvm/svm/sev.c | 48 +++++++++++++++++--
> 3 files changed, 92 insertions(+), 7 deletions(-)
>

..

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index acf5c45ef14e..78c52764453f 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -252,7 +252,9 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
> sev_decommission(handle);
> }
>
> -static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> + struct kvm_sev_init *data,
> + unsigned long vm_type)
> {
> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> int asid, ret;
> @@ -260,7 +262,10 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> if (kvm->created_vcpus)
> return -EINVAL;
>
> - if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM)
> + if (data->flags)
> + return -EINVAL;
> +
> + if (data->vmsa_features & ~sev_supported_vmsa_features)

An SEV guest doesn't have protected state and so it doesn't have a VMSA to
which you can apply features. So this should be:

if (vm_type != KVM_X86_SEV_VM &&
(data->vmsa_features & ~sev_supported_vmsa_features))

Thanks,
Tom

> return -EINVAL;
>
> ret = -EBUSY;
> @@ -268,8 +273,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> return ret;
>
> sev->active = true;
> - sev->es_active = argp->id == KVM_SEV_ES_INIT;
> - sev->vmsa_features = sev_supported_vmsa_features;
> + sev->es_active = (vm_type & __KVM_X86_PROTECTED_STATE_TYPE) != 0;
> + sev->vmsa_features = data->vmsa_features;
>
> asid = sev_asid_new(sev);
> if (asid < 0)
> @@ -298,6 +303,38 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> return ret;
> }
>
> +static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + struct kvm_sev_init data = {
> + .vmsa_features = sev_supported_vmsa_features,
> + };
> + unsigned long vm_type;
> +
> + if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM)
> + return -EINVAL;
> +
> + vm_type = (argp->id == KVM_SEV_INIT ? KVM_X86_SEV_VM : KVM_X86_SEV_ES_VM);
> + return __sev_guest_init(kvm, argp, &data, vm_type);
> +}
> +
> +static int sev_guest_init2(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> + struct kvm_sev_init data;
> +
> + if (!sev->need_init)
> + return -EINVAL;
> +
> + if (kvm->arch.vm_type != KVM_X86_SEV_VM &&
> + kvm->arch.vm_type != KVM_X86_SEV_ES_VM)
> + return -EINVAL;
> +
> + if (copy_from_user(&data, (void __user *)(uintptr_t)argp->data, sizeof(data)))
> + return -EFAULT;
> +
> + return __sev_guest_init(kvm, argp, &data, kvm->arch.vm_type);
> +}
> +
> static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
> {
> struct sev_data_activate activate;
> @@ -1915,6 +1952,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> case KVM_SEV_INIT:
> r = sev_guest_init(kvm, &sev_cmd);
> break;
> + case KVM_SEV_INIT2:
> + r = sev_guest_init2(kvm, &sev_cmd);
> + break;
> case KVM_SEV_LAUNCH_START:
> r = sev_launch_start(kvm, &sev_cmd);
> break;

2024-02-17 01:41:07

by Sean Christopherson

[permalink] [raw]

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

On Tue, Feb 13, 2024, Paolo Bonzini wrote:
> On Tue, Feb 13, 2024 at 3:46 AM Sean Christopherson <[email protected]> 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*