Re: [PATCH v2 1/6] KVM: SVM: Use maxphyaddr in emulator RAX check for VMRUN/VMLOAD/VMSAVE

[Sean Christopherson <seanjc@google.com>]

On Wed, Mar 11, 2026, Yosry Ahmed wrote:
> > > > Sean, I intend to send a new version today with 2 main diffs:
> > > > - Use cpuid_maxphyaddr() here instead of  kvm_host.maxphyaddr.
> > > > - Use a common helper for checking RAX for SVM instructions for both
> > > > the emulator and gp_interception() (see response on patch 4).
> > > >
> > > > Holler if you want me to wait for further feedback.
> > >
> > > I just realized I cannot just do cpuid_maxphyaddr(ctxt->vcpu) in
> > > check_svme_pa(), because vcpu is defined as a void pointer in
> > > x86_emulate_ctxt. Looking at commit c9b8b07cded5 ("KVM: x86:
> > > Dynamically allocate per-vCPU emulation context"), I cannot tell why.
> >
> > To prevent dereferencing the vcpu object in emulator code.  It's kinda silly
> > because common KVM is tightly coupled to the emulator, but we try to contain
> > what the emulator can do.
> >
> > > I was going to move emul_to_vcpu() to arch/x86/kvm/kvm_emulate.h, but
> > > maybe we should just make this a normal struct kvm_vpcu pointer and
> > > drop emul_to_vcpu() completely?
> >
> > Heh, talk about scope creep, that'll open a much bigger can of worms and subsequent
> > discussion.
> 
> Ack.
> 
> > Honestly, why bother keeping check_svme_pa()?  Can't we just do the checks in
> > svm_check_intercept()?  E.g. vmx_check_intercept() already "injects" #UD for RDTSCP.
> 
> Hmm svm_check_intercept() isn't semantically the right place AFAICT,

Actually, I think it is.  Per the APM:

  Generally, instruction intercepts are checked after simple exceptions (such as
  #GP—when CPL is incorrect—or #UD) have been checked, but before exceptions
  related to memory accesses (such as page faults) and exceptions based on
  specific operand values.

This #GP is operand specific.

> and more importantly, it's only called if the instruction is executed
> in guest mode (i.e. in L2+).

Hold up, we're getting ahead of ourselves.

The only legitimate reason the emulator is at all aware of VMSAVE, VMLOAD, and
VMRUN is to deal with #GP due to the RAX check, because hardware checks the GPA
against the host's physical address space.  See commit 82a11e9c6fa2 ("KVM: SVM:
Add emulation support for #GP triggered by SVM instructions").

The emulator "support" was originally added by commit 01de8b09e606 ("KVM: SVM:
Add intercept checks for SVM instructions"), but AFAICT, for all intents and
purposes that was dead code when it was added, because the emulator doesn't
actually _emulate_ the instructions.  I assume if they aren't intercepted, and
KVM is full on emulating instead of just decoding, they end up at EMULATION_FAILED
and get a #UD or something.

Outside of forced emulation or code stream rewriting, KVM should _never_ fully
emulate any of the SVM instructions except VMMCALL (and that is a super special
case).  KVM does need to _decode_ the instruction, and it needs to get the
pre-intercept exception checks correct so that KVM correctly injects e.g. #GP
instead of synthesizing a #VMEXIT for the CPL check, but KVM doesn't need to do
*all* of the checks.

Note, for L2, the SVME check is meaningless, as EFER.SVME has to be set for L2
to be active, i.e. it's L1's responsibility to handle that check.

Back to the physical address thing, KVM _already_ handles that check in the #GP
path, it's just wrong too:

		/* All SVM instructions expect page aligned RAX */
		if (svm->vmcb->save.rax & ~PAGE_MASK)
			goto reinject;

So I think what we want is to

  (a) fix the RAX check in gp_interception()
  (b) drop the RAX check in the emulator
  (c) add a CPL check in the emulator (because the intercepted #GP could have
      been due to L2 executing at CPL>0, not due to a bad-but-good RAX).

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c8e292e9a24d..74df977a38ca 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3867,18 +3867,10 @@ static int check_svme(struct x86_emulate_ctxt *ctxt)
        if (!(efer & EFER_SVME))
                return emulate_ud(ctxt);
 
-       return X86EMUL_CONTINUE;
-}
-
-static int check_svme_pa(struct x86_emulate_ctxt *ctxt)
-{
-       u64 rax = reg_read(ctxt, VCPU_REGS_RAX);
-
-       /* Valid physical address? */
-       if (rax & 0xffff000000000000ULL)
+       if (ctxt->ops->cpl(ctxt))
                return emulate_gp(ctxt, 0);
 
-       return check_svme(ctxt);
+       return X86EMUL_CONTINUE;
 }
 
 static int check_rdtsc(struct x86_emulate_ctxt *ctxt)
@@ -3984,10 +3976,10 @@ static const struct opcode group7_rm2[] = {
 };
 
 static const struct opcode group7_rm3[] = {
-       DIP(SrcNone | Prot | Priv,              vmrun,          check_svme_pa),
+       DIP(SrcNone | Prot | Priv,              vmrun,          check_svme),
        II(SrcNone  | Prot | EmulateOnUD,       em_hypercall,   vmmcall),
-       DIP(SrcNone | Prot | Priv,              vmload,         check_svme_pa),
-       DIP(SrcNone | Prot | Priv,              vmsave,         check_svme_pa),
+       DIP(SrcNone | Prot | Priv,              vmload,         check_svme),
+       DIP(SrcNone | Prot | Priv,              vmsave,         check_svme),
        DIP(SrcNone | Prot | Priv,              stgi,           check_svme),
        DIP(SrcNone | Prot | Priv,              clgi,           check_svme),
        DIP(SrcNone | Prot | Priv,              skinit,         check_svme),
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e6691c044913..e1223c07593b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2294,7 +2294,7 @@ static int gp_interception(struct kvm_vcpu *vcpu)
                                EMULTYPE_VMWARE_GP | EMULTYPE_NO_DECODE);
        } else {
                /* All SVM instructions expect page aligned RAX */
-               if (svm->vmcb->save.rax & ~PAGE_MASK)
+               if (!page_address_valid(vcpu, svm->vmcb->save.rax))
                        goto reinject;
 
                return emulate_svm_instr(vcpu, opcode);