[PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Singh, Brijesh]

The command is used to create an outgoing SEV guest encryption context.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../virtual/kvm/amd-memory-encryption.rst     |  27 +++++
 arch/x86/kvm/svm.c                            | 105 ++++++++++++++++++
 include/uapi/linux/kvm.h                      |  12 ++
 3 files changed, 144 insertions(+)

diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index d18c97b4e140..0e9e1e9f9687 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -238,6 +238,33 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+10. KVM_SEV_SEND_START
+----------------------
+
+The KVM_SEV_SEND_START command can be used by the hypervisor to create an
+outgoing guest encryption context.
+
+Parameters (in): struct kvm_sev_send_start
+
+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_cert_uaddr;        /* platform certificate chain */
+                __u32 plat_cert_len;
+
+                __u64 amd_cert_uaddr;         /* AMD certificate */
+                __u32 amd_cert_len;
+
+                __u64 session_uaddr;         /* Guest session information */
+                __u32 session_len;
+        };
+
 References
 ==========
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 48c865a4e5dd..0b0937f53520 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6957,6 +6957,108 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	void *amd_cert = NULL, *session_data = NULL;
+	void *pdh_cert = NULL, *plat_cert = NULL;
+	struct sev_data_send_start *data = NULL;
+	struct kvm_sev_send_start params;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* userspace wants to query the session length */
+	if (!params.session_len)
+		goto cmd;
+
+	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
+	    !params.session_uaddr)
+		return -EINVAL;
+
+	/* copy the certificate blobs from userspace */
+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free;
+	}
+
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+
+	plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len);
+	if (IS_ERR(plat_cert)) {
+		ret = PTR_ERR(plat_cert);
+		goto e_free_pdh;
+	}
+
+	data->plat_cert_address = __psp_pa(plat_cert);
+	data->plat_cert_len = params.plat_cert_len;
+
+	amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len);
+	if (IS_ERR(amd_cert)) {
+		ret = PTR_ERR(amd_cert);
+		goto e_free_plat_cert;
+	}
+
+	data->amd_cert_address = __psp_pa(amd_cert);
+	data->amd_cert_len = params.amd_cert_len;
+
+	ret = -EINVAL;
+	if (params.session_len > SEV_FW_BLOB_MAX_SIZE)
+		goto e_free_amd_cert;
+
+	ret = -ENOMEM;
+	session_data = kmalloc(params.session_len, GFP_KERNEL);
+	if (!session_data)
+		goto e_free_amd_cert;
+
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+cmd:
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	/* if we queried the session length, FW responded with expected data */
+	if (!params.session_len)
+		goto done;
+
+	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+			session_data, params.session_len)) {
+		ret = -EFAULT;
+		goto e_free_session;
+	}
+
+	params.policy = data->policy;
+
+done:
+	params.session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+e_free_session:
+	kfree(session_data);
+e_free_amd_cert:
+	kfree(amd_cert);
+e_free_plat_cert:
+	kfree(plat_cert);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free:
+	kfree(data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -6998,6 +7100,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_LAUNCH_SECRET:
 		r = sev_launch_secret(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 2fe12b40d503..4e9e7a5b2066 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1531,6 +1531,18 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {
+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_cert_uaddr;
+	__u32 plat_cert_len;
+	__u64 amd_cert_uaddr;
+	__u32 amd_cert_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.17.1

Re: [PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:00PM +0000, Singh, Brijesh wrote:
> The command is used to create an outgoing SEV guest encryption context.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../virtual/kvm/amd-memory-encryption.rst     |  27 +++++
>  arch/x86/kvm/svm.c                            | 105 ++++++++++++++++++
>  include/uapi/linux/kvm.h                      |  12 ++
>  3 files changed, 144 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
> index d18c97b4e140..0e9e1e9f9687 100644
> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst

Do a

s/virtual/virt/g

for the next revision because this path got changed recently:

2f5947dfcaec ("Documentation: move Documentation/virtual to Documentation/virt")

> @@ -238,6 +238,33 @@ Returns: 0 on success, -negative on error
>                  __u32 trans_len;
>          };
>  
> +10. KVM_SEV_SEND_START
> +----------------------
> +
> +The KVM_SEV_SEND_START command can be used by the hypervisor to create an
> +outgoing guest encryption context.
> +
> +Parameters (in): struct kvm_sev_send_start
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +        struct kvm_sev_send_start {
> +                __u32 policy;                 /* guest policy */
> +
> +                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
> +                __u32 pdh_cert_len;
> +
> +                __u64 plat_cert_uaddr;        /* platform certificate chain */
> +                __u32 plat_cert_len;
> +
> +                __u64 amd_cert_uaddr;         /* AMD certificate */
> +                __u32 amd_cert_len;
> +
> +                __u64 session_uaddr;         /* Guest session information */
> +                __u32 session_len;
> +        };

SEV API doc has "CERT" for PDH members but "CERTS" for the others.
Judging by the description, you should do the same here too. Just so that
there's no discrepancy from the docs.

> +
>  References
>  ==========
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 48c865a4e5dd..0b0937f53520 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -6957,6 +6957,108 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return ret;
>  }
>  
> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	void *amd_cert = NULL, *session_data = NULL;
> +	void *pdh_cert = NULL, *plat_cert = NULL;
> +	struct sev_data_send_start *data = NULL;

Why are you initializing those to NULL?

Also, SEV API text on SEND_START talks about a bunch of requirements in
section

"6.8.1 Actions"

like

"The platform must be in the PSTATE.WORKING state.
The guest must be in the GSTATE.RUNNING state.
GCTX.POLICY.NOSEND must be zero. Otherwise, an error is returned.
..."

Where are we checking/verifying those?

> +	struct kvm_sev_send_start params;
> +	int ret;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;
> +
> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +				sizeof(struct kvm_sev_send_start)))
> +		return -EFAULT;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;

Move that allocation...

> +
> +	/* userspace wants to query the session length */
> +	if (!params.session_len)
> +		goto cmd;
> +
> +	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
> +	    !params.session_uaddr)
> +		return -EINVAL;
> +
> +	/* copy the certificate blobs from userspace */
> +	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len);
> +	if (IS_ERR(pdh_cert)) {
> +		ret = PTR_ERR(pdh_cert);
> +		goto e_free;
> +	}

... here so that it doesn't happen unnecessarily if the above fail.

> +
> +	data->pdh_cert_address = __psp_pa(pdh_cert);
> +	data->pdh_cert_len = params.pdh_cert_len;
> +
> +	plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len);
> +	if (IS_ERR(plat_cert)) {
> +		ret = PTR_ERR(plat_cert);
> +		goto e_free_pdh;
> +	}
> +
> +	data->plat_cert_address = __psp_pa(plat_cert);
> +	data->plat_cert_len = params.plat_cert_len;
> +
> +	amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len);
> +	if (IS_ERR(amd_cert)) {
> +		ret = PTR_ERR(amd_cert);
> +		goto e_free_plat_cert;
> +	}
> +
> +	data->amd_cert_address = __psp_pa(amd_cert);
> +	data->amd_cert_len = params.amd_cert_len;
> +
> +	ret = -EINVAL;
> +	if (params.session_len > SEV_FW_BLOB_MAX_SIZE)
> +		goto e_free_amd_cert;

That check could go up where the other params.session_len check is
happening and you can save yourself the cert alloc+freeing.

> +
> +	ret = -ENOMEM;
> +	session_data = kmalloc(params.session_len, GFP_KERNEL);
> +	if (!session_data)
> +		goto e_free_amd_cert;

Ditto.

...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Mary Higgins, Sri Rasiah, HRB 21284 (AG Nürnberg)

Re: [PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Singh, Brijesh]

On 8/22/19 5:08 AM, Borislav Petkov wrote:
> On Wed, Jul 10, 2019 at 08:13:00PM +0000, Singh, Brijesh wrote:
>> The command is used to create an outgoing SEV guest encryption context.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: x86@kernel.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   .../virtual/kvm/amd-memory-encryption.rst     |  27 +++++
>>   arch/x86/kvm/svm.c                            | 105 ++++++++++++++++++
>>   include/uapi/linux/kvm.h                      |  12 ++
>>   3 files changed, 144 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
>> index d18c97b4e140..0e9e1e9f9687 100644
>> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
>> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
> 
> Do a
> 
> s/virtual/virt/g
> 
> for the next revision because this path got changed recently:
> 
> 2f5947dfcaec ("Documentation: move Documentation/virtual to Documentation/virt")
> 
>> @@ -238,6 +238,33 @@ Returns: 0 on success, -negative on error
>>                   __u32 trans_len;
>>           };
>>   
>> +10. KVM_SEV_SEND_START
>> +----------------------
>> +
>> +The KVM_SEV_SEND_START command can be used by the hypervisor to create an
>> +outgoing guest encryption context.
>> +
>> +Parameters (in): struct kvm_sev_send_start
>> +
>> +Returns: 0 on success, -negative on error
>> +
>> +::
>> +        struct kvm_sev_send_start {
>> +                __u32 policy;                 /* guest policy */
>> +
>> +                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
>> +                __u32 pdh_cert_len;
>> +
>> +                __u64 plat_cert_uaddr;        /* platform certificate chain */
>> +                __u32 plat_cert_len;
>> +
>> +                __u64 amd_cert_uaddr;         /* AMD certificate */
>> +                __u32 amd_cert_len;
>> +
>> +                __u64 session_uaddr;         /* Guest session information */
>> +                __u32 session_len;
>> +        };
> 
> SEV API doc has "CERT" for PDH members but "CERTS" for the others.
> Judging by the description, you should do the same here too. Just so that
> there's no discrepancy from the docs.
> 

Noted.

>> +
>>   References
>>   ==========
>>   
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 48c865a4e5dd..0b0937f53520 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -6957,6 +6957,108 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   	return ret;
>>   }
>>   
>> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> +{
>> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +	void *amd_cert = NULL, *session_data = NULL;
>> +	void *pdh_cert = NULL, *plat_cert = NULL;
>> +	struct sev_data_send_start *data = NULL;
> 
> Why are you initializing those to NULL?
> 

It simplifies the error handling path where we can do kfree() without
knowing whether the buffers were allocated or not.


> Also, SEV API text on SEND_START talks about a bunch of requirements in
> section
> 
> "6.8.1 Actions"
> 
> like
> 
> "The platform must be in the PSTATE.WORKING state.
> The guest must be in the GSTATE.RUNNING state.
> GCTX.POLICY.NOSEND must be zero. Otherwise, an error is returned.
> ..."
> 
> Where are we checking/verifying those?
> 


The kernel driver does not keep track of all those SEV VM states. The
userspace application (e.g qemu) will ensure that VM is in proper
state before calling those commands. If userspace does not check states
and makes a blind calls then we let the FW error out and propagate
the correct error message to the caller so that it can take the required
action.


>> +	struct kvm_sev_send_start params;
>> +	int ret;
>> +
>> +	if (!sev_guest(kvm))
>> +		return -ENOTTY;
>> +
>> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>> +				sizeof(struct kvm_sev_send_start)))
>> +		return -EFAULT;
>> +
>> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
> 
> Move that allocation...
> 
>> +
>> +	/* userspace wants to query the session length */
>> +	if (!params.session_len)
>> +		goto cmd;
>> +
>> +	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
>> +	    !params.session_uaddr)
>> +		return -EINVAL;
>> +
>> +	/* copy the certificate blobs from userspace */
>> +	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len);
>> +	if (IS_ERR(pdh_cert)) {
>> +		ret = PTR_ERR(pdh_cert);
>> +		goto e_free;
>> +	}
> 
> ... here so that it doesn't happen unnecessarily if the above fail.
> 

Noted.

>> +
>> +	data->pdh_cert_address = __psp_pa(pdh_cert);
>> +	data->pdh_cert_len = params.pdh_cert_len;
>> +
>> +	plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len);
>> +	if (IS_ERR(plat_cert)) {
>> +		ret = PTR_ERR(plat_cert);
>> +		goto e_free_pdh;
>> +	}
>> +
>> +	data->plat_cert_address = __psp_pa(plat_cert);
>> +	data->plat_cert_len = params.plat_cert_len;
>> +
>> +	amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len);
>> +	if (IS_ERR(amd_cert)) {
>> +		ret = PTR_ERR(amd_cert);
>> +		goto e_free_plat_cert;
>> +	}
>> +
>> +	data->amd_cert_address = __psp_pa(amd_cert);
>> +	data->amd_cert_len = params.amd_cert_len;
>> +
>> +	ret = -EINVAL;
>> +	if (params.session_len > SEV_FW_BLOB_MAX_SIZE)
>> +		goto e_free_amd_cert;
> 
> That check could go up where the other params.session_len check is
> happening and you can save yourself the cert alloc+freeing.
> 

I will take a look.

>> +
>> +	ret = -ENOMEM;
>> +	session_data = kmalloc(params.session_len, GFP_KERNEL);
>> +	if (!session_data)
>> +		goto e_free_amd_cert;
> 
> Ditto.
> 
> ...
> 

Re: [PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Peter Gonda]

On Wed, Jul 10, 2019 at 1:13 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:
>
> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +       void *amd_cert = NULL, *session_data = NULL;
> +       void *pdh_cert = NULL, *plat_cert = NULL;
> +       struct sev_data_send_start *data = NULL;
> +       struct kvm_sev_send_start params;
> +       int ret;
> +
> +       if (!sev_guest(kvm))
> +               return -ENOTTY;
> +
> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +                               sizeof(struct kvm_sev_send_start)))
> +               return -EFAULT;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       /* userspace wants to query the session length */
> +       if (!params.session_len)
> +               goto cmd;
> +
> +       if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
> +           !params.session_uaddr)
> +               return -EINVAL;

I think pdh_cert is only required if the guest policy SEV bit is set.
Can pdh_cert be optional?

> +
> +       /* copy the certificate blobs from userspace */
> +       pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len);
> +       if (IS_ERR(pdh_cert)) {
> +               ret = PTR_ERR(pdh_cert);
> +               goto e_free;
> +       }
> +
> +       data->pdh_cert_address = __psp_pa(pdh_cert);
> +       data->pdh_cert_len = params.pdh_cert_len;
> +
> +       plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len);
> +       if (IS_ERR(plat_cert)) {
> +               ret = PTR_ERR(plat_cert);
> +               goto e_free_pdh;
> +       }

I think plat_cert is also only required if the guest policy SEV bit is
set. Can plat_cert also be optional?

> +
> +       data->plat_cert_address = __psp_pa(plat_cert);
> +       data->plat_cert_len = params.plat_cert_len;
> +
> +       amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len);
> +       if (IS_ERR(amd_cert)) {
> +               ret = PTR_ERR(amd_cert);
> +               goto e_free_plat_cert;
> +       }

