Re: [PATCH 2/2] KVM: SVM: Add support for VMCB address check change

[Maxim Levitsky <mlevitsk@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]

On Tue, 2021-01-12 at 11:18 -0800, Sean Christopherson wrote:
> On Tue, Jan 12, 2021, Wei Huang wrote:
> > New AMD CPUs have a change that checks VMEXIT intercept on special SVM
> > instructions before checking their EAX against reserved memory region.
> > This change is indicated by CPUID_0x8000000A_EDX[28]. If it is 1, KVM
> > doesn't need to intercept and emulate #GP faults for such instructions
> > because #GP isn't supposed to be triggered.
> > 
> > Co-developed-by: Bandan Das <bsd@redhat.com>
> > Signed-off-by: Bandan Das <bsd@redhat.com>
> > Signed-off-by: Wei Huang <wei.huang2@amd.com>
> > ---
> >  arch/x86/include/asm/cpufeatures.h | 1 +
> >  arch/x86/kvm/svm/svm.c             | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > index 84b887825f12..ea89d6fdd79a 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -337,6 +337,7 @@
> >  #define X86_FEATURE_AVIC		(15*32+13) /* Virtual Interrupt Controller */
> >  #define X86_FEATURE_V_VMSAVE_VMLOAD	(15*32+15) /* Virtual VMSAVE VMLOAD */
> >  #define X86_FEATURE_VGIF		(15*32+16) /* Virtual GIF */
> > +#define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* "" SVME addr check */
> 
> Heh, KVM should advertise this to userspace by setting the kvm_cpu_cap bit.  KVM
> KVM forwards relevant VM-Exits to L1 without checking if rAX points at an
> invalid L1 GPA.


I agree that we should be able to fix/hide the errata from the L1,
and expose this bit to L1 to avoid it trying to apply this workaround
itself when it itself runs nested guests.

Note that there is currently a bug in this patch series, that prevents
this workaround to work for a guest that runs nested guests itself (e.g L3):

(when we intercept the #GP, and we are running
a nested guest, we should do a vmexit with SVM_EXIT_VMRUN/VMSAVE/etc exit
reason instead of running the instruction), but this can be fixed,
I did it locally and it works.

(lightly tested) patch for that attached.

Best regards,
	Maxim Levitsky
> 
> >  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
> >  #define X86_FEATURE_AVX512VBMI		(16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 74620d32aa82..451b82df2eab 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -311,7 +311,7 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
> >  	svm->vmcb->save.efer = efer | EFER_SVME;
> >  	vmcb_mark_dirty(svm->vmcb, VMCB_CR);
> >  	/* Enable GP interception for SVM instructions if needed */
> > -	if (efer & EFER_SVME)
> > +	if ((efer & EFER_SVME) && !boot_cpu_has(X86_FEATURE_SVME_ADDR_CHK))
> >  		set_exception_intercept(svm, GP_VECTOR);
> >  
> >  	return 0;
> > -- 
> > 2.27.0
> > 






[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1315 bytes --]

commit 28ab89aaa11380306bafbf49265222f2a2da71da
Author: Maxim Levitsky <mlevitsk@redhat.com>
Date:   Thu Jan 14 10:53:25 2021 +0200

    kvm: x86: fix that errata for nested guests

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c31e005252d69..9cfa5946fac69 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2027,6 +2027,26 @@ static int svm_emulate_vm_instr(struct kvm_vcpu *vcpu, u8 modrm)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (is_guest_mode(vcpu)) {
+		switch (modrm) {
+		case 0xd8: /* VMRUN */
+			svm->vmcb->control.exit_code = SVM_EXIT_VMRUN;
+			break;
+		case 0xda: /* VMLOAD */
+			svm->vmcb->control.exit_code = SVM_EXIT_VMLOAD;
+			break;
+		case 0xdb: /* VMSAVE */
+			svm->vmcb->control.exit_code = SVM_EXIT_VMLOAD;
+			break;
+		default:
+			goto inject_exception;
+		}
+
+		svm->vmcb->control.exit_info_1 = 0;
+		svm->vmcb->control.exit_info_2 = 0;
+		return nested_svm_vmexit(svm);
+	}
+
 	switch (modrm) {
 	case 0xd8: /* VMRUN */
 		return vmrun_interception(svm);
@@ -2035,6 +2055,7 @@ static int svm_emulate_vm_instr(struct kvm_vcpu *vcpu, u8 modrm)
 	case 0xdb: /* VMSAVE */
 		return vmsave_interception(svm);
 	default:
+inject_exception:
 		/* inject a #GP for all other cases */
 		kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
 		return 1;