[PATCH 0/2] KVM: SEV: Fall back to __vmalloc() for SEV-ES scratch

[PATCH 0/2] KVM: SEV: Fall back to __vmalloc() for SEV-ES scratch

[Sean Christopherson]

Two cleanups related to handling failure to setup the GHCB scratch area
for SEV-ES.  Found by inspection when reviewing the SNP series.  Compile
tested only.

Sean Christopherson (2):
  KVM: SEV: Return appropriate error codes if SEV-ES scratch setup fails
  KVM: SEV: Fall back to __vmalloc() for SEV-ES scratch area if
    necessary

 arch/x86/kvm/svm/sev.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH 1/2] KVM: SEV: Return appropriate error codes if SEV-ES scratch setup fails

[Sean Christopherson]

Return appropriate error codes if setting up the GHCB scratch area for an
SEV-ES guest fails.  In particular, returning -EINVAL instead of -ENOMEM
when allocating the kernel buffer could be confusing as userspace would
likely suspect a guest issue.

Fixes: 8f423a80d299 ("KVM: SVM: Support MMIO for an SEV-ES guest")
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 3e2769855e51..ea8069c9b5cb 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2299,7 +2299,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
 }
 
 #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
-static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
+static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;
 	struct ghcb *ghcb = svm->ghcb;
@@ -2310,14 +2310,14 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
 	if (!scratch_gpa_beg) {
 		pr_err("vmgexit: scratch gpa not provided\n");
-		return false;
+		return -EINVAL;
 	}
 
 	scratch_gpa_end = scratch_gpa_beg + len;
 	if (scratch_gpa_end < scratch_gpa_beg) {
 		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
 		       len, scratch_gpa_beg);
-		return false;
+		return -EINVAL;
 	}
 
 	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
@@ -2335,7 +2335,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 		    scratch_gpa_end > ghcb_scratch_end) {
 			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
 			       scratch_gpa_beg, scratch_gpa_end);
-			return false;
+			return -EINVAL;
 		}
 
 		scratch_va = (void *)svm->ghcb;
@@ -2348,18 +2348,18 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 		if (len > GHCB_SCRATCH_AREA_LIMIT) {
 			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
 			       len, GHCB_SCRATCH_AREA_LIMIT);
-			return false;
+			return -EINVAL;
 		}
 		scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);
 		if (!scratch_va)
-			return false;
+			return -ENOMEM;
 
 		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
 			/* Unable to copy scratch area from guest */
 			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
 
 			kfree(scratch_va);