I think amd_cert is also only required if the guest policy SEV bit is
set. Can amd_cert also be optional?

> +
> +       data->amd_cert_address = __psp_pa(amd_cert);
> +       data->amd_cert_len = params.amd_cert_len;
> +
> +       ret = -EINVAL;
> +       if (params.session_len > SEV_FW_BLOB_MAX_SIZE)
> +               goto e_free_amd_cert;
> +
> +       ret = -ENOMEM;
> +       session_data = kmalloc(params.session_len, GFP_KERNEL);
> +       if (!session_data)
> +               goto e_free_amd_cert;

This pattern of returning -EINVAL if a length is greater than
SEV_FW_BLOB_MAX_SIZE and -ENOMEM if kmalloc fails is used at
sev_launch_measure. And I think in your later patches you do similar,
did you consider factoring this out into a helper function similar to
psp_copy_user_blob?

Re: [PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Brijesh Singh]

On 11/12/19 12:35 PM, Peter Gonda wrote:
> On Wed, Jul 10, 2019 at 1:13 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:
>> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> +{
>> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +       void *amd_cert = NULL, *session_data = NULL;
>> +       void *pdh_cert = NULL, *plat_cert = NULL;
>> +       struct sev_data_send_start *data = NULL;
>> +       struct kvm_sev_send_start params;
>> +       int ret;
>> +
>> +       if (!sev_guest(kvm))
>> +               return -ENOTTY;
>> +
>> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>> +                               sizeof(struct kvm_sev_send_start)))
>> +               return -EFAULT;
>> +
>> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +       if (!data)
>> +               return -ENOMEM;
>> +
>> +       /* userspace wants to query the session length */
>> +       if (!params.session_len)
>> +               goto cmd;
>> +
>> +       if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
>> +           !params.session_uaddr)
>> +               return -EINVAL;
> I think pdh_cert is only required if the guest policy SEV bit is set.
> Can pdh_cert be optional?


We don't cache the policy information in kernel, having said so we can
try caching it during the LAUNCH_START to optimize this case. I have to
check with FW folks but I believe all those fields are required. IIRC,
When I passed NULL then SEND_START failed for me. But I double check it
and update you on this.


>
>> +
>> +       /* copy the certificate blobs from userspace */
>> +       pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len);
>> +       if (IS_ERR(pdh_cert)) {
>> +               ret = PTR_ERR(pdh_cert);
>> +               goto e_free;
>> +       }
>> +
>> +       data->pdh_cert_address = __psp_pa(pdh_cert);
>> +       data->pdh_cert_len = params.pdh_cert_len;
>> +
>> +       plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len);
>> +       if (IS_ERR(plat_cert)) {
>> +               ret = PTR_ERR(plat_cert);
>> +               goto e_free_pdh;
>> +       }
> I think plat_cert is also only required if the guest policy SEV bit is
> set. Can plat_cert also be optional?


Same as above, I believe its required.


>
>> +
>> +       data->plat_cert_address = __psp_pa(plat_cert);
>> +       data->plat_cert_len = params.plat_cert_len;
>> +
>> +       amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len);
>> +       if (IS_ERR(amd_cert)) {
>> +               ret = PTR_ERR(amd_cert);
>> +               goto e_free_plat_cert;
>> +       }
> I think amd_cert is also only required if the guest policy SEV bit is
> set. Can amd_cert also be optional?


Same as above, I believe its required. I will double check it.


>> +
>> +       data->amd_cert_address = __psp_pa(amd_cert);
>> +       data->amd_cert_len = params.amd_cert_len;
>> +
>> +       ret = -EINVAL;
>> +       if (params.session_len > SEV_FW_BLOB_MAX_SIZE)
>> +               goto e_free_amd_cert;
>> +
>> +       ret = -ENOMEM;
>> +       session_data = kmalloc(params.session_len, GFP_KERNEL);
>> +       if (!session_data)
>> +               goto e_free_amd_cert;
> This pattern of returning -EINVAL if a length is greater than
> SEV_FW_BLOB_MAX_SIZE and -ENOMEM if kmalloc fails is used at
> sev_launch_measure. And I think in your later patches you do similar,
> did you consider factoring this out into a helper function similar to
> psp_copy_user_blob?


Yes, we could factor out this check into a separate function. Let me see
what I can do in next iteration. thanks for the feedbacks.

-Brijesh

Re: [PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Peter Gonda]

On Tue, Nov 12, 2019 at 2:27 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
>
> On 11/12/19 12:35 PM, Peter Gonda wrote:
> > On Wed, Jul 10, 2019 at 1:13 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:
> >> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >> +{
> >> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> >> +       void *amd_cert = NULL, *session_data = NULL;
> >> +       void *pdh_cert = NULL, *plat_cert = NULL;
> >> +       struct sev_data_send_start *data = NULL;
> >> +       struct kvm_sev_send_start params;
> >> +       int ret;
> >> +
> >> +       if (!sev_guest(kvm))
> >> +               return -ENOTTY;
> >> +
> >> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> >> +                               sizeof(struct kvm_sev_send_start)))
> >> +               return -EFAULT;
> >> +
> >> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> >> +       if (!data)
> >> +               return -ENOMEM;
> >> +
> >> +       /* userspace wants to query the session length */
> >> +       if (!params.session_len)
> >> +               goto cmd;
> >> +
> >> +       if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
> >> +           !params.session_uaddr)
> >> +               return -EINVAL;
> > I think pdh_cert is only required if the guest policy SEV bit is set.
> > Can pdh_cert be optional?
>
>
> We don't cache the policy information in kernel, having said so we can
> try caching it during the LAUNCH_START to optimize this case. I have to
> check with FW folks but I believe all those fields are required. IIRC,
> When I passed NULL then SEND_START failed for me. But I double check it
> and update you on this.


I must have misinterpreted the this line of the spec:
"If GCTX.POLICY.SEV is 1, the PDH, PEK, CEK, ASK, and ARK certificates
are validated."
I thought that since they were not validated they were not needed.

Re: [PATCH v3 01/11] KVM: SVM: Add KVM_SEV SEND_START command

[Brijesh Singh]

On 11/14/19 1:27 PM, Peter Gonda wrote:
> On Tue, Nov 12, 2019 at 2:27 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>>
>>
>> On 11/12/19 12:35 PM, Peter Gonda wrote:
>>> On Wed, Jul 10, 2019 at 1:13 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:
>>>> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>> +{
>>>> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>> +       void *amd_cert = NULL, *session_data = NULL;
>>>> +       void *pdh_cert = NULL, *plat_cert = NULL;
>>>> +       struct sev_data_send_start *data = NULL;
>>>> +       struct kvm_sev_send_start params;
>>>> +       int ret;
>>>> +
>>>> +       if (!sev_guest(kvm))
>>>> +               return -ENOTTY;
>>>> +
>>>> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>>>> +                               sizeof(struct kvm_sev_send_start)))
>>>> +               return -EFAULT;
>>>> +
>>>> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
>>>> +       if (!data)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       /* userspace wants to query the session length */
>>>> +       if (!params.session_len)
>>>> +               goto cmd;
>>>> +
>>>> +       if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
>>>> +           !params.session_uaddr)
>>>> +               return -EINVAL;
>>> I think pdh_cert is only required if the guest policy SEV bit is set.
>>> Can pdh_cert be optional?
>>
>>
>> We don't cache the policy information in kernel, having said so we can
>> try caching it during the LAUNCH_START to optimize this case. I have to
>> check with FW folks but I believe all those fields are required. IIRC,
>> When I passed NULL then SEND_START failed for me. But I double check it
>> and update you on this.
> 
> 
> I must have misinterpreted the this line of the spec:
> "If GCTX.POLICY.SEV is 1, the PDH, PEK, CEK, ASK, and ARK certificates
> are validated."
> I thought that since they were not validated they were not needed.
> 

I have confirmed that these fields are required by the SEND_START
even when GCTX.POLICY.SEV=0. The FW needs us to pass certificate
chain, if POLICY.SEV=1 then FW will validating the chain otherwise
ignore it.

[PATCH v3 02/11] KVM: SVM: Add KVM_SEND_UPDATE_DATA command

[Singh, Brijesh]

The command is used for encrypting the guest memory region using the encryption
context created with KVM_SEV_SEND_START.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../virtual/kvm/amd-memory-encryption.rst     |  24 ++++
 arch/x86/kvm/svm.c                            | 120 +++++++++++++++++-
 include/uapi/linux/kvm.h                      |   9 ++
 3 files changed, 149 insertions(+), 4 deletions(-)

diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index 0e9e1e9f9687..060ac2316d69 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -265,6 +265,30 @@ Returns: 0 on success, -negative on error
                 __u32 session_len;
         };
 
+11. KVM_SEV_SEND_UPDATE_DATA
+----------------------------
+
+The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
+outgoing guest memory region with the encryption context creating using
+KVM_SEV_SEND_START.
+
+Parameters (in): struct kvm_sev_send_update_data
+
+Returns: 0 on success, -negative on error
+
+::
+
+        struct kvm_sev_launch_send_update_data {
+                __u64 hdr_uaddr;        /* userspace address containing the packet header */
+                __u32 hdr_len;
+
+                __u64 guest_uaddr;      /* the source memory region to be encrypted */
+                __u32 guest_len;
+
+                __u64 trans_uaddr;      /* the destition memory region  */
+                __u32 trans_len;
+        };
+
 References
 ==========
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0b0937f53520..8e815a53c420 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -418,6 +418,7 @@ enum {
 
 static unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
+static unsigned long sev_me_mask;
 static unsigned long *sev_asid_bitmap;
 #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
 
@@ -1216,16 +1217,21 @@ static int avic_ga_log_notifier(u32 ga_tag)
 static __init int sev_hardware_setup(void)
 {
 	struct sev_user_data_status *status;
+	int eax, ebx;
 	int rc;
 
-	/* Maximum number of encrypted guests supported simultaneously */
-	max_sev_asid = cpuid_ecx(0x8000001F);
+	/*
+	 * Query the memory encryption information.
+	 *  EBX:  Bit 0:5 Pagetable bit position used to indicate encryption (aka Cbit).
+	 *  ECX:  Maximum number of encrypted guests supported simultaneously.
+	 *  EDX:  Minimum ASID value that should be used for SEV guest.
+	 */
+	cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
 
 	if (!max_sev_asid)
 		return 1;
 
-	/* Minimum ASID value that should be used for SEV guest */
-	min_sev_asid = cpuid_edx(0x8000001F);
+	sev_me_mask = 1UL << (ebx & 0x3f);
 
 	/* Initialize SEV ASID bitmap */
 	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
@@ -7059,6 +7065,109 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_update_data *data;
+	struct kvm_sev_send_update_data params;
+	void *hdr = NULL, *trans_data = NULL;
+	struct page **guest_page = NULL;
+	unsigned long n;
+	int ret, offset;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			sizeof(struct kvm_sev_send_update_data)))
+		return -EFAULT;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* userspace wants to query either header or trans length */
+	if (!params.trans_len || !params.hdr_len)
+		goto cmd;
+
+	ret = -EINVAL;
+	if (!params.trans_uaddr || !params.guest_uaddr ||
+	    !params.guest_len || !params.hdr_uaddr)
+		goto e_free;
+
+	/* Check if we are crossing the page boundry */
+	ret = -EINVAL;
+	offset = params.guest_uaddr & (PAGE_SIZE - 1);
+	if ((params.guest_len + offset > PAGE_SIZE))
+		goto e_free;
+
+	ret = -ENOMEM;
+	hdr = kmalloc(params.hdr_len, GFP_KERNEL);
+	if (!hdr)
+		goto e_free;
+
+	data->hdr_address = __psp_pa(hdr);
+	data->hdr_len = params.hdr_len;
+
+	ret = -ENOMEM;
+	trans_data = kmalloc(params.trans_len, GFP_KERNEL);
+	if (!trans_data)
+		goto e_free;
+
+	data->trans_address = __psp_pa(trans_data);
+	data->trans_len = params.trans_len;
+
+	/* Pin guest memory */
+	ret = -EFAULT;
+	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
+				    PAGE_SIZE, &n, 0);
+	if (!guest_page)
+		goto e_free;
+
+	/* The SEND_UPDATE_DATA command requires C-bit to be always set. */
+	data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
+	data->guest_address |= sev_me_mask;
+	data->guest_len = params.guest_len;
+
+cmd:
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
+
+	/* userspace asked for header or trans length and FW responded with data */
+	if (!params.trans_len || !params.hdr_len) {
+		params.hdr_len = data->hdr_len;
+		params.trans_len = data->trans_len;
+		goto done;
+	}
+
+	if (ret)
+		goto e_unpin;
+
+	/* copy transport buffer to user space */
+	if (copy_to_user((void __user *)(uintptr_t)params.trans_uaddr,
+			 trans_data, params.trans_len)) {
+		ret = -EFAULT;
+		goto e_unpin;
+	}
+
+	/* copy packet header to userspace */
+	if (copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr, params.hdr_len))
+		ret = -EFAULT;
+
+e_unpin:
+	sev_unpin_memory(kvm, guest_page, n);
+done:
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+			sizeof(struct kvm_sev_send_update_data)))
+		ret = -EFAULT;
+e_free:
+	kfree(data);
+	kfree(trans_data);
+	kfree(hdr);
+
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7103,6 +7212,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SEND_START:
 		r = sev_send_start(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_UPDATE_DATA:
+		r = sev_send_update_data(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4e9e7a5b2066..4cb6c3774ec2 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1543,6 +1543,15 @@ struct kvm_sev_send_start {
 	__u32 session_len;
 };
 
+struct kvm_sev_send_update_data {
+	__u64 hdr_uaddr;
+	__u32 hdr_len;
+	__u64 guest_uaddr;
+	__u32 guest_len;
+	__u64 trans_uaddr;
+	__u32 trans_len;
+};
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.17.1

Re: [PATCH v3 02/11] KVM: SVM: Add KVM_SEND_UPDATE_DATA command

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:01PM +0000, Singh, Brijesh wrote:
> The command is used for encrypting the guest memory region using the encryption
> context created with KVM_SEV_SEND_START.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../virtual/kvm/amd-memory-encryption.rst     |  24 ++++
>  arch/x86/kvm/svm.c                            | 120 +++++++++++++++++-
>  include/uapi/linux/kvm.h                      |   9 ++
>  3 files changed, 149 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
> index 0e9e1e9f9687..060ac2316d69 100644
> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
> @@ -265,6 +265,30 @@ Returns: 0 on success, -negative on error
>                  __u32 session_len;
>          };
>  
> +11. KVM_SEV_SEND_UPDATE_DATA
> +----------------------------
> +
> +The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
> +outgoing guest memory region with the encryption context creating using

s/creating/created/

> +KVM_SEV_SEND_START.
> +
> +Parameters (in): struct kvm_sev_send_update_data
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +
> +        struct kvm_sev_launch_send_update_data {
> +                __u64 hdr_uaddr;        /* userspace address containing the packet header */
> +                __u32 hdr_len;
> +
> +                __u64 guest_uaddr;      /* the source memory region to be encrypted */
> +                __u32 guest_len;
> +
> +                __u64 trans_uaddr;      /* the destition memory region  */

s/destition/destination/

> +                __u32 trans_len;

Those addresses are all system physical addresses, according to the doc.
Why do you call them "uaddr"?

> +        };
> +
>  References
>  ==========
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0b0937f53520..8e815a53c420 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -418,6 +418,7 @@ enum {
>  
>  static unsigned int max_sev_asid;
>  static unsigned int min_sev_asid;
> +static unsigned long sev_me_mask;
>  static unsigned long *sev_asid_bitmap;
>  #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
>  
> @@ -1216,16 +1217,21 @@ static int avic_ga_log_notifier(u32 ga_tag)
>  static __init int sev_hardware_setup(void)
>  {
>  	struct sev_user_data_status *status;
> +	int eax, ebx;
>  	int rc;
>  
> -	/* Maximum number of encrypted guests supported simultaneously */
> -	max_sev_asid = cpuid_ecx(0x8000001F);
> +	/*
> +	 * Query the memory encryption information.
> +	 *  EBX:  Bit 0:5 Pagetable bit position used to indicate encryption (aka Cbit).
> +	 *  ECX:  Maximum number of encrypted guests supported simultaneously.
> +	 *  EDX:  Minimum ASID value that should be used for SEV guest.
> +	 */
> +	cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
>  
>  	if (!max_sev_asid)
>  		return 1;
>  
> -	/* Minimum ASID value that should be used for SEV guest */
> -	min_sev_asid = cpuid_edx(0x8000001F);
> +	sev_me_mask = 1UL << (ebx & 0x3f);
>  
>  	/* Initialize SEV ASID bitmap */
>  	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
> @@ -7059,6 +7065,109 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return ret;
>  }
>  
> +static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct sev_data_send_update_data *data;
> +	struct kvm_sev_send_update_data params;
> +	void *hdr = NULL, *trans_data = NULL;
> +	struct page **guest_page = NULL;

Ah, I see why you do init them to NULL - -Wmaybe-uninitialized. See below.

> +	unsigned long n;
> +	int ret, offset;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;
> +
> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +			sizeof(struct kvm_sev_send_update_data)))
> +		return -EFAULT;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	/* userspace wants to query either header or trans length */
> +	if (!params.trans_len || !params.hdr_len)
> +		goto cmd;
> +
> +	ret = -EINVAL;
> +	if (!params.trans_uaddr || !params.guest_uaddr ||
> +	    !params.guest_len || !params.hdr_uaddr)
> +		goto e_free;
> +
> +	/* Check if we are crossing the page boundry */

