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

[Wanpeng Li]

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

Regards,
Wanpeng Li