-			return false;
+			return -EFAULT;
 		}
 
 		/*
@@ -2375,7 +2375,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	svm->ghcb_sa = scratch_va;
 	svm->ghcb_sa_len = len;
 
-	return true;
+	return 0;
 }
 
 static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
@@ -2514,10 +2514,10 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 	ghcb_set_sw_exit_info_1(ghcb, 0);
 	ghcb_set_sw_exit_info_2(ghcb, 0);
 
-	ret = -EINVAL;
 	switch (exit_code) {
 	case SVM_VMGEXIT_MMIO_READ:
-		if (!setup_vmgexit_scratch(svm, true, control->exit_info_2))
+		ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
+		if (ret)
 			break;
 
 		ret = kvm_sev_es_mmio_read(vcpu,
@@ -2526,7 +2526,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 					   svm->ghcb_sa);
 		break;
 	case SVM_VMGEXIT_MMIO_WRITE:
-		if (!setup_vmgexit_scratch(svm, false, control->exit_info_2))
+		ret = setup_vmgexit_scratch(svm, false, control->exit_info_2);
+		if (ret)
 			break;
 
 		ret = kvm_sev_es_mmio_write(vcpu,
@@ -2569,6 +2570,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		vcpu_unimpl(vcpu,
 			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",
 			    control->exit_info_1, control->exit_info_2);
+		ret = -EINVAL;
 		break;
 	default:
 		ret = svm_invoke_exit_handler(vcpu, exit_code);
@@ -2579,8 +2581,11 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 
 int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
 {
-	if (!setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2))
-		return -EINVAL;
+	int r;
+
+	r = setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2);
+	if (r)
+		return r;
 
 	return kvm_sev_es_string_io(&svm->vcpu, size, port,
 				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH 1/2] KVM: SEV: Return appropriate error codes if SEV-ES scratch setup fails

[Tom Lendacky]

On 11/9/21 4:23 PM, Sean Christopherson wrote:
> Return appropriate error codes if setting up the GHCB scratch area for an
> SEV-ES guest fails.  In particular, returning -EINVAL instead of -ENOMEM
> when allocating the kernel buffer could be confusing as userspace would
> likely suspect a guest issue.

Based on previous feedback and to implement the changes to the GHCB 
specification, I'm planning on submitting a patch that will return an 
error code back to the guest, instead of terminating the guest, if the 
scratch area fails to be setup properly. So you could hold off on this 
patch if you want.

Thanks,
Tom

> 
> Fixes: 8f423a80d299 ("KVM: SVM: Support MMIO for an SEV-ES guest")
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/sev.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 3e2769855e51..ea8069c9b5cb 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2299,7 +2299,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>   }
>   
>   #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> -static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
> +static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   {
>   	struct vmcb_control_area *control = &svm->vmcb->control;
>   	struct ghcb *ghcb = svm->ghcb;
> @@ -2310,14 +2310,14 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
>   	if (!scratch_gpa_beg) {
>   		pr_err("vmgexit: scratch gpa not provided\n");
> -		return false;
> +		return -EINVAL;
>   	}
>   
>   	scratch_gpa_end = scratch_gpa_beg + len;
>   	if (scratch_gpa_end < scratch_gpa_beg) {
>   		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
>   		       len, scratch_gpa_beg);
> -		return false;
> +		return -EINVAL;
>   	}
>   
>   	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
> @@ -2335,7 +2335,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   		    scratch_gpa_end > ghcb_scratch_end) {
>   			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
>   			       scratch_gpa_beg, scratch_gpa_end);
> -			return false;
> +			return -EINVAL;
>   		}
>   
>   		scratch_va = (void *)svm->ghcb;
> @@ -2348,18 +2348,18 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   		if (len > GHCB_SCRATCH_AREA_LIMIT) {
>   			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
>   			       len, GHCB_SCRATCH_AREA_LIMIT);
> -			return false;
> +			return -EINVAL;
>   		}
>   		scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);
>   		if (!scratch_va)
> -			return false;
> +			return -ENOMEM;
>   
>   		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
>   			/* Unable to copy scratch area from guest */
>   			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
>   
>   			kfree(scratch_va);
> -			return false;
> +			return -EFAULT;
>   		}
>   
>   		/*
> @@ -2375,7 +2375,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   	svm->ghcb_sa = scratch_va;
>   	svm->ghcb_sa_len = len;
>   
> -	return true;
> +	return 0;
>   }
>   
>   static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
> @@ -2514,10 +2514,10 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   	ghcb_set_sw_exit_info_1(ghcb, 0);
>   	ghcb_set_sw_exit_info_2(ghcb, 0);
>   
> -	ret = -EINVAL;
>   	switch (exit_code) {
>   	case SVM_VMGEXIT_MMIO_READ:
> -		if (!setup_vmgexit_scratch(svm, true, control->exit_info_2))
> +		ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
> +		if (ret)
>   			break;
>   
>   		ret = kvm_sev_es_mmio_read(vcpu,
> @@ -2526,7 +2526,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   					   svm->ghcb_sa);
>   		break;
>   	case SVM_VMGEXIT_MMIO_WRITE:
> -		if (!setup_vmgexit_scratch(svm, false, control->exit_info_2))
> +		ret = setup_vmgexit_scratch(svm, false, control->exit_info_2);
> +		if (ret)
>   			break;
>   
>   		ret = kvm_sev_es_mmio_write(vcpu,
> @@ -2569,6 +2570,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   		vcpu_unimpl(vcpu,
>   			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",
>   			    control->exit_info_1, control->exit_info_2);
> +		ret = -EINVAL;
>   		break;
>   	default:
>   		ret = svm_invoke_exit_handler(vcpu, exit_code);
> @@ -2579,8 +2581,11 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   
>   int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>   {
> -	if (!setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2))
> -		return -EINVAL;
> +	int r;
> +
> +	r = setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2);
> +	if (r)
> +		return r;
>   
>   	return kvm_sev_es_string_io(&svm->vcpu, size, port,
>   				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
> 

Re: [PATCH 1/2] KVM: SEV: Return appropriate error codes if SEV-ES scratch setup fails

[Paolo Bonzini]

On 11/11/21 16:14, Tom Lendacky wrote:
> 
>> Return appropriate error codes if setting up the GHCB scratch area for an
>> SEV-ES guest fails.  In particular, returning -EINVAL instead of -ENOMEM
>> when allocating the kernel buffer could be confusing as userspace would
>> likely suspect a guest issue.
> 
> Based on previous feedback and to implement the changes to the GHCB 
> specification, I'm planning on submitting a patch that will return an 
> error code back to the guest, instead of terminating the guest, if the 
> scratch area fails to be setup properly. So you could hold off on this 
> patch if you want.

I think we still want these two patches in 5.16.

Paolo

Re: [PATCH 1/2] KVM: SEV: Return appropriate error codes if SEV-ES scratch setup fails

[Tom Lendacky]

On 11/11/21 9:47 AM, Paolo Bonzini wrote:
> On 11/11/21 16:14, Tom Lendacky wrote:
>>
>>> Return appropriate error codes if setting up the GHCB scratch area for an
>>> SEV-ES guest fails.  In particular, returning -EINVAL instead of -ENOMEM
>>> when allocating the kernel buffer could be confusing as userspace would
>>> likely suspect a guest issue.
>>
>> Based on previous feedback and to implement the changes to the GHCB 
>> specification, I'm planning on submitting a patch that will return an 
>> error code back to the guest, instead of terminating the guest, if the 
>> scratch area fails to be setup properly. So you could hold off on this 
>> patch if you want.
> 
> I think we still want these two patches in 5.16.

Ok, I'll rebase my changes on top of these then once you push them.

Thanks,
Tom

> 
> Paolo
> 

[PATCH 2/2] KVM: SEV: Fall back to __vmalloc() for SEV-ES scratch area if necessary

[Sean Christopherson]

Use kvzalloc() to allocate KVM's buffer for SEV-ES's GHCB scratch area so
that KVM falls back to __vmalloc() if physically contiguous memory isn't
available.  The buffer is purely a KVM software construct, i.e. there's
no need for it to be physically contiguous, and at a max allowed size of
16kb it's just large enough that kzalloc() could feasibly fail due to
memory fragmentation.

Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ea8069c9b5cb..2d5a0eea27ea 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2030,7 +2030,7 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
 	__free_page(virt_to_page(svm->vmsa));
 
 	if (svm->ghcb_sa_free)
-		kfree(svm->ghcb_sa);
+		kvfree(svm->ghcb_sa);
 }
 
 static void dump_ghcb(struct vcpu_svm *svm)
@@ -2262,7 +2262,7 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 			svm->ghcb_sa_sync = false;
 		}
 
-		kfree(svm->ghcb_sa);
+		kvfree(svm->ghcb_sa);
 		svm->ghcb_sa = NULL;
 		svm->ghcb_sa_free = false;
 	}
@@ -2350,7 +2350,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 			       len, GHCB_SCRATCH_AREA_LIMIT);
 			return -EINVAL;
 		}
-		scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);
+		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
 		if (!scratch_va)
 			return -ENOMEM;
 
@@ -2358,7 +2358,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 			/* Unable to copy scratch area from guest */
 			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
 
-			kfree(scratch_va);
+			kvfree(scratch_va);
 			return -EFAULT;
 		}
 
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH 2/2] KVM: SEV: Fall back to __vmalloc() for SEV-ES scratch area if necessary

[Paolo Bonzini]

On 11/9/21 23:23, Sean Christopherson wrote:
> The buffer is purely a KVM software construct, i.e. there's
> no need for it to be physically contiguous, and at a max allowed size of
> 16kb it's just large enough that kzalloc() could feasibly fail due to
> memory fragmentation.

That isn't entirely correct, as kzalloc() won't really fail for requests 
up to 32kb, i.e. order <= PAGE_ALLOC_COSTLY_ORDER.  However, the 
reasoning is correct so I queued the patches.

Paolo