WARNING: 'boundry' may be misspelled - perhaps 'boundary'?

So the fact that you have to init local variables to NULL means that gcc
doesn't see the that kfree() can take a NULL.

But also, you can restructure your labels in a way so that gcc sees them
properly and doesn't issue the warning even without having to init those
local variables.

And also, you can cleanup that function and split out the header and
trans length query functionality into a separate helper and this way
make it a lot more readable. I gave it a try here and it looks more
readable to me but this could be just me.

I could've missed some case too... pasting the whole thing for easier
review than as a diff:


---
/* Userspace wants to query either header or trans length. */
static int
__sev_send_update_data_query_lengths(struct kvm *kvm, struct kvm_sev_cmd *argp,
				     struct kvm_sev_send_update_data *params)
{
	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
	struct sev_data_send_update_data data;

	memset(&data, 0, sizeof(data));

	data.handle = sev->handle;
	sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, &data, &argp->error);

	params->hdr_len   = data.hdr_len;
	params->trans_len = data.trans_len;

	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
			 sizeof(struct kvm_sev_send_update_data)))
		return -EFAULT;

	return 0;
}

static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
	struct sev_data_send_update_data *data;
	struct kvm_sev_send_update_data params;
	struct page **guest_page;
	void *hdr, *trans_data;
	unsigned long n;
	int ret, offset;

	if (!sev_guest(kvm))
		return -ENOTTY;

	if (copy_from_user(&params,
			   (void __user *)(uintptr_t)argp->data,
			   sizeof(struct kvm_sev_send_update_data)))
		return -EFAULT;

	/* Userspace wants to query either header or trans length */
	if (!params.trans_len || !params.hdr_len)
		return __sev_send_update_data_query_lengths(kvm, argp, &params);

	if (!params.trans_uaddr || !params.guest_uaddr ||
	    !params.guest_len || !params.hdr_uaddr)
		return -EINVAL;

	/* Check if we are crossing the page boundary: */
	offset = params.guest_uaddr & (PAGE_SIZE - 1);
	if ((params.guest_len + offset > PAGE_SIZE))
		return -EINVAL;

	hdr = kmalloc(params.hdr_len, GFP_KERNEL);
	if (!hdr)
		return -ENOMEM;

	ret = -ENOMEM;
	trans_data = kmalloc(params.trans_len, GFP_KERNEL);
	if (!trans_data)
		goto free_hdr;

	data = kzalloc(sizeof(*data), GFP_KERNEL);
        if (!data)
                goto free_trans;

	/* Pin guest memory */
	ret = -EFAULT;
	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK, PAGE_SIZE, &n, 0);
	if (!guest_page)
		goto free_data;

	/* The SEND_UPDATE_DATA command requires C-bit to be always set. */
	data->guest_address	= (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
	data->guest_address     |= sev_me_mask;
	data->guest_len		= params.guest_len;
	data->hdr_address	= __psp_pa(hdr);
	data->hdr_len		= params.hdr_len;
	data->trans_address	= __psp_pa(trans_data);
	data->trans_len		= params.trans_len;
	data->handle		= sev->handle;

	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
	if (ret)
		goto unpin_memory;

	/* Copy transport buffer to user space. */
	ret = copy_to_user((void __user *)(uintptr_t)params.trans_uaddr, trans_data, params.trans_len);
	if (ret)
		goto unpin_memory;

	/* Copy packet header to userspace. */
	ret = copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr, params.hdr_len);

unpin_memory:
	sev_unpin_memory(kvm, guest_page, n);

free_data:
	kfree(data);

free_trans:
	kfree(trans_data);

free_hdr:
	kfree(hdr);

	return ret;
}

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Mary Higgins, Sri Rasiah, HRB 21284 (AG Nürnberg)

Re: [PATCH v3 02/11] KVM: SVM: Add KVM_SEND_UPDATE_DATA command

[Singh, Brijesh]

On 8/22/19 7:02 AM, Borislav Petkov wrote:
> On Wed, Jul 10, 2019 at 08:13:01PM +0000, Singh, Brijesh wrote:
>> The command is used for encrypting the guest memory region using the encryption
>> context created with KVM_SEV_SEND_START.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: x86@kernel.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   .../virtual/kvm/amd-memory-encryption.rst     |  24 ++++
>>   arch/x86/kvm/svm.c                            | 120 +++++++++++++++++-
>>   include/uapi/linux/kvm.h                      |   9 ++
>>   3 files changed, 149 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
>> index 0e9e1e9f9687..060ac2316d69 100644
>> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
>> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
>> @@ -265,6 +265,30 @@ Returns: 0 on success, -negative on error
>>                   __u32 session_len;
>>           };
>>   
>> +11. KVM_SEV_SEND_UPDATE_DATA
>> +----------------------------
>> +
>> +The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
>> +outgoing guest memory region with the encryption context creating using
> 
> s/creating/created/
> 
>> +KVM_SEV_SEND_START.
>> +
>> +Parameters (in): struct kvm_sev_send_update_data
>> +
>> +Returns: 0 on success, -negative on error
>> +
>> +::
>> +
>> +        struct kvm_sev_launch_send_update_data {
>> +                __u64 hdr_uaddr;        /* userspace address containing the packet header */
>> +                __u32 hdr_len;
>> +
>> +                __u64 guest_uaddr;      /* the source memory region to be encrypted */
>> +                __u32 guest_len;
>> +
>> +                __u64 trans_uaddr;      /* the destition memory region  */
> 
> s/destition/destination/
> 
>> +                __u32 trans_len;
> 
> Those addresses are all system physical addresses, according to the doc.
> Why do you call them "uaddr"?
> 


FW accepts the system physical address but the userspace does not know
the system physical instead it will give host virtual address and we
will find its corresponding system physical address and make a FW
call. This is a userspace interface and not the FW.


>> +        };
>> +
>>   References
>>   ==========
>>   
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 0b0937f53520..8e815a53c420 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -418,6 +418,7 @@ enum {
>>   
>>   static unsigned int max_sev_asid;
>>   static unsigned int min_sev_asid;
>> +static unsigned long sev_me_mask;
>>   static unsigned long *sev_asid_bitmap;
>>   #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
>>   
>> @@ -1216,16 +1217,21 @@ static int avic_ga_log_notifier(u32 ga_tag)
>>   static __init int sev_hardware_setup(void)
>>   {
>>   	struct sev_user_data_status *status;
>> +	int eax, ebx;
>>   	int rc;
>>   
>> -	/* Maximum number of encrypted guests supported simultaneously */
>> -	max_sev_asid = cpuid_ecx(0x8000001F);
>> +	/*
>> +	 * Query the memory encryption information.
>> +	 *  EBX:  Bit 0:5 Pagetable bit position used to indicate encryption (aka Cbit).
>> +	 *  ECX:  Maximum number of encrypted guests supported simultaneously.
>> +	 *  EDX:  Minimum ASID value that should be used for SEV guest.
>> +	 */
>> +	cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
>>   
>>   	if (!max_sev_asid)
>>   		return 1;
>>   
>> -	/* Minimum ASID value that should be used for SEV guest */
>> -	min_sev_asid = cpuid_edx(0x8000001F);
>> +	sev_me_mask = 1UL << (ebx & 0x3f);
>>   
>>   	/* Initialize SEV ASID bitmap */
>>   	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
>> @@ -7059,6 +7065,109 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   	return ret;
>>   }
>>   
>> +static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> +{
>> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +	struct sev_data_send_update_data *data;
>> +	struct kvm_sev_send_update_data params;
>> +	void *hdr = NULL, *trans_data = NULL;
>> +	struct page **guest_page = NULL;
> 
> Ah, I see why you do init them to NULL - -Wmaybe-uninitialized. See below.
> 
>> +	unsigned long n;
>> +	int ret, offset;
>> +
>> +	if (!sev_guest(kvm))
>> +		return -ENOTTY;
>> +
>> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>> +			sizeof(struct kvm_sev_send_update_data)))
>> +		return -EFAULT;
>> +
>> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	/* userspace wants to query either header or trans length */
>> +	if (!params.trans_len || !params.hdr_len)
>> +		goto cmd;
>> +
>> +	ret = -EINVAL;
>> +	if (!params.trans_uaddr || !params.guest_uaddr ||
>> +	    !params.guest_len || !params.hdr_uaddr)
>> +		goto e_free;
>> +
>> +	/* Check if we are crossing the page boundry */
> 
> WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
> 
> So the fact that you have to init local variables to NULL means that gcc
> doesn't see the that kfree() can take a NULL.
> 
> But also, you can restructure your labels in a way so that gcc sees them
> properly and doesn't issue the warning even without having to init those
> local variables.
> 
> And also, you can cleanup that function and split out the header and
> trans length query functionality into a separate helper and this way
> make it a lot more readable. I gave it a try here and it looks more
> readable to me but this could be just me.
> 
> I could've missed some case too... pasting the whole thing for easier
> review than as a diff:
> 

Okay, I will take a look and will probably reuse your functions. thank you.

-Brijesh

> 
> ---
> /* Userspace wants to query either header or trans length. */
> static int
> __sev_send_update_data_query_lengths(struct kvm *kvm, struct kvm_sev_cmd *argp,
> 				     struct kvm_sev_send_update_data *params)
> {
> 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> 	struct sev_data_send_update_data data;
> 
> 	memset(&data, 0, sizeof(data));
> 
> 	data.handle = sev->handle;
> 	sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, &data, &argp->error);
> 
> 	params->hdr_len   = data.hdr_len;
> 	params->trans_len = data.trans_len;
> 
> 	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
> 			 sizeof(struct kvm_sev_send_update_data)))
> 		return -EFAULT;
> 
> 	return 0;
> }
> 
> static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> {
> 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> 	struct sev_data_send_update_data *data;
> 	struct kvm_sev_send_update_data params;
> 	struct page **guest_page;
> 	void *hdr, *trans_data;
> 	unsigned long n;
> 	int ret, offset;
> 
> 	if (!sev_guest(kvm))
> 		return -ENOTTY;
> 
> 	if (copy_from_user(&params,
> 			   (void __user *)(uintptr_t)argp->data,
> 			   sizeof(struct kvm_sev_send_update_data)))
> 		return -EFAULT;
> 
> 	/* Userspace wants to query either header or trans length */
> 	if (!params.trans_len || !params.hdr_len)
> 		return __sev_send_update_data_query_lengths(kvm, argp, &params);
> 
> 	if (!params.trans_uaddr || !params.guest_uaddr ||
> 	    !params.guest_len || !params.hdr_uaddr)
> 		return -EINVAL;
> 
> 	/* Check if we are crossing the page boundary: */
> 	offset = params.guest_uaddr & (PAGE_SIZE - 1);
> 	if ((params.guest_len + offset > PAGE_SIZE))
> 		return -EINVAL;
> 
> 	hdr = kmalloc(params.hdr_len, GFP_KERNEL);
> 	if (!hdr)
> 		return -ENOMEM;
> 
> 	ret = -ENOMEM;
> 	trans_data = kmalloc(params.trans_len, GFP_KERNEL);
> 	if (!trans_data)
> 		goto free_hdr;
> 
> 	data = kzalloc(sizeof(*data), GFP_KERNEL);
>          if (!data)
>                  goto free_trans;
> 
> 	/* Pin guest memory */
> 	ret = -EFAULT;
> 	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK, PAGE_SIZE, &n, 0);
> 	if (!guest_page)
> 		goto free_data;
> 
> 	/* The SEND_UPDATE_DATA command requires C-bit to be always set. */
> 	data->guest_address	= (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
> 	data->guest_address     |= sev_me_mask;
> 	data->guest_len		= params.guest_len;
> 	data->hdr_address	= __psp_pa(hdr);
> 	data->hdr_len		= params.hdr_len;
> 	data->trans_address	= __psp_pa(trans_data);
> 	data->trans_len		= params.trans_len;
> 	data->handle		= sev->handle;
> 
> 	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
> 	if (ret)
> 		goto unpin_memory;
> 
> 	/* Copy transport buffer to user space. */
> 	ret = copy_to_user((void __user *)(uintptr_t)params.trans_uaddr, trans_data, params.trans_len);
> 	if (ret)
> 		goto unpin_memory;
> 
> 	/* Copy packet header to userspace. */
> 	ret = copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr, params.hdr_len);
> 
> unpin_memory:
> 	sev_unpin_memory(kvm, guest_page, n);
> 
> free_data:
> 	kfree(data);
> 
> free_trans:
> 	kfree(trans_data);
> 
> free_hdr:
> 	kfree(hdr);
> 
> 	return ret;
> }
> 

