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

[Yang Zhang]

On 2017/8/17 16:51, Wanpeng Li wrote:
> 2017-08-17 16:48 GMT+08:00 Yang Zhang <yang.zhang.wz@xxxxxxxxx>:
> > On 2017/8/17 16:31, Wanpeng Li wrote:
> > > 2017-08-17 16:28 GMT+08:00 Wanpeng Li <kernellwp@xxxxxxxxx>:
> > > > 2017-08-17 16:07 GMT+08:00 Yang Zhang <yang.zhang.wz@xxxxxxxxx>:
> > > > > On 2017/8/17 0:56, Radim KrÄmÃÅ wrote:
> > > > > > 2017-08-16 17:10+0300, 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.
> > > > > >
> > > > > >
> > > > > >
> > > > > > Paolo, did Microsoft point it out because they hit the bug when running
> > > > > > KVM on Hyper-V?
> > > > >
> > > > >
> > > > >
> > > > > Does this mean the nested emulation of EPT violation and
> > > > > misconfiguration in
> > > > > KVM side doesn't strictly follow the manual since we didn't hit the bug
> > > > > in
> > > > > KVM?
> > > >
> > > >
> > > > The VM-exit instruction length of vmcs12 is provided by vmcs02
> > > > (prepare_vmcs12()), so unless the length from vmcs02 is wrong. In
> > > > addition, something like mov instruction which can trigger the EPT
> > > > violation/misconfig in guest has already been decoded before executing
> > > > I think, IIUC, then exit qualification can have the information about
> > > > the instruction length.
> > >
> > >
> > > s/exit qualification/VM-exit instruction length
> >
> >
> > According to Paolo's comment "neither EPT violations nor misconfigurations
> > are listed in the manual among the VM exits that set the VM-exit instruction
> > length field", it seems to set the instruction length in vmcs12 is not right
> > though it is harmless.
>
> But Paolo also mentioned this "It just happens that the actual
> condition for VM-exit instruction length being set correctly is "the
> fault was taken after the accessing instruction has been decoded"."

We are talking the different thing. As manual mentioned, "All VM exits 
other than those listed in the above items leave this field undefined." 
If we set the field which is not in the listed VM exits that means our 
emulation is not correct. But i have checked the code, KVM just read the 
instruction length from hardware which means we didn't set it artificially.

> Regards,
> Wanpeng Li


--
Yang
Alibaba Cloud Computing