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

Brijesh Singh
2017-04-25T22:03:08Z

>>
>> 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