On Fri, Feb 09, 2024 at 01:37:41PM -0500, 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 <pbonzini@xxxxxxxxxx>
> ---
> .../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/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> index 5ed11bc16b96..a4291e7bd8ed 100644
> --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> @@ -75,15 +75,50 @@ are defined in ``<linux/psp-dev.h>``.
> KVM implements the following commands to support common lifecycle events of SEV
> guests, such as launching, running, snapshotting, migrating and decommissioning.
>
> -1. KVM_SEV_INIT
> ----------------
> +1. KVM_SEV_INIT2
> +----------------
>
> -The KVM_SEV_INIT command is used by the hypervisor to initialize the SEV platform
> +The KVM_SEV_INIT2 command is used by the hypervisor to initialize the SEV platform
> context. In a typical workflow, this command should be the first command issued.
>
> +For this command to be accepted, either KVM_X86_SEV_VM or KVM_X86_SEV_ES_VM
> +must have been passed to the KVM_CREATE_VM ioctl. A virtual machine created
> +with those machine types in turn cannot be run until KVM_SEV_INIT2 is invoked.
> +
> +Parameters: struct kvm_sev_init (in)
>
> Returns: 0 on success, -negative on error
>
> +::
> +
> + struct struct kvm_sev_init {
Missing the vm_type param here.
> + __u32 flags; /* must be 0 */
> + __u64 vmsa_features; /* initial value of features field in VMSA */
> + __u32 pad[8];
> + };
> +
> +It is an error if the hypervisor does not support any of the bits that
> +are set in ``flags`` or ``vmsa_features``.
> +
> +This command replaces the deprecated KVM_SEV_INIT and KVM_SEV_ES_INIT commands.
> +The commands did not have any parameters (the ```data``` field was unused) and
> +only work for the KVM_X86_DEFAULT_VM machine type (0).
> +
> +They behave as if:
> +
> +* the VM type is KVM_X86_SEV_VM for KVM_SEV_INIT, or KVM_X86_SEV_ES_VM for
> + KVM_SEV_ES_INIT
> +
> +* the ``flags`` field of ``struct kvm_sev_init`` is set to zero
> +
> +* the ``vmsa_features`` field of ``struct kvm_sev_init`` is set to all features
> + supported by the hypervisor (as returned by ``KVM_GET_DEVICE_ATTR`` when
> + passed group 0 and attribute id ``KVM_X86_SEV_VMSA_FEATURES``).
> +
> +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. 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.
-Mike
> +
> 2. KVM_SEV_LAUNCH_START
> -----------------------
>
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 7c46e96cfe62..6baf18335c7b 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -683,6 +683,9 @@ enum sev_cmd_id {
> /* Guest Migration Extension */
> KVM_SEV_SEND_CANCEL,
>
> + /* Second time is the charm; improved versions of the above ioctls. */
> + KVM_SEV_INIT2,
> +
> KVM_SEV_NR_MAX,
> };
>
> @@ -694,6 +697,13 @@ struct kvm_sev_cmd {
> __u32 sev_fd;
> };
>
> +struct kvm_sev_init {
> + __u32 vm_type;
> + __u32 flags;
> + __u64 vmsa_features;
> + __u32 pad[8];
> +};
> +
> struct kvm_sev_launch_start {
> __u32 handle;
> __u32 policy;
> 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)
> 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;
> --
> 2.39.0
>
>
