Re: [PATCH] x86: kvm: Avoid guest page table walk when gpa_available is set

[Brijesh Singh]

> > I also wanted to avoid adding yet another variable but we can't depend on
> > cr2 parameters passed into x86_emulate_instruction().
> >
> > The x86_emulate_instruction() function is called from two places:
> >
> > 1) handling the page-fault.
> > pf_interception [svm.c]
> >  kvm_mmu_page_fault [mmu.c]
> >   x86_emulate_instruction [x86.c]
> >
> > 2) completing the IO/MMIO's from previous instruction decode
> > kvm_arch_vcpu_ioctl_run
> >  complete_emulated_io
> >    emulate_instruction
> >     x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0)
> >
> > In #1, we are guaranteed that cr2 variable will contain a valid GPA but
> > in #2, CR2 is set to zero.
>
> We are setting up the completion in #1 x86_emulate_instruction(), where
> the gpa (cr2) is available, so we could store the value while arming
> vcpu->arch.complete_userspace_io.
>
> emulator_read_write_onepage() already saves gpa in frag->gpa, which is
> then passed into complete_emulated_mmio -- isn't that mechanism
> sufficient?

I see that complete_emulated_mmio() saves the frag>gpa into run->mmio.phys_addr,
so based on the exit_reason we should be able to get the saved gpa.
In my debug patch below, I tried doing something similar to verify that frag->gpa
contains the valid CR2 value but I saw a bunch of mismatch. So it seems like we
may not able to use frag->gpa mechanism. Additionally we also need to handle the
PIO cases (e.g what if we are called from complete_emulated_pio), which also takes
similar code path

complete_emulated_pio
 completed_emulated_io
   emulate_instruction
    x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0)

@@ -5682,13 +5686,20 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 
 restart:
        /*
-        * Save the faulting GPA (cr2) in the address field
-        * NOTE: If gpa_available is set then gpa_val will contain a valid GPA
+        * if previous exit was due to userspace mmio completion then actual
+        * cr2 is stored in mmio.phys_addr.
         */
-       if (vcpu->arch.gpa_available)
-               ctxt->exception.address = vcpu->arch.gpa_val;
-       else
-               ctxt->exception.address = cr2;
+       if (vcpu->run->exit_reason == KVM_EXIT_MMIO) {
+               cr2 = vcpu->run->mmio.phys_addr;
+               if (cr2 != vcpu->arch.gpa_val)
+                       pr_err("** mismatch %llx %llx\n",
+                               vcpu->run->mmio.phys_addr, vcpu->arch.gpa_val);
+       }
+
+        /* Save the faulting GPA (cr2) in the address field */
+       ctxt->exception.address = cr2;


> > handle_exit [svm.c]
> >  pf_interception  [svm.c]
> > /* it invokes the fault handler with CR2 = svm->vmcb->control.exit_info_2 */
> >   kvm_mmu_page_fault [mmu.c]
> >    x86_emulate_instruction  [x86.c]
> >     emulator_read_write_onepage  [x86.c]
> >      /*
> >       *this is where we walk the guest page table to translate
> >       * a GVA to GPA. If gpa_available is set then we use the
> >       * gpa_val instead of walking the pgtable.
> >       */
>
> pf_interception is the NPF exit handler -- please move the setting
> there, at least.  handle_exit() is a hot path that shouldn't contain
> code that isn't applicable to all exits.

Sure, Will do.

> Btw. we need some other guarantees to declare it as GPA (cr2 is GPA in
> NPT exits, but might not be in other) ... isn't arch.mmu.direct_map a
> condition we are interested in?
>
> The other code uses it to interpret cr2 directly as gpa, so we might be
> able to avoid setting the arch.gpa_available in a hot path too.

Hmm looking at the call trace I am not sure how arch.mmu_direct_map will help
but I will investigate a bit more.

> > See my previous comment. In some cases CR2 may be set to zero
> > (e.g when completing the instruction from previous io/mmio page-fault).
> >
> > If we are decide to add the gpa_val then we can remove above if
> > statement from x86_emulate_instruction() and update emulator_read_write_onepage
> > to use the vcpu->arch.gpa_val instead of exception->address.
>
> Yeah, that would be nicer than setting exception->address at a random
> place.
>
> We could also pass the value as cr2 in all cases if it made something
> better.
>
> > if (vcpu->arch.gpa_available &&
> >      emulator_can_use_gpa(ctxt) &&
> >      (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
> >       gpa = vcpu=>arch.gpa_val;
> >       ...
> >       ...
> >    }
>
> If at all possible, I'd like to have the gpa passed with other relevant
> data, instead of having it isolated like this ... and we can't manage
> that, then at least good benchmark results to excuse the bad code.

I ran two tests to see how many times we walk  guest page table.

Test1: run kvm-unit-test
Test2: launch Ubuntu 16.06 guest, run a stressapptest for 20 seconds, shutdown the VM

Before patch
  * Test1: 10419
  * Test2: 243365

After patch:
  * Test1:  1259
  * Test2:  1221

Please let me know if you want me to run other other benchmark and capture the data.

-Brijesh