Re: [PATCH v3 02/11] KVM: SVM: Add KVM_SEND_UPDATE_DATA command

[Borislav Petkov]

On Thu, Aug 22, 2019 at 01:27:25PM +0000, Singh, Brijesh wrote:
> FW accepts the system physical address but the userspace does not know
> the system physical instead it will give host virtual address and we
> will find its corresponding system physical address and make a FW
> call. This is a userspace interface and not the FW.

That fact could be in a sentence or two as a comment above the struct
definition.

> Okay, I will take a look and will probably reuse your functions. thank you.

Sure, and pls see if you can simplify the other command-sending
functions in a similar manner.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Mary Higgins, Sri Rasiah, HRB 21284 (AG Nürnberg)

Re: [PATCH v3 02/11] KVM: SVM: Add KVM_SEND_UPDATE_DATA command

[Peter Gonda]

On Wed, Jul 10, 2019 at 1:14 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:

> +static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +       struct sev_data_send_update_data *data;
> +       struct kvm_sev_send_update_data params;
> +       void *hdr = NULL, *trans_data = NULL;
> +       struct page **guest_page = NULL;
> +       unsigned long n;
> +       int ret, offset;
> +
> +       if (!sev_guest(kvm))
> +               return -ENOTTY;
> +
> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +                       sizeof(struct kvm_sev_send_update_data)))
> +               return -EFAULT;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       /* userspace wants to query either header or trans length */
> +       if (!params.trans_len || !params.hdr_len)
> +               goto cmd;
> +
> +       ret = -EINVAL;
> +       if (!params.trans_uaddr || !params.guest_uaddr ||
> +           !params.guest_len || !params.hdr_uaddr)
> +               goto e_free;
> +
> +       /* Check if we are crossing the page boundry */
> +       ret = -EINVAL;
> +       offset = params.guest_uaddr & (PAGE_SIZE - 1);
> +       if ((params.guest_len + offset > PAGE_SIZE))
> +               goto e_free;
> +
> +       ret = -ENOMEM;
> +       hdr = kmalloc(params.hdr_len, GFP_KERNEL);
> +       if (!hdr)
> +               goto e_free;

Should we be checking params.hdr_len against SEV_FW_BLOB_MAX_SIZE?

> +
> +       data->hdr_address = __psp_pa(hdr);
> +       data->hdr_len = params.hdr_len;
> +
> +       ret = -ENOMEM;
> +       trans_data = kmalloc(params.trans_len, GFP_KERNEL);
> +       if (!trans_data)
> +               goto e_free;

Ditto, should we be checking params.hdr_len against SEV_FW_BLOB_MAX_SIZE?

[PATCH v3 03/11] KVM: SVM: Add KVM_SEV_SEND_FINISH command

[Singh, Brijesh]

The command is used to finailize the encryption context created with
KVM_SEV_SEND_START command.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../virtual/kvm/amd-memory-encryption.rst     |  8 +++++++
 arch/x86/kvm/svm.c                            | 23 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index 060ac2316d69..9864f9215c43 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -289,6 +289,14 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+12. KVM_SEV_SEND_FINISH
+------------------------
+
+After completion of the migration flow, the KVM_SEV_SEND_FINISH command can be
+issued by the hypervisor to delete the encryption context.
+
+Returns: 0 on success, -negative on error
+
 References
 ==========
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8e815a53c420..be73a87a8c4f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7168,6 +7168,26 @@ static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_send_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_finish *data;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_FINISH, data, &argp->error);
+
+	kfree(data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7215,6 +7235,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SEND_UPDATE_DATA:
 		r = sev_send_update_data(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_FINISH:
+		r = sev_send_finish(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
-- 
2.17.1

Re: [PATCH v3 03/11] KVM: SVM: Add KVM_SEV_SEND_FINISH command

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:03PM +0000, Singh, Brijesh wrote:
> The command is used to finailize the encryption context created with
> KVM_SEV_SEND_START command.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../virtual/kvm/amd-memory-encryption.rst     |  8 +++++++
>  arch/x86/kvm/svm.c                            | 23 +++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
> index 060ac2316d69..9864f9215c43 100644
> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
> @@ -289,6 +289,14 @@ Returns: 0 on success, -negative on error
>                  __u32 trans_len;
>          };
>  
> +12. KVM_SEV_SEND_FINISH
> +------------------------
> +
> +After completion of the migration flow, the KVM_SEV_SEND_FINISH command can be
> +issued by the hypervisor to delete the encryption context.
> +
> +Returns: 0 on success, -negative on error
> +
>  References
>  ==========
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 8e815a53c420..be73a87a8c4f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7168,6 +7168,26 @@ static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return ret;
>  }
>  
> +static int sev_send_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct sev_data_send_finish *data;
> +	int ret;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;

Almost all sev_ command functions do that check, except
sev_guest_init(). You could pull up that check, into svm_mem_enc_op()
and save yourself the repeated pattern:

---
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 273ad624b23d..950282c8c4f7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7225,6 +7225,11 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	if (copy_from_user(&sev_cmd, argp, sizeof(struct kvm_sev_cmd)))
 		return -EFAULT;
 
+	if (sev_cmd.id != KVM_SEV_INIT) {
+		if (!sev_guest(kvm))
+			return -ENOTTY;
+	}
+
 	mutex_lock(&kvm->lock);
 
 	switch (sev_cmd.id) {
---

> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);

Btw, since

  1ec696470c86 ("kvm: svm: Add memcg accounting to KVM allocations")

gfp flags should be GFP_KERNEL_ACCOUNT now.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

[PATCH v3 04/11] KVM: SVM: Add support for KVM_SEV_RECEIVE_START command

[Singh, Brijesh]

The command is used to create the encryption context for an incoming
SEV guest. The encryption context can be later used by the hypervisor
to import the incoming data into the SEV guest memory space.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../virtual/kvm/amd-memory-encryption.rst     | 29 +++++++
 arch/x86/kvm/svm.c                            | 80 +++++++++++++++++++
 include/uapi/linux/kvm.h                      |  9 +++
 3 files changed, 118 insertions(+)

diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index 9864f9215c43..59a7903f7c4e 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -297,6 +297,35 @@ issued by the hypervisor to delete the encryption context.
 
 Returns: 0 on success, -negative on error
 
+13. KVM_SEV_RECEIVE_START
+------------------------
+
+The KVM_SEV_RECEIVE_START command is used for creating the memory encryption
+context for an incoming SEV guest. To create the encryption context, the user must
+provide a guest policy, the platform public Diffie-Hellman (PDH) key and session
+information.
+
+Parameters: struct  kvm_sev_receive_start (in/out)
+
+Returns: 0 on success, -negative on error
+
+::
+
+        struct kvm_sev_receive_start {
+                __u32 handle;           /* if zero then firmware creates a new handle */
+                __u32 policy;           /* guest's policy */
+
+                __u64 pdh_uaddr;         /* userspace address pointing to the PDH key */
+                __u32 dh_len;
+
+                __u64 session_addr;     /* userspace address which points to the guest session information */
+                __u32 session_len;
+        };
+
+On success, the 'handle' field contains a new handle and on error, a negative value.
+
+For more details, see SEV spec Section 6.12.
+
 References
 ==========
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index be73a87a8c4f..fae6a870b29a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7188,6 +7188,83 @@ static int sev_send_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_receive_start *start;
+	struct kvm_sev_receive_start params;
+	int *error = &argp->error;
+	void *session_data = NULL;
+	void *pdh_data = NULL;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	/* Get parameter from the user */
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			sizeof(struct kvm_sev_receive_start)))
+		return -EFAULT;
+
+	if (!params.pdh_uaddr || !params.pdh_len ||
+	    !params.session_uaddr || !params.session_len)
+		return -EINVAL;
+
+	start = kzalloc(sizeof(*start), GFP_KERNEL);
+	if (!start)
+		return -ENOMEM;
+
+	start->handle = params.handle;
+	start->policy = params.policy;
+
+	pdh_data = psp_copy_user_blob(params.pdh_uaddr, params.pdh_len);
+	if (IS_ERR(pdh_data)) {
+		ret = PTR_ERR(pdh_data);
+		goto e_free;
+	}
+
+	start->pdh_cert_address = __psp_pa(pdh_data);
+	start->pdh_cert_len = params.pdh_len;
+
+	session_data = psp_copy_user_blob(params.session_uaddr, params.session_len);
+	if (IS_ERR(session_data)) {
+		ret = PTR_ERR(session_data);
+		goto e_free_pdh;
+	}
+
+	start->session_address = __psp_pa(session_data);
+	start->session_len = params.session_len;
+
+	/* create memory encryption context */
+	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_RECEIVE_START, start, error);
+	if (ret)
+		goto e_free_session;
+
+	/* Bind ASID to this guest */
+	ret = sev_bind_asid(kvm, start->handle, error);
+	if (ret)
+		goto e_free_session;
+
+	params.handle = start->handle;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data,
+			 &params, sizeof(struct kvm_sev_receive_start))) {
+		ret = -EFAULT;
+		sev_unbind_asid(kvm, start->handle);
+		goto e_free_session;
+	}
+
+	sev->handle = start->handle;
+	sev->fd = argp->sev_fd;
+
+e_free_session:
+	kfree(session_data);
+e_free_pdh:
+	kfree(pdh_data);
+e_free:
+	kfree(start);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7238,6 +7315,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SEND_FINISH:
 		r = sev_send_finish(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_RECEIVE_START:
+		r = sev_receive_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4cb6c3774ec2..28d240974ea7 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1552,6 +1552,15 @@ struct kvm_sev_send_update_data {
 	__u32 trans_len;
 };
 
+struct kvm_sev_receive_start {
+	__u32 handle;
+	__u32 policy;
+	__u64 pdh_uaddr;
+	__u32 pdh_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.17.1

[PATCH v3 05/11] KVM: SVM: Add KVM_SEV_RECEIVE_UPDATE_DATA command

[Singh, Brijesh]

The command is used for copying the incoming buffer into the
SEV guest memory space.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../virtual/kvm/amd-memory-encryption.rst     | 24 ++++++
 arch/x86/kvm/svm.c                            | 75 +++++++++++++++++++
 include/uapi/linux/kvm.h                      |  9 +++
 3 files changed, 108 insertions(+)

diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index 59a7903f7c4e..f0ec0cbe2aaf 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -326,6 +326,30 @@ On success, the 'handle' field contains a new handle and on error, a negative va
 
 For more details, see SEV spec Section 6.12.
 
+14. KVM_SEV_RECEIVE_UPDATE_DATA
+----------------------------
+
+The KVM_SEV_RECEIVE_UPDATE_DATA command can be used by the hypervisor to copy
+the incoming buffers into the guest memory region with encryption context
+created during the KVM_SEV_RECEIVE_START.
+
+Parameters (in): struct kvm_sev_receive_update_data
+
+Returns: 0 on success, -negative on error
+
+::
+
+        struct kvm_sev_launch_receive_update_data {
+                __u64 hdr_uaddr;        /* userspace address containing the packet header */
+                __u32 hdr_len;
+
+                __u64 guest_uaddr;      /* the destination guest memory region */
+                __u32 guest_len;
+
+                __u64 trans_uaddr;      /* the incoming buffer memory region  */
+                __u32 trans_len;
+        };
+
 References
 ==========
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index fae6a870b29a..ea084716f966 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7265,6 +7265,78 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct kvm_sev_receive_update_data params;
+	struct sev_data_receive_update_data *data;
+	void *hdr = NULL, *trans = NULL;
+	struct page **guest_page;
+	unsigned long n;
+	int ret, offset;
+
+	if (!sev_guest(kvm))
+		return -EINVAL;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			sizeof(struct kvm_sev_receive_update_data)))
+		return -EFAULT;
+
+	if (!params.hdr_uaddr || !params.hdr_len ||
+	    !params.guest_uaddr || !params.guest_len ||
+	    !params.trans_uaddr || !params.trans_len)
+		return -EINVAL;
+
+	/* Check if we are crossing the page boundry */
+	offset = params.guest_uaddr & (PAGE_SIZE - 1);
+	if ((params.guest_len + offset > PAGE_SIZE))
+		return -EINVAL;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
+	if (IS_ERR(hdr)) {
+		ret = PTR_ERR(hdr);
+		goto e_free;
+	}
+
+	data->hdr_address = __psp_pa(hdr);
+	data->hdr_len = params.hdr_len;
+
+	trans = psp_copy_user_blob(params.trans_uaddr, params.trans_len);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto e_free;
+	}
+
+	data->trans_address = __psp_pa(trans);
+	data->trans_len = params.trans_len;
+
+	/* Pin guest memory */
+	ret = -EFAULT;
+	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
+				    PAGE_SIZE, &n, 0);
+	if (!guest_page)
+		goto e_free;
+
+	/* The RECEIVE_UPDATE_DATA command requires C-bit to be always set. */
+	data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
+	data->guest_address |= sev_me_mask;
+	data->guest_len = params.guest_len;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_RECEIVE_UPDATE_DATA, data, &argp->error);
+
+	sev_unpin_memory(kvm, guest_page, n);
+e_free:
+	kfree(data);
+	kfree(hdr);
+	kfree(trans);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7318,6 +7390,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_RECEIVE_START:
 		r = sev_receive_start(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_RECEIVE_UPDATE_DATA:
+		r = sev_receive_update_data(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 28d240974ea7..e31cdb41519f 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1561,6 +1561,15 @@ struct kvm_sev_receive_start {
 	__u32 session_len;
 };
 
+struct kvm_sev_receive_update_data {
+	__u64 hdr_uaddr;
+	__u32 hdr_len;
+	__u64 guest_uaddr;
+	__u32 guest_len;
+	__u64 trans_uaddr;
+	__u32 trans_len;
+};
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.17.1

Re: [PATCH v3 05/11] KVM: SVM: Add KVM_SEV_RECEIVE_UPDATE_DATA command

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:06PM +0000, Singh, Brijesh wrote:
> The command is used for copying the incoming buffer into the
> SEV guest memory space.

...

> +static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct kvm_sev_receive_update_data params;
> +	struct sev_data_receive_update_data *data;
> +	void *hdr = NULL, *trans = NULL;
> +	struct page **guest_page;
> +	unsigned long n;
> +	int ret, offset;
> +
> +	if (!sev_guest(kvm))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +			sizeof(struct kvm_sev_receive_update_data)))
> +		return -EFAULT;
> +
> +	if (!params.hdr_uaddr || !params.hdr_len ||
> +	    !params.guest_uaddr || !params.guest_len ||
> +	    !params.trans_uaddr || !params.trans_len)
> +		return -EINVAL;
> +
> +	/* Check if we are crossing the page boundry */

