Received-SPF: pass (google.com: domain of linux-kernel+bounces-66229-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date: Wed, 14 Feb 2024 19:34:15 -0600
From: Michael Roth <michael.roth@amd.com>
To: Paolo Bonzini <pbonzini@redhat.com>
CC: <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>,
	<seanjc@google.com>, <aik@amd.com>, <isaku.yamahata@intel.com>
Subject: Re: [PATCH 09/10] KVM: SEV: introduce KVM_SEV_INIT2 operation
Message-ID: <20240215013415.bmlsmt7tmebmgtkh@amd.com>
References: <20240209183743.22030-1-pbonzini@redhat.com>
 <20240209183743.22030-10-pbonzini@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240209183743.22030-10-pbonzini@redhat.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2024 01:34:38.1420
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 8cfd4b43-6d79-4181-fe1d-08dc2dc6460b
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	BL02EPF0001A0FC.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB7695

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@redhat.com>
> ---
>  .../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
> 
>