On Wed, Aug 16, 2017 at 03:34:54PM +0200, Paolo Bonzini wrote:
> Microsoft pointed out privately to me that KVM's handling of
> KVM_FAST_MMIO_BUS is invalid. Using skip_emulation_instruction is invalid
> in EPT misconfiguration vmexit handlers, because neither EPT violations
> nor misconfigurations are listed in the manual among the VM exits that
> set the VM-exit instruction length field.
>
> While physical processors seem to set the field, this is not architectural
> and is just a side effect of the implementation. I couldn't convince
> myself of any condition on the exit qualification where VM-exit
> instruction length "has" to be defined; there are no trap-like VM-exits
> that can be repurposed; and fault-like VM-exits such as descriptor-table
> exits provide no decoding information. So I don't really see any way
> to keep the full speedup.
>
> What we can do is use EMULTYPE_SKIP; it only saves 200 clock cycles
> because computing the physical RIP and reading the instruction is
> expensive, but at least the eventfd is signaled before entering the
> emulator. This saves on latency. While at it, don't check breakpoints
> when skipping the instruction, as presumably any side effect has been
> exposed already.
>
> Adding a hypercall or MSR write that does a fast MMIO write to a physical
> address would do it, but it adds hypervisor knowledge in virtio, including
> CPUID handling. So it would be pretty ugly in the guest-side implementation,
> but if somebody wants to do it and the virtio side is acceptable to the
> virtio maintainers, I am okay with it.
>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 68c3b4d1676d870f0453c31d5a52e7e65c7448ae
> Suggested-by: Radim Krčmář <rkrcmar@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Jason (cc) who worked on the original optimization said he can
work to test the performance impact.
I suggest we don't rush this (it's been like this for 2 years),
and the issue seems to be largely theoretical.
> ---
> arch/x86/kvm/vmx.c | 3 ++-
> arch/x86/kvm/x86.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index df8d2f127508..5ec47fd0b990 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6407,7 +6407,8 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
> gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
> trace_kvm_fast_mmio(gpa);
> - return kvm_skip_emulated_instruction(vcpu);
> + return x86_emulate_instruction(vcpu, gpa, EMULTYPE_SKIP,
> + NULL, 0) == EMULATE_DONE;
> }
>
> ret = handle_mmio_page_fault(vcpu, gpa, true);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e10eda86bc7b..e74b79dab343 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5654,7 +5654,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> * handle watchpoints yet, those would be handled in
> * the emulate_ops.
> */
> - if (kvm_vcpu_check_breakpoint(vcpu, &r))
> + if (!(emulation_type & EMULTYPE_SKIP) &&
> + kvm_vcpu_check_breakpoint(vcpu, &r))
> return r;
>
> ctxt->interruptibility = 0;
> --
> 1.8.3.1