WARNING: 'boundry' may be misspelled - perhaps 'boundary'?

> +	offset = params.guest_uaddr & (PAGE_SIZE - 1);
> +	if ((params.guest_len + offset > PAGE_SIZE))
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
> +	if (IS_ERR(hdr)) {
> +		ret = PTR_ERR(hdr);
> +		goto e_free;
> +	}
> +
> +	data->hdr_address = __psp_pa(hdr);
> +	data->hdr_len = params.hdr_len;
> +
> +	trans = psp_copy_user_blob(params.trans_uaddr, params.trans_len);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto e_free;
> +	}
> +
> +	data->trans_address = __psp_pa(trans);
> +	data->trans_len = params.trans_len;
> +
> +	/* Pin guest memory */
> +	ret = -EFAULT;
> +	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
> +				    PAGE_SIZE, &n, 0);
> +	if (!guest_page)
> +		goto e_free;
> +
> +	/* The RECEIVE_UPDATE_DATA command requires C-bit to be always set. */
> +	data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
> +	data->guest_address |= sev_me_mask;
> +	data->guest_len = params.guest_len;
> +
> +	data->handle = sev->handle;
> +	ret = sev_issue_cmd(kvm, SEV_CMD_RECEIVE_UPDATE_DATA, data, &argp->error);
> +
> +	sev_unpin_memory(kvm, guest_page, n);
> +e_free:
> +	kfree(data);
> +	kfree(hdr);
> +	kfree(trans);

Pls add separate labels so that you don't have to init function-local
vars above to NULL.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

Re: [PATCH v3 05/11] KVM: SVM: Add KVM_SEV_RECEIVE_UPDATE_DATA command

[Peter Gonda]

> +
> +       /* Check if we are crossing the page boundry */
> +       offset = params.guest_uaddr & (PAGE_SIZE - 1);
> +       if ((params.guest_len + offset > PAGE_SIZE))
> +               return -EINVAL;

Just curious spec only says that "System physical of the guest memory
region. Must be 16 B aligned with the C-bit set." and "Length of guest
memory region. Must be a multiple of 16 B and no more than 16 kB". Why
do we want to avoid crossing a page boundary?

Also is there an overflow concern in the conditional because
params.guest_len is not checked before this?

> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
> +       if (IS_ERR(hdr)) {
> +               ret = PTR_ERR(hdr);
> +               goto e_free;
> +       }
> +

[PATCH v3 06/11] KVM: SVM: Add KVM_SEV_RECEIVE_FINISH command

[Singh, Brijesh]

The command finalize the guest receiving process and make the SEV guest
ready for the execution.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 .../virtual/kvm/amd-memory-encryption.rst     |  8 +++++++
 arch/x86/kvm/svm.c                            | 23 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index f0ec0cbe2aaf..b3319f0221ee 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -350,6 +350,14 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+15. KVM_SEV_RECEIVE_FINISH
+------------------------
+
+After completion of the migration flow, the KVM_SEV_RECEIVE_FINISH command can be
+issued by the hypervisor to make the guest ready for execution.
+
+Returns: 0 on success, -negative on error
+
 References
 ==========
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ea084716f966..3089942f6630 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7337,6 +7337,26 @@ static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_receive_finish *data;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_RECEIVE_FINISH, data, &argp->error);
+
+	kfree(data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7393,6 +7413,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_RECEIVE_UPDATE_DATA:
 		r = sev_receive_update_data(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_RECEIVE_FINISH:
+		r = sev_receive_finish(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
-- 
2.17.1

Re: [PATCH v3 06/11] KVM: SVM: Add KVM_SEV_RECEIVE_FINISH command

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:07PM +0000, Singh, Brijesh wrote:
> The command finalize the guest receiving process and make the SEV guest
> ready for the execution.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../virtual/kvm/amd-memory-encryption.rst     |  8 +++++++
>  arch/x86/kvm/svm.c                            | 23 +++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
> index f0ec0cbe2aaf..b3319f0221ee 100644
> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
> @@ -350,6 +350,14 @@ Returns: 0 on success, -negative on error
>                  __u32 trans_len;
>          };
>  
> +15. KVM_SEV_RECEIVE_FINISH
> +------------------------
> +
> +After completion of the migration flow, the KVM_SEV_RECEIVE_FINISH command can be
> +issued by the hypervisor to make the guest ready for execution.
> +
> +Returns: 0 on success, -negative on error
> +
>  References
>  ==========
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ea084716f966..3089942f6630 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7337,6 +7337,26 @@ static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return ret;
>  }
>  
> +static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct sev_data_receive_finish *data;
> +	int ret;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->handle = sev->handle;
> +	ret = sev_issue_cmd(kvm, SEV_CMD_RECEIVE_FINISH, data, &argp->error);
> +
> +	kfree(data);
> +	return ret;
> +}

sev_receive_finish
sev_send_finish
sev_launch_finish

all three are almost identical. Please aggregate them into a single
__sev_send_finish_cmd() and make all three wrappers pass in the
respective SEV_CMD_*.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

[PATCH v3 07/11] KVM: x86: Add AMD SEV specific Hypercall3

[Singh, Brijesh]

KVM hypercall framework relies on alternative framework to patch the
VMCALL -> VMMCALL on AMD platform. If a hypercall is made before
apply_alternative() is called then it defaults to VMCALL. The approach
works fine on non SEV guest. A VMCALL would causes #UD, and hypervisor
will be able to decode the instruction and do the right things. But
when SEV is active, guest memory is encrypted with guest key and
hypervisor will not be able to decode the instruction bytes.

Add SEV specific hypercall3, it unconditionally uses VMMCALL. The hypercall
will be used by the SEV guest to notify encrypted pages to the hypervisor.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/include/asm/kvm_para.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 5ed3cf1c3934..94e91c0bc2e0 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -84,6 +84,18 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
 	return ret;
 }
 
+static inline long kvm_sev_hypercall3(unsigned int nr, unsigned long p1,
+				      unsigned long p2, unsigned long p3)
+{
+	long ret;
+
+	asm volatile("vmmcall"
+		     : "=a"(ret)
+		     : "a"(nr), "b"(p1), "c"(p2), "d"(p3)
+		     : "memory");
+	return ret;
+}
+
 #ifdef CONFIG_KVM_GUEST
 bool kvm_para_available(void);
 unsigned int kvm_arch_para_features(void);
-- 
2.17.1

[PATCH v3 08/11] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS hypercall

[Singh, Brijesh]

This hypercall is used by the SEV guest to notify a change in the page
encryption status to the hypervisor. The hypercall should be invoked
only when the encryption attribute is changed from encrypted -> decrypted
and vice versa. By default all guest pages are considered encrypted.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 Documentation/virtual/kvm/hypercalls.txt | 14 +++++
 arch/x86/include/asm/kvm_host.h          |  2 +
 arch/x86/kvm/svm.c                       | 70 ++++++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.c                   |  1 +
 arch/x86/kvm/x86.c                       |  5 ++
 include/uapi/linux/kvm_para.h            |  1 +
 6 files changed, 93 insertions(+)

diff --git a/Documentation/virtual/kvm/hypercalls.txt b/Documentation/virtual/kvm/hypercalls.txt
index da24c138c8d1..94f0611f4d88 100644
--- a/Documentation/virtual/kvm/hypercalls.txt
+++ b/Documentation/virtual/kvm/hypercalls.txt
@@ -141,3 +141,17 @@ a0 corresponds to the APIC ID in the third argument (a2), bit 1
 corresponds to the APIC ID a2+1, and so on.
 
 Returns the number of CPUs to which the IPIs were delivered successfully.
+
+7. KVM_HC_PAGE_ENC_STATUS
+-------------------------
+Architecture: x86
+Status: active
+Purpose: Notify the encryption status changes in guest page table (SEV guest)
+
+a0: the guest physical address of the start page
+a1: the number of pages
+a2: encryption attribute
+
+   Where:
+	* 1: Encryption attribute is set
+	* 0: Encryption attribute is cleared
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 26d1eb83f72a..b463a81dc176 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1199,6 +1199,8 @@ struct kvm_x86_ops {
 	uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);
 
 	bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu);
+	int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
+				  unsigned long sz, unsigned long mode);
 };
 
 struct kvm_arch_async_pf {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3089942f6630..431718309359 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -135,6 +135,8 @@ struct kvm_sev_info {
 	int fd;			/* SEV device fd */
 	unsigned long pages_locked; /* Number of pages locked */
 	struct list_head regions_list;  /* List of registered regions */
+	unsigned long *page_enc_bmap;
+	unsigned long page_enc_bmap_size;
 };
 
 struct kvm_svm {
@@ -1910,6 +1912,8 @@ static void sev_vm_destroy(struct kvm *kvm)
 
 	sev_unbind_asid(kvm, sev->handle);
 	sev_asid_free(kvm);
+
+	kvfree(sev->page_enc_bmap);
 }
 
 static void avic_vm_destroy(struct kvm *kvm)
@@ -2084,6 +2088,7 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
 
 static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
+	struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info;
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u32 dummy;
 	u32 eax = 1;
@@ -2105,6 +2110,12 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 
 	if (kvm_vcpu_apicv_active(vcpu) && !init_event)
 		avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
+
+	/* reset the page encryption bitmap */
+	if (sev_guest(vcpu->kvm)) {
+		kvfree(sev->page_enc_bmap);
+		sev->page_enc_bmap_size = 0;
+	}
 }
 
 static int avic_init_vcpu(struct vcpu_svm *svm)
@@ -7357,6 +7368,63 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_resize_page_enc_bitmap(struct kvm *kvm, unsigned long new_size)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	unsigned long *map;
+	unsigned long sz;
+
+	if (sev->page_enc_bmap_size >= new_size)
+		return 0;
+
+	sz = ALIGN(new_size, BITS_PER_LONG) / 8;
+
+	map = vmalloc(sz);
+	if (!map) {
+		pr_err_once("Failed to allocate decrypted bitmap size %lx\n", sz);
+		return -ENOMEM;
+	}
+
+	/* mark the page encrypted (by default) */
+	memset(map, 0xff, sz);
+
+	bitmap_copy(map, sev->page_enc_bmap, sev->page_enc_bmap_size);
+	kvfree(sev->page_enc_bmap);
+
+	sev->page_enc_bmap = map;
+	sev->page_enc_bmap_size = new_size;
+
+	return 0;
+}
+
+static int svm_page_enc_status_hc(struct kvm *kvm, unsigned long gpa,
+				  unsigned long npages, unsigned long enc)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	gfn_t gfn_start, gfn_end;
+	int ret;
+
+	if (!npages)
+		return 0;
+
+	gfn_start = gpa_to_gfn(gpa);
+	gfn_end = gfn_start + npages;
+
+	mutex_lock(&kvm->lock);
+	ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
+	if (ret)
+		goto unlock;
+
+	if (enc)
+		__bitmap_set(sev->page_enc_bmap, gfn_start, gfn_end - gfn_start);
+	else
+		__bitmap_clear(sev->page_enc_bmap, gfn_start, gfn_end - gfn_start);
+
+unlock:
+	mutex_unlock(&kvm->lock);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7698,6 +7766,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 	.nested_get_evmcs_version = nested_get_evmcs_version,
 
 	.need_emulation_on_page_fault = svm_need_emulation_on_page_fault,
+
+	.page_enc_status_hc = svm_page_enc_status_hc
 };
 
 static int __init svm_init(void)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d98eac371c0a..78f8a93fc6dd 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7724,6 +7724,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 	.get_vmcs12_pages = NULL,
 	.nested_enable_evmcs = NULL,
 	.need_emulation_on_page_fault = vmx_need_emulation_on_page_fault,
+	.page_enc_status_hc = NULL,
 };
 
 static void vmx_cleanup_l1d_flush(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 63bb1ee8258e..6baf48ec0ed4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7219,6 +7219,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	case KVM_HC_SEND_IPI:
 		ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
 		break;
+	case KVM_HC_PAGE_ENC_STATUS:
+		ret = -KVM_ENOSYS;
+		if (kvm_x86_ops->page_enc_status_hc)
+			ret = kvm_x86_ops->page_enc_status_hc(vcpu->kvm, a0, a1, a2);
+		break;
 	default:
 		ret = -KVM_ENOSYS;
 		break;
diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
index 6c0ce49931e5..3dc9e579f4f9 100644
--- a/include/uapi/linux/kvm_para.h
+++ b/include/uapi/linux/kvm_para.h
@@ -28,6 +28,7 @@
 #define KVM_HC_MIPS_CONSOLE_OUTPUT	8
 #define KVM_HC_CLOCK_PAIRING		9
 #define KVM_HC_SEND_IPI		10
+#define KVM_HC_PAGE_ENC_STATUS		11
 
 /*
  * hypercalls use architecture specific
-- 
2.17.1

Re: [PATCH v3 08/11] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS hypercall

[David Rientjes]

On Wed, 10 Jul 2019, Singh, Brijesh wrote:

> diff --git a/Documentation/virtual/kvm/hypercalls.txt b/Documentation/virtual/kvm/hypercalls.txt
> index da24c138c8d1..94f0611f4d88 100644
> --- a/Documentation/virtual/kvm/hypercalls.txt
> +++ b/Documentation/virtual/kvm/hypercalls.txt
> @@ -141,3 +141,17 @@ a0 corresponds to the APIC ID in the third argument (a2), bit 1
>  corresponds to the APIC ID a2+1, and so on.
>  
>  Returns the number of CPUs to which the IPIs were delivered successfully.
> +
> +7. KVM_HC_PAGE_ENC_STATUS
> +-------------------------
> +Architecture: x86
> +Status: active
> +Purpose: Notify the encryption status changes in guest page table (SEV guest)
> +
> +a0: the guest physical address of the start page
> +a1: the number of pages
> +a2: encryption attribute
> +
> +   Where:
> +	* 1: Encryption attribute is set
> +	* 0: Encryption attribute is cleared
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 26d1eb83f72a..b463a81dc176 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1199,6 +1199,8 @@ struct kvm_x86_ops {
>  	uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);
>  
>  	bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu);
> +	int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
> +				  unsigned long sz, unsigned long mode);
>  };
>  
>  struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 3089942f6630..431718309359 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -135,6 +135,8 @@ struct kvm_sev_info {
>  	int fd;			/* SEV device fd */
>  	unsigned long pages_locked; /* Number of pages locked */
>  	struct list_head regions_list;  /* List of registered regions */
> +	unsigned long *page_enc_bmap;
> +	unsigned long page_enc_bmap_size;
>  };
>  
>  struct kvm_svm {
> @@ -1910,6 +1912,8 @@ static void sev_vm_destroy(struct kvm *kvm)
>  
>  	sev_unbind_asid(kvm, sev->handle);
>  	sev_asid_free(kvm);
> +
> +	kvfree(sev->page_enc_bmap);
>  }
>  
>  static void avic_vm_destroy(struct kvm *kvm)

