Re: [PATCH] kvm: VMX: do not use vm-exit instruction length for fast MMIO

[Michael S. Tsirkin]

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