Adding Cfir who flagged this kvfree().

Other freeing of sev->page_enc_bmap in this patch also set 
sev->page_enc_bmap_size to 0 and neither set sev->page_enc_bmap to NULL 
after freeing it.

For extra safety, is it possible to sev->page_enc_bmap = NULL anytime the 
bitmap is kvfreed?

> @@ -2084,6 +2088,7 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>  
>  static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  {
> +	struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info;
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u32 dummy;
>  	u32 eax = 1;
> @@ -2105,6 +2110,12 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  
>  	if (kvm_vcpu_apicv_active(vcpu) && !init_event)
>  		avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
> +
> +	/* reset the page encryption bitmap */
> +	if (sev_guest(vcpu->kvm)) {
> +		kvfree(sev->page_enc_bmap);
> +		sev->page_enc_bmap_size = 0;
> +	}
>  }
>  
>  static int avic_init_vcpu(struct vcpu_svm *svm)

What is protecting sev->page_enc_bmap and sev->page_enc_bmap_size in calls 
to svm_vcpu_reset()?

Re: [PATCH v3 08/11] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS hypercall

[Cfir Cohen]

In addition, it seems that svm_page_enc_status_hc() accepts 'gpa',
'npages', 'enc' directly from the guest, and so these can take
arbitrary values. A very large 'npages' could lead to an int overflow
in 'gfn_end = gfn_start + npages', making gfn_end < gfn_start. This
could an OOB access in the bitmap. Concrete example: gfn_start = 2,
npages = -1, gfn_end = 2+(-1) = 1, sev_resize_page_enc_bitmap
allocates a bitmap for a single page (new_size=1), __bitmap_set access
offset gfn_end - gfn_start = -1.


On Sun, Jul 21, 2019 at 1:57 PM David Rientjes <rientjes@google.com> wrote:
>
> On Wed, 10 Jul 2019, Singh, Brijesh wrote:
>
> > diff --git a/Documentation/virtual/kvm/hypercalls.txt b/Documentation/virtual/kvm/hypercalls.txt
> > index da24c138c8d1..94f0611f4d88 100644
> > --- a/Documentation/virtual/kvm/hypercalls.txt
> > +++ b/Documentation/virtual/kvm/hypercalls.txt
> > @@ -141,3 +141,17 @@ a0 corresponds to the APIC ID in the third argument (a2), bit 1
> >  corresponds to the APIC ID a2+1, and so on.
> >
> >  Returns the number of CPUs to which the IPIs were delivered successfully.
> > +
> > +7. KVM_HC_PAGE_ENC_STATUS
> > +-------------------------
> > +Architecture: x86
> > +Status: active
> > +Purpose: Notify the encryption status changes in guest page table (SEV guest)
> > +
> > +a0: the guest physical address of the start page
> > +a1: the number of pages
> > +a2: encryption attribute
> > +
> > +   Where:
> > +     * 1: Encryption attribute is set
> > +     * 0: Encryption attribute is cleared
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 26d1eb83f72a..b463a81dc176 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1199,6 +1199,8 @@ struct kvm_x86_ops {
> >       uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);
> >
> >       bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu);
> > +     int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
> > +                               unsigned long sz, unsigned long mode);
> >  };
> >
> >  struct kvm_arch_async_pf {
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 3089942f6630..431718309359 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -135,6 +135,8 @@ struct kvm_sev_info {
> >       int fd;                 /* SEV device fd */
> >       unsigned long pages_locked; /* Number of pages locked */
> >       struct list_head regions_list;  /* List of registered regions */
> > +     unsigned long *page_enc_bmap;
> > +     unsigned long page_enc_bmap_size;
> >  };
> >
> >  struct kvm_svm {
> > @@ -1910,6 +1912,8 @@ static void sev_vm_destroy(struct kvm *kvm)
> >
> >       sev_unbind_asid(kvm, sev->handle);
> >       sev_asid_free(kvm);
> > +
> > +     kvfree(sev->page_enc_bmap);
> >  }
> >
> >  static void avic_vm_destroy(struct kvm *kvm)
>
> Adding Cfir who flagged this kvfree().
>
> Other freeing of sev->page_enc_bmap in this patch also set
> sev->page_enc_bmap_size to 0 and neither set sev->page_enc_bmap to NULL
> after freeing it.
>
> For extra safety, is it possible to sev->page_enc_bmap = NULL anytime the
> bitmap is kvfreed?
>
> > @@ -2084,6 +2088,7 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
> >
> >  static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> >  {
> > +     struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info;
> >       struct vcpu_svm *svm = to_svm(vcpu);
> >       u32 dummy;
> >       u32 eax = 1;
> > @@ -2105,6 +2110,12 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> >
> >       if (kvm_vcpu_apicv_active(vcpu) && !init_event)
> >               avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
> > +
> > +     /* reset the page encryption bitmap */
> > +     if (sev_guest(vcpu->kvm)) {
> > +             kvfree(sev->page_enc_bmap);
> > +             sev->page_enc_bmap_size = 0;
> > +     }
> >  }
> >
> >  static int avic_init_vcpu(struct vcpu_svm *svm)
>
> What is protecting sev->page_enc_bmap and sev->page_enc_bmap_size in calls
> to svm_vcpu_reset()?

Re: [PATCH v3 08/11] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS hypercall

[Singh, Brijesh]

On 7/22/19 12:12 PM, Cfir Cohen wrote:
> In addition, it seems that svm_page_enc_status_hc() accepts 'gpa',
> 'npages', 'enc' directly from the guest, and so these can take
> arbitrary values. A very large 'npages' could lead to an int overflow
> in 'gfn_end = gfn_start + npages', making gfn_end < gfn_start. This
> could an OOB access in the bitmap. Concrete example: gfn_start = 2,
> npages = -1, gfn_end = 2+(-1) = 1, sev_resize_page_enc_bitmap
> allocates a bitmap for a single page (new_size=1), __bitmap_set access
> offset gfn_end - gfn_start = -1.
> 

Good point. I will add a check for it, something like

if (gfn_end <= gfn_start)
	return -EINVAL;


> 
> On Sun, Jul 21, 2019 at 1:57 PM David Rientjes <rientjes@google.com> wrote:
>>
>> On Wed, 10 Jul 2019, Singh, Brijesh wrote:
>>
>>> diff --git a/Documentation/virtual/kvm/hypercalls.txt b/Documentation/virtual/kvm/hypercalls.txt
>>> index da24c138c8d1..94f0611f4d88 100644
>>> --- a/Documentation/virtual/kvm/hypercalls.txt
>>> +++ b/Documentation/virtual/kvm/hypercalls.txt
>>> @@ -141,3 +141,17 @@ a0 corresponds to the APIC ID in the third argument (a2), bit 1
>>>   corresponds to the APIC ID a2+1, and so on.
>>>
>>>   Returns the number of CPUs to which the IPIs were delivered successfully.
>>> +
>>> +7. KVM_HC_PAGE_ENC_STATUS
>>> +-------------------------
>>> +Architecture: x86
>>> +Status: active
>>> +Purpose: Notify the encryption status changes in guest page table (SEV guest)
>>> +
>>> +a0: the guest physical address of the start page
>>> +a1: the number of pages
>>> +a2: encryption attribute
>>> +
>>> +   Where:
>>> +     * 1: Encryption attribute is set
>>> +     * 0: Encryption attribute is cleared
>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>> index 26d1eb83f72a..b463a81dc176 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -1199,6 +1199,8 @@ struct kvm_x86_ops {
>>>        uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);
>>>
>>>        bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu);
>>> +     int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
>>> +                               unsigned long sz, unsigned long mode);
>>>   };
>>>
>>>   struct kvm_arch_async_pf {
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index 3089942f6630..431718309359 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -135,6 +135,8 @@ struct kvm_sev_info {
>>>        int fd;                 /* SEV device fd */
>>>        unsigned long pages_locked; /* Number of pages locked */
>>>        struct list_head regions_list;  /* List of registered regions */
>>> +     unsigned long *page_enc_bmap;
>>> +     unsigned long page_enc_bmap_size;
>>>   };
>>>
>>>   struct kvm_svm {
>>> @@ -1910,6 +1912,8 @@ static void sev_vm_destroy(struct kvm *kvm)
>>>
>>>        sev_unbind_asid(kvm, sev->handle);
>>>        sev_asid_free(kvm);
>>> +
>>> +     kvfree(sev->page_enc_bmap);
>>>   }
>>>
>>>   static void avic_vm_destroy(struct kvm *kvm)
>>
>> Adding Cfir who flagged this kvfree().
>>
>> Other freeing of sev->page_enc_bmap in this patch also set
>> sev->page_enc_bmap_size to 0 and neither set sev->page_enc_bmap to NULL
>> after freeing it.
>>
>> For extra safety, is it possible to sev->page_enc_bmap = NULL anytime the
>> bitmap is kvfreed?
>>
>>> @@ -2084,6 +2088,7 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>>>
>>>   static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>   {
>>> +     struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info;
>>>        struct vcpu_svm *svm = to_svm(vcpu);
>>>        u32 dummy;
>>>        u32 eax = 1;
>>> @@ -2105,6 +2110,12 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>
>>>        if (kvm_vcpu_apicv_active(vcpu) && !init_event)
>>>                avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
>>> +
>>> +     /* reset the page encryption bitmap */
>>> +     if (sev_guest(vcpu->kvm)) {
>>> +             kvfree(sev->page_enc_bmap);
>>> +             sev->page_enc_bmap_size = 0;
>>> +     }
>>>   }
>>>
>>>   static int avic_init_vcpu(struct vcpu_svm *svm)
>>
>> What is protecting sev->page_enc_bmap and sev->page_enc_bmap_size in calls
>> to svm_vcpu_reset()?

Re: [PATCH v3 08/11] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS hypercall

[Singh, Brijesh]

On 7/21/19 3:57 PM, David Rientjes wrote:
> On Wed, 10 Jul 2019, Singh, Brijesh wrote:
> 
>> diff --git a/Documentation/virtual/kvm/hypercalls.txt b/Documentation/virtual/kvm/hypercalls.txt
>> index da24c138c8d1..94f0611f4d88 100644
>> --- a/Documentation/virtual/kvm/hypercalls.txt
>> +++ b/Documentation/virtual/kvm/hypercalls.txt
>> @@ -141,3 +141,17 @@ a0 corresponds to the APIC ID in the third argument (a2), bit 1
>>   corresponds to the APIC ID a2+1, and so on.
>>   
>>   Returns the number of CPUs to which the IPIs were delivered successfully.
>> +
>> +7. KVM_HC_PAGE_ENC_STATUS
>> +-------------------------
>> +Architecture: x86
>> +Status: active
>> +Purpose: Notify the encryption status changes in guest page table (SEV guest)
>> +
>> +a0: the guest physical address of the start page
>> +a1: the number of pages
>> +a2: encryption attribute
>> +
>> +   Where:
>> +	* 1: Encryption attribute is set
>> +	* 0: Encryption attribute is cleared
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 26d1eb83f72a..b463a81dc176 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1199,6 +1199,8 @@ struct kvm_x86_ops {
>>   	uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);
>>   
>>   	bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu);
>> +	int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
>> +				  unsigned long sz, unsigned long mode);
>>   };
>>   
>>   struct kvm_arch_async_pf {
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 3089942f6630..431718309359 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -135,6 +135,8 @@ struct kvm_sev_info {
>>   	int fd;			/* SEV device fd */
>>   	unsigned long pages_locked; /* Number of pages locked */
>>   	struct list_head regions_list;  /* List of registered regions */
>> +	unsigned long *page_enc_bmap;
>> +	unsigned long page_enc_bmap_size;
>>   };
>>   
>>   struct kvm_svm {
>> @@ -1910,6 +1912,8 @@ static void sev_vm_destroy(struct kvm *kvm)
>>   
>>   	sev_unbind_asid(kvm, sev->handle);
>>   	sev_asid_free(kvm);
>> +
>> +	kvfree(sev->page_enc_bmap);
>>   }
>>   
>>   static void avic_vm_destroy(struct kvm *kvm)
> 
> Adding Cfir who flagged this kvfree().
> 
> Other freeing of sev->page_enc_bmap in this patch also set
> sev->page_enc_bmap_size to 0 and neither set sev->page_enc_bmap to NULL
> after freeing it.
> 
> For extra safety, is it possible to sev->page_enc_bmap = NULL anytime the
> bitmap is kvfreed?
> 

Good catch, I'll fix it in next rev.

>> @@ -2084,6 +2088,7 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>>   
>>   static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>   {
>> +	struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info;
>>   	struct vcpu_svm *svm = to_svm(vcpu);
>>   	u32 dummy;
>>   	u32 eax = 1;
>> @@ -2105,6 +2110,12 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>   
>>   	if (kvm_vcpu_apicv_active(vcpu) && !init_event)
>>   		avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
>> +
>> +	/* reset the page encryption bitmap */
>> +	if (sev_guest(vcpu->kvm)) {
>> +		kvfree(sev->page_enc_bmap);
>> +		sev->page_enc_bmap_size = 0;
>> +	}
>>   }
>>   
>>   static int avic_init_vcpu(struct vcpu_svm *svm)
> 
> What is protecting sev->page_enc_bmap and sev->page_enc_bmap_size in calls
> to svm_vcpu_reset()?
> 

Yes, it need to be protected with vm lock. I will fix it in next rev.
Additionally, I think what I have here is wrong, we need to reset the
bitmap only when bsp is getting reset.

-Brijesh

Re: [PATCH v3 08/11] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS hypercall

[Peter Gonda]

>  struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 3089942f6630..431718309359 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -135,6 +135,8 @@ struct kvm_sev_info {
>         int fd;                 /* SEV device fd */
>         unsigned long pages_locked; /* Number of pages locked */
>         struct list_head regions_list;  /* List of registered regions */
> +       unsigned long *page_enc_bmap;
> +       unsigned long page_enc_bmap_size;
>  };
>

Just a high level question. Would it be better for these bitmaps to
live in kvm_memory_slot and the ioctl to be take a memslot instead of
a GPA + length? The c-bit status bitmap will probably need to be
checked at when checking the dirty log and KVM_GET_DIRTY_LOG
operations on memslots.

[PATCH v3 09/11] KVM: x86: Introduce KVM_GET_PAGE_ENC_BITMAP ioctl

[Singh, Brijesh]

The ioctl can be used to retrieve page encryption bitmap for a given
gfn range.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 Documentation/virtual/kvm/api.txt | 27 +++++++++++++++++++
 arch/x86/include/asm/kvm_host.h   |  1 +
 arch/x86/kvm/svm.c                | 44 ++++++++++++++++++++++++++++++-
 arch/x86/kvm/x86.c                | 12 +++++++++
 include/uapi/linux/kvm.h          | 12 +++++++++
 5 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 383b292966fa..ed61a4d63b10 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4081,6 +4081,33 @@ KVM_ARM_VCPU_FINALIZE call.
 See KVM_ARM_VCPU_INIT for details of vcpu features that require finalization
 using this ioctl.
 
+4.120 KVM_GET_PAGE_ENC_BITMAP (vm ioctl)
+
+Capability: basic
+Architectures: x86
+Type: vm ioctl
+Parameters: struct kvm_page_enc_bitmap (in/out)
+Returns: 0 on success, -1 on error
+
+/* for KVM_GET_PAGE_ENC_BITMAP */
+struct kvm_page_enc_bitmap {
+	__u64 start_gfn;
+	__u64 num_pages;
+	union {
+		void __user *enc_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
+The encrypted VMs have concept of private and shared pages. The private
+page is encrypted with the guest-specific key, while shared page may
+be encrypted with the hypervisor key. The KVM_GET_PAGE_ENC_BITMAP can
+be used to get the bitmap indicating whether the guest page is private
+or shared. The bitmap can be used during the guest migration, if the page
+is private then userspace need to use SEV migration commands to transmit
+the page.
+
+
 5. The kvm_run structure
 ------------------------
 
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b463a81dc176..9df0e2315543 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1201,6 +1201,7 @@ struct kvm_x86_ops {
 	bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu);
 	int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
 				  unsigned long sz, unsigned long mode);
+	int (*get_page_enc_bitmap)(struct kvm *kvm, struct kvm_page_enc_bitmap *bmap);
 };
 
 struct kvm_arch_async_pf {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 431718309359..e675fd89bb9a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7425,6 +7425,47 @@ static int svm_page_enc_status_hc(struct kvm *kvm, unsigned long gpa,
 	return ret;
 }
 
+static int svm_get_page_enc_bitmap(struct kvm *kvm,
+				   struct kvm_page_enc_bitmap *bmap)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	unsigned long gfn_start, gfn_end;
+	unsigned long *bitmap;
+	unsigned long sz, i;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	gfn_start = bmap->start_gfn;
+	gfn_end = gfn_start + bmap->num_pages;
+
+	sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
+	bitmap = kmalloc(sz, GFP_KERNEL);
+	if (!bitmap)
+		return -ENOMEM;
+
+	memset(bitmap, 0xff, sz); /* by default all pages are marked encrypted */
+
+	mutex_lock(&kvm->lock);
+	if (sev->page_enc_bmap) {
+		i = gfn_start;
+		for_each_clear_bit_from(i, sev->page_enc_bmap,
+				      min(sev->page_enc_bmap_size, gfn_end))
+			clear_bit(i - gfn_start, bitmap);
+	}
+	mutex_unlock(&kvm->lock);
+
+	ret = -EFAULT;
+	if (copy_to_user(bmap->enc_bitmap, bitmap, sz))
+		goto out;
+
+	ret = 0;
+out:
+	kfree(bitmap);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7767,7 +7808,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 
 	.need_emulation_on_page_fault = svm_need_emulation_on_page_fault,
 
-	.page_enc_status_hc = svm_page_enc_status_hc
+	.page_enc_status_hc = svm_page_enc_status_hc,
+	.get_page_enc_bitmap = svm_get_page_enc_bitmap
 };
 
 static int __init svm_init(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6baf48ec0ed4..59ae49b1b914 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4927,6 +4927,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = kvm_vm_ioctl_hv_eventfd(kvm, &hvevfd);
 		break;
 	}
+	case KVM_GET_PAGE_ENC_BITMAP: {
+		struct kvm_page_enc_bitmap bitmap;
+
+		r = -EFAULT;
+		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
+			goto out;
+
+		r = -ENOTTY;
+		if (kvm_x86_ops->get_page_enc_bitmap)
+			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index e31cdb41519f..0d08bcf94ef5 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -492,6 +492,16 @@ struct kvm_dirty_log {
 	};
 };
 
+/* for KVM_GET_PAGE_ENC_BITMAP */
+struct kvm_page_enc_bitmap {
+	__u64 start_gfn;
+	__u64 num_pages;
+	union {
+		void __user *enc_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
 /* for KVM_CLEAR_DIRTY_LOG */
 struct kvm_clear_dirty_log {
 	__u32 slot;
@@ -1451,6 +1461,8 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+#define KVM_GET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc2, struct kvm_page_enc_bitmap)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */
-- 
2.17.1

Re: [PATCH v3 09/11] KVM: x86: Introduce KVM_GET_PAGE_ENC_BITMAP ioctl

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:10PM +0000, Singh, Brijesh wrote:
> @@ -7767,7 +7808,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
>  
>  	.need_emulation_on_page_fault = svm_need_emulation_on_page_fault,
>  
> -	.page_enc_status_hc = svm_page_enc_status_hc
> +	.page_enc_status_hc = svm_page_enc_status_hc,
> +	.get_page_enc_bitmap = svm_get_page_enc_bitmap
>  };
>  
>  static int __init svm_init(void)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6baf48ec0ed4..59ae49b1b914 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4927,6 +4927,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		r = kvm_vm_ioctl_hv_eventfd(kvm, &hvevfd);
>  		break;
>  	}
> +	case KVM_GET_PAGE_ENC_BITMAP: {
> +		struct kvm_page_enc_bitmap bitmap;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
> +			goto out;
> +
> +		r = -ENOTTY;
> +		if (kvm_x86_ops->get_page_enc_bitmap)
> +			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);

I don't know what tree you've done those patches against but against -rc6+, the
first argument above needs to be vcpu->kvm:

arch/x86/kvm/x86.c: In function ‘kvm_arch_vcpu_ioctl’:
arch/x86/kvm/x86.c:4343:41: error: ‘kvm’ undeclared (first use in this function)
    r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
                                         ^~~
arch/x86/kvm/x86.c:4343:41: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [scripts/Makefile.build:280: arch/x86/kvm/x86.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [scripts/Makefile.build:497: arch/x86/kvm] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1083: arch/x86] Error 2
make: *** Waiting for unfinished jobs....

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

Re: [PATCH v3 09/11] KVM: x86: Introduce KVM_GET_PAGE_ENC_BITMAP ioctl

[Singh, Brijesh]

On 8/29/19 11:26 AM, Borislav Petkov wrote:
> On Wed, Jul 10, 2019 at 08:13:10PM +0000, Singh, Brijesh wrote:
>> @@ -7767,7 +7808,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
>>   
>>   	.need_emulation_on_page_fault = svm_need_emulation_on_page_fault,
>>   
>> -	.page_enc_status_hc = svm_page_enc_status_hc
>> +	.page_enc_status_hc = svm_page_enc_status_hc,
>> +	.get_page_enc_bitmap = svm_get_page_enc_bitmap
>>   };
>>   
>>   static int __init svm_init(void)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 6baf48ec0ed4..59ae49b1b914 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -4927,6 +4927,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>   		r = kvm_vm_ioctl_hv_eventfd(kvm, &hvevfd);
>>   		break;
>>   	}
>> +	case KVM_GET_PAGE_ENC_BITMAP: {
>> +		struct kvm_page_enc_bitmap bitmap;
>> +
>> +		r = -EFAULT;
>> +		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
>> +			goto out;
>> +
>> +		r = -ENOTTY;
>> +		if (kvm_x86_ops->get_page_enc_bitmap)
>> +			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
> 
> I don't know what tree you've done those patches against but against -rc6+, the
> first argument above needs to be vcpu->kvm:
> 
> arch/x86/kvm/x86.c: In function ‘kvm_arch_vcpu_ioctl’:
> arch/x86/kvm/x86.c:4343:41: error: ‘kvm’ undeclared (first use in this function)
>      r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
>                                           ^~~
> arch/x86/kvm/x86.c:4343:41: note: each undeclared identifier is reported only once for each function it appears in
> make[2]: *** [scripts/Makefile.build:280: arch/x86/kvm/x86.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [scripts/Makefile.build:497: arch/x86/kvm] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1083: arch/x86] Error 2
> make: *** Waiting for unfinished jobs....
> 

The patches were based on KVM tree July 9th

commit id: e9a83bd2322035ed9d7dcf35753d3f984d76c6a5

I have been waiting for some feedback before refreshing it. If you want
then I can refresh the patch with latest from Linus and send v4.

thanks

Re: [PATCH v3 09/11] KVM: x86: Introduce KVM_GET_PAGE_ENC_BITMAP ioctl

[Borislav Petkov]

On Thu, Aug 29, 2019 at 04:41:50PM +0000, Singh, Brijesh wrote:
> I have been waiting for some feedback before refreshing it. If you want
> then I can refresh the patch with latest from Linus and send v4.

That's Paolo's call but we're at -rc6 now and if it were me, I'd take
them the next round so that stuff gets tested longer as -rc6 is pretty
late in the game. IMO, of course.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

[PATCH v3 10/11] mm: x86: Invoke hypercall when page encryption status is changed

[Singh, Brijesh]

Invoke a hypercall when a memory region is changed from encrypted ->
decrypted and vice versa. Hypervisor need to know the page encryption
status during the guest migration.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/include/asm/mem_encrypt.h |  3 ++
 arch/x86/mm/mem_encrypt.c          | 45 +++++++++++++++++++++++++++++-
 arch/x86/mm/pageattr.c             | 15 ++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
index 0c196c47d621..6e654ab5a8e4 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -94,4 +94,7 @@ extern char __start_bss_decrypted[], __end_bss_decrypted[], __start_bss_decrypte
 
 #endif	/* __ASSEMBLY__ */
 
+extern void set_memory_enc_dec_hypercall(unsigned long vaddr,
+					 unsigned long size, bool enc);
+
 #endif	/* __X86_MEM_ENCRYPT_H__ */
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index e0df96fdfe46..f3fda1de2869 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -15,6 +15,7 @@
 #include <linux/dma-direct.h>
 #include <linux/swiotlb.h>
 #include <linux/mem_encrypt.h>
+#include <linux/kvm_para.h>
 
 #include <asm/tlbflush.h>
 #include <asm/fixmap.h>
@@ -25,6 +26,7 @@
 #include <asm/processor-flags.h>
 #include <asm/msr.h>
 #include <asm/cmdline.h>
+#include <asm/kvm_para.h>
 
 #include "mm_internal.h"
 
@@ -192,6 +194,45 @@ void __init sme_early_init(void)
 		swiotlb_force = SWIOTLB_FORCE;
 }
 
+void set_memory_enc_dec_hypercall(unsigned long vaddr, unsigned long sz, bool enc)
+{
+	unsigned long vaddr_end, vaddr_next;
+
+	vaddr_end = vaddr + sz;
+
+	for (; vaddr < vaddr_end; vaddr = vaddr_next) {
+		int psize, pmask, level;
+		unsigned long pfn;
+		pte_t *kpte;
+
+		kpte = lookup_address(vaddr, &level);
+		if (!kpte || pte_none(*kpte))
+			return;
+
+		switch (level) {
+		case PG_LEVEL_4K:
+			pfn = pte_pfn(*kpte);
+			break;
+		case PG_LEVEL_2M:
+			pfn = pmd_pfn(*(pmd_t *)kpte);
+			break;
+		case PG_LEVEL_1G:
+			pfn = pud_pfn(*(pud_t *)kpte);
+			break;
+		default:
+			return;
+		}
+
+		psize = page_level_size(level);
+		pmask = page_level_mask(level);
+
+		kvm_sev_hypercall3(KVM_HC_PAGE_ENC_STATUS,
+				   pfn << PAGE_SHIFT, psize >> PAGE_SHIFT, enc);
+
+		vaddr_next = (vaddr & pmask) + psize;
+	}
+}
+
 static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
 {
 	pgprot_t old_prot, new_prot;
@@ -249,12 +290,13 @@ static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
 static int __init early_set_memory_enc_dec(unsigned long vaddr,
 					   unsigned long size, bool enc)
 {
-	unsigned long vaddr_end, vaddr_next;
+	unsigned long vaddr_end, vaddr_next, start;
 	unsigned long psize, pmask;
 	int split_page_size_mask;
 	int level, ret;
 	pte_t *kpte;
 
+	start = vaddr;
 	vaddr_next = vaddr;
 	vaddr_end = vaddr + size;
 
@@ -309,6 +351,7 @@ static int __init early_set_memory_enc_dec(unsigned long vaddr,
 
 	ret = 0;
 
+	set_memory_enc_dec_hypercall(start, size, enc);
 out:
 	__flush_tlb_all();
 	return ret;
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 6a9a77a403c9..971f70f58f49 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -26,6 +26,7 @@
 #include <asm/proto.h>
 #include <asm/pat.h>
 #include <asm/set_memory.h>
+#include <asm/mem_encrypt.h>
 
 #include "mm_internal.h"
 
@@ -2020,6 +2021,12 @@ int set_memory_global(unsigned long addr, int numpages)
 				    __pgprot(_PAGE_GLOBAL), 0);
 }
 
+void __attribute__((weak)) set_memory_enc_dec_hypercall(unsigned long addr,
+							unsigned long size,
+							bool enc)
+{
+}
+
 static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
 {
 	struct cpa_data cpa;
@@ -2060,6 +2067,14 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
 	 */
 	cpa_flush(&cpa, 0);
 
+	/*
+	 * When SEV is active, notify hypervisor that a given memory range is mapped
+	 * encrypted or decrypted. Hypervisor will use this information during
+	 * the VM migration.
+	 */
+	if (sev_active())
+		set_memory_enc_dec_hypercall(addr, numpages << PAGE_SHIFT, enc);
+
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH v3 10/11] mm: x86: Invoke hypercall when page encryption status is changed

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:11PM +0000, Singh, Brijesh wrote:

> Subject: Re: [PATCH v3 10/11] mm: x86: Invoke hypercall when page encryption status is changed

Subject prefix: "x86/mm: Invoke ..."

git log <filename> would usually show you how the prefixing should look
like.

> Invoke a hypercall when a memory region is changed from encrypted ->
> decrypted and vice versa. Hypervisor need to know the page encryption
> status during the guest migration.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/include/asm/mem_encrypt.h |  3 ++
>  arch/x86/mm/mem_encrypt.c          | 45 +++++++++++++++++++++++++++++-
>  arch/x86/mm/pageattr.c             | 15 ++++++++++
>  3 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> index 0c196c47d621..6e654ab5a8e4 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -94,4 +94,7 @@ extern char __start_bss_decrypted[], __end_bss_decrypted[], __start_bss_decrypte
>  
>  #endif	/* __ASSEMBLY__ */
>  
> +extern void set_memory_enc_dec_hypercall(unsigned long vaddr,
> +					 unsigned long size, bool enc);
> +
>  #endif	/* __X86_MEM_ENCRYPT_H__ */
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index e0df96fdfe46..f3fda1de2869 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -15,6 +15,7 @@
>  #include <linux/dma-direct.h>
>  #include <linux/swiotlb.h>
>  #include <linux/mem_encrypt.h>
> +#include <linux/kvm_para.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/fixmap.h>
> @@ -25,6 +26,7 @@
>  #include <asm/processor-flags.h>
>  #include <asm/msr.h>
>  #include <asm/cmdline.h>
> +#include <asm/kvm_para.h>
>  
>  #include "mm_internal.h"
>  
> @@ -192,6 +194,45 @@ void __init sme_early_init(void)
>  		swiotlb_force = SWIOTLB_FORCE;
>  }
>  
> +void set_memory_enc_dec_hypercall(unsigned long vaddr, unsigned long sz, bool enc)
> +{
> +	unsigned long vaddr_end, vaddr_next;
> +
> +	vaddr_end = vaddr + sz;
> +
> +	for (; vaddr < vaddr_end; vaddr = vaddr_next) {
> +		int psize, pmask, level;
> +		unsigned long pfn;
> +		pte_t *kpte;
> +
> +		kpte = lookup_address(vaddr, &level);
> +		if (!kpte || pte_none(*kpte))
> +			return;
> +
> +		switch (level) {
> +		case PG_LEVEL_4K:
> +			pfn = pte_pfn(*kpte);
> +			break;
> +		case PG_LEVEL_2M:
> +			pfn = pmd_pfn(*(pmd_t *)kpte);
> +			break;
> +		case PG_LEVEL_1G:
> +			pfn = pud_pfn(*(pud_t *)kpte);
> +			break;
> +		default:
> +			return;
> +		}
> +
> +		psize = page_level_size(level);
> +		pmask = page_level_mask(level);
> +
> +		kvm_sev_hypercall3(KVM_HC_PAGE_ENC_STATUS,
> +				   pfn << PAGE_SHIFT, psize >> PAGE_SHIFT, enc);
> +
> +		vaddr_next = (vaddr & pmask) + psize;
> +	}
> +}
> +
>  static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
>  {
>  	pgprot_t old_prot, new_prot;
> @@ -249,12 +290,13 @@ static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
>  static int __init early_set_memory_enc_dec(unsigned long vaddr,
>  					   unsigned long size, bool enc)
>  {
> -	unsigned long vaddr_end, vaddr_next;
> +	unsigned long vaddr_end, vaddr_next, start;
>  	unsigned long psize, pmask;
>  	int split_page_size_mask;
>  	int level, ret;
>  	pte_t *kpte;
>  
> +	start = vaddr;
>  	vaddr_next = vaddr;
>  	vaddr_end = vaddr + size;
>  
> @@ -309,6 +351,7 @@ static int __init early_set_memory_enc_dec(unsigned long vaddr,
>  
>  	ret = 0;
>  
> +	set_memory_enc_dec_hypercall(start, size, enc);

That function iterates the same way over the virtual addresses as
early_set_memory_enc_dec() does. Please call kvm_sev_hypercall3(),
wrapped of course, directly from early_set_memory_enc_dec(), for
each iteration of the loop instead of iterating over all the virtual
addresses a second time in set_memory_enc_dec_hypercall().

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

Re: [PATCH v3 10/11] mm: x86: Invoke hypercall when page encryption status is changed

[Borislav Petkov]

On Wed, Jul 10, 2019 at 08:13:11PM +0000, Singh, Brijesh wrote:
> @@ -2060,6 +2067,14 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
>  	 */
>  	cpa_flush(&cpa, 0);
>  
> +	/*
> +	 * When SEV is active, notify hypervisor that a given memory range is mapped
> +	 * encrypted or decrypted. Hypervisor will use this information during
> +	 * the VM migration.
> +	 */
> +	if (sev_active())
> +		set_memory_enc_dec_hypercall(addr, numpages << PAGE_SHIFT, enc);

Btw, tglx has a another valid design concern here: why isn't this a
pv_ops thing? So that it is active only when the hypervisor is actually
present?

I know, I know, this will run on SEV guests only because it is all
(hopefully) behind "if (sev_active())" checks but the clean and accepted
design is a paravirt call, I'd say.

Especially if some day other hypervisors should want to run SEV guests
too...

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München

Re: [PATCH v3 10/11] mm: x86: Invoke hypercall when page encryption status is changed

[Thomas Gleixner]

On Thu, 29 Aug 2019, Borislav Petkov wrote:

> On Wed, Jul 10, 2019 at 08:13:11PM +0000, Singh, Brijesh wrote:
> > @@ -2060,6 +2067,14 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> >  	 */
> >  	cpa_flush(&cpa, 0);
> >  
> > +	/*
> > +	 * When SEV is active, notify hypervisor that a given memory range is mapped
> > +	 * encrypted or decrypted. Hypervisor will use this information during
> > +	 * the VM migration.
> > +	 */
> > +	if (sev_active())
> > +		set_memory_enc_dec_hypercall(addr, numpages << PAGE_SHIFT, enc);
> 
> Btw, tglx has a another valid design concern here: why isn't this a
> pv_ops thing? So that it is active only when the hypervisor is actually
> present?
> 
> I know, I know, this will run on SEV guests only because it is all
> (hopefully) behind "if (sev_active())" checks but the clean and accepted
> design is a paravirt call, I'd say.

No. sev_active() has nothing to do with guest mode. It tells whether SEV is
active or not. So yes, this calls into this function on both guest and
host. The latter is beyond pointless.

Thanks,

	tglx

Re: [PATCH v3 10/11] mm: x86: Invoke hypercall when page encryption status is changed

[Thomas Gleixner]

On Thu, 29 Aug 2019, Thomas Gleixner wrote:
> On Thu, 29 Aug 2019, Borislav Petkov wrote:
> 
> > On Wed, Jul 10, 2019 at 08:13:11PM +0000, Singh, Brijesh wrote:
> > > @@ -2060,6 +2067,14 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> > >  	 */
> > >  	cpa_flush(&cpa, 0);
> > >  
> > > +	/*
> > > +	 * When SEV is active, notify hypervisor that a given memory range is mapped
> > > +	 * encrypted or decrypted. Hypervisor will use this information during
> > > +	 * the VM migration.
> > > +	 */
> > > +	if (sev_active())
> > > +		set_memory_enc_dec_hypercall(addr, numpages << PAGE_SHIFT, enc);
> > 
> > Btw, tglx has a another valid design concern here: why isn't this a
> > pv_ops thing? So that it is active only when the hypervisor is actually
> > present?
> > 
> > I know, I know, this will run on SEV guests only because it is all
> > (hopefully) behind "if (sev_active())" checks but the clean and accepted
> > design is a paravirt call, I'd say.
> 
> No. sev_active() has nothing to do with guest mode. It tells whether SEV is
> active or not. So yes, this calls into this function on both guest and
> host. The latter is beyond pointless.

Oops. sme != sev.

But yes, can we please hide that a bit better....

[PATCH v3 11/11] KVM: x86: Introduce KVM_SET_PAGE_ENC_BITMAP ioctl

[Singh, Brijesh]

The ioctl can be used to set page encryption bitmap for an
incoming guest.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 Documentation/virtual/kvm/api.txt | 21 +++++++++++++++
 arch/x86/include/asm/kvm_host.h   |  1 +
 arch/x86/kvm/svm.c                | 44 ++++++++++++++++++++++++++++++-
 arch/x86/kvm/x86.c                | 12 +++++++++
 include/uapi/linux/kvm.h          |  1 +
 5 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index ed61a4d63b10..1073004f178e 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4107,6 +4107,27 @@ or shared. The bitmap can be used during the guest migration, if the page
 is private then userspace need to use SEV migration commands to transmit
 the page.
 
+4.121 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
+
+Capability: basic
+Architectures: x86
+Type: vm ioctl
+Parameters: struct kvm_page_enc_bitmap (in/out)
+Returns: 0 on success, -1 on error
+
+/* for KVM_SET_PAGE_ENC_BITMAP */
+struct kvm_page_enc_bitmap {
+	__u64 start_gfn;
+	__u64 num_pages;
+	union {
+		void __user *enc_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
+During the guest live migration the outgoing guest exports its page encryption
+bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
+bitmap for an incoming guest.
 
 5. The kvm_run structure
 ------------------------
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9df0e2315543..369a06b275d5 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1202,6 +1202,7 @@ struct kvm_x86_ops {
 	int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
 				  unsigned long sz, unsigned long mode);
 	int (*get_page_enc_bitmap)(struct kvm *kvm, struct kvm_page_enc_bitmap *bmap);
+	int (*set_page_enc_bitmap)(struct kvm *kvm, struct kvm_page_enc_bitmap *bmap);
 };
 
 struct kvm_arch_async_pf {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index e675fd89bb9a..31653e8d5927 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7466,6 +7466,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
 	return ret;
 }
 
+static int svm_set_page_enc_bitmap(struct kvm *kvm,
+				   struct kvm_page_enc_bitmap *bmap)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	unsigned long gfn_start, gfn_end;
+	unsigned long *bitmap;
+	unsigned long sz, i;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	gfn_start = bmap->start_gfn;
+	gfn_end = gfn_start + bmap->num_pages;
+
+	sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
+	bitmap = kmalloc(sz, GFP_KERNEL);
+	if (!bitmap)
+		return -ENOMEM;
+
+	ret = -EFAULT;
+	if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
+		goto out;
+
+	mutex_lock(&kvm->lock);
+	ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
+	if (ret)
+		goto unlock;
+
+	i = gfn_start;
+	for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
+		clear_bit(i + gfn_start, sev->page_enc_bmap);
+
+	ret = 0;
+unlock:
+	mutex_unlock(&kvm->lock);
+out:
+	kfree(bitmap);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -7809,7 +7850,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 	.need_emulation_on_page_fault = svm_need_emulation_on_page_fault,
 
 	.page_enc_status_hc = svm_page_enc_status_hc,
-	.get_page_enc_bitmap = svm_get_page_enc_bitmap
+	.get_page_enc_bitmap = svm_get_page_enc_bitmap,
+	.set_page_enc_bitmap = svm_set_page_enc_bitmap
 };
 
 static int __init svm_init(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 59ae49b1b914..ea21cb1cbfb7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4939,6 +4939,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
 		break;
 	}
+	case KVM_SET_PAGE_ENC_BITMAP: {
+		struct kvm_page_enc_bitmap bitmap;
+
+		r = -EFAULT;
+		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
+			goto out;
+
+		r = -ENOTTY;
+		if (kvm_x86_ops->set_page_enc_bitmap)
+			r = kvm_x86_ops->set_page_enc_bitmap(kvm, &bitmap);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0d08bcf94ef5..90dd336e8080 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1462,6 +1462,7 @@ struct kvm_enc_region {
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
 #define KVM_GET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc2, struct kvm_page_enc_bitmap)
+#define KVM_SET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc3, struct kvm_page_enc_bitmap)
 
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
-- 
2.17.1

Re: [PATCH v3 11/11] KVM: x86: Introduce KVM_SET_PAGE_ENC_BITMAP ioctl

[Steve Rutherford]

On Wed, Jul 10, 2019 at 1:13 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:
>
>  struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index e675fd89bb9a..31653e8d5927 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7466,6 +7466,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
>         return ret;
>  }
>
> +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> +                                  struct kvm_page_enc_bitmap *bmap)
> +{
> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +       unsigned long gfn_start, gfn_end;
> +       unsigned long *bitmap;
> +       unsigned long sz, i;
> +       int ret;
> +
> +       if (!sev_guest(kvm))
> +               return -ENOTTY;
> +
> +       gfn_start = bmap->start_gfn;
> +       gfn_end = gfn_start + bmap->num_pages;
> +
> +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> +       bitmap = kmalloc(sz, GFP_KERNEL);

This kmalloc should probably be either a vmalloc or kvmalloc. The max
size, if I'm reading kmalloc correctly, is 2^10 pages. That's 4MB,
which should correspond to a bitmap for a 128GB VM, which is a
plausible VM size.

--Steve

Re: [PATCH v3 11/11] KVM: x86: Introduce KVM_SET_PAGE_ENC_BITMAP ioctl

[Steve Rutherford]

On Thu, Nov 14, 2019 at 5:22 PM Steve Rutherford <srutherford@google.com> wrote:
>
> On Wed, Jul 10, 2019 at 1:13 PM Singh, Brijesh <brijesh.singh@amd.com> wrote:
> >
> >  struct kvm_arch_async_pf {
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index e675fd89bb9a..31653e8d5927 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -7466,6 +7466,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
> >         return ret;
> >  }
> >
> > +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> > +                                  struct kvm_page_enc_bitmap *bmap)
> > +{
> > +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > +       unsigned long gfn_start, gfn_end;
> > +       unsigned long *bitmap;
> > +       unsigned long sz, i;
> > +       int ret;
> > +
> > +       if (!sev_guest(kvm))
> > +               return -ENOTTY;
> > +
> > +       gfn_start = bmap->start_gfn;
> > +       gfn_end = gfn_start + bmap->num_pages;
> > +
> > +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> > +       bitmap = kmalloc(sz, GFP_KERNEL);
>
> This kmalloc should probably be either a vmalloc or kvmalloc. The max
> size, if I'm reading kmalloc correctly, is 2^10 pages. That's 4MB,
> which should correspond to a bitmap for a 128GB VM, which is a
> plausible VM size.
>
> --Steve

Ignore this, you are clearly never going to copy that much in one
call, and the backing bitmap correctly uses kvmalloc.