> On 23 Aug 2019, at 12:23, Vitaly Kuznetsov <vkuzn...@redhat.com> wrote:
>
> Sean Christopherson <sean.j.christopher...@intel.com> writes:
>
>> When handling emulation failure, return the emulation result directly
>> instead of capturing it in a local variable. Future patches will move
>> additional cases into handle_emulation_failure(), clean up the cruft
>> before so there isn't an ugly mix of setting a local variable and
>> returning directly.
>>
>> Signed-off-by: Sean Christopherson <sean.j.christopher...@intel.com>
>> ---
>> arch/x86/kvm/x86.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index cd425f54096a..c6de5bc4fa5e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6207,24 +6207,22 @@ EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>>
>> static int handle_emulation_failure(struct kvm_vcpu *vcpu, int
>> emulation_type)
>> {
>> - int r = EMULATE_DONE;
>> -
>> ++vcpu->stat.insn_emulation_fail;
>> trace_kvm_emulate_insn_failed(vcpu);
>>
>> if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
>> return EMULATE_FAIL;
>>
>> + kvm_queue_exception(vcpu, UD_VECTOR);
>> +
>> if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
>> vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>> vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>> vcpu->run->internal.ndata = 0;
>> - r = EMULATE_USER_EXIT;
>> + return EMULATE_USER_EXIT;
>> }
>>
>> - kvm_queue_exception(vcpu, UD_VECTOR);
>> -
>> - return r;
>> + return EMULATE_DONE;
>> }
>>
>> static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t cr2,
>
> No functional change,
>
> Reviewed-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>
> Just for self-education, what sane userspace is supposed to do when it
> sees KVM_EXIT_INTERNAL_ERROR other than kill the guest? Why does it make
> sense to still prepare to inject '#UD’
>
> --
> Vitaly
The commit which introduced this behaviour seems to be
6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to
userspace")
I actually agree with Vitaly. It made more sense that the ABI would be that
on internal emulation failure, we just return to userspace and allow it to
handle
the scenario however it likes. If it wishes to queue #UD on vCPU and resume
guest in case CPL==3 then it made sense that this logic would only be in
userspace.
Thus, there is no need for KVM to queue a #UD from kernel on this scenario...
What’s even weirder is that this ABI was then further broken by 2 later commits:
First, fc3a9157d314 ("KVM: X86: Don't report L2 emulation failures to
user-space")
changed behaviour to avoid reporting emulation error in case vCPU in guest-mode.
Then, a2b9e6c1a35a ("KVM: x86: Don't report guest userspace emulation error to
userspace")
Changed behaviour similarly to avoid reporting emulation error in case vCPU
CPL!=0.
In both cases, only #UD is injected to guest without userspace being aware of
it.
Problem is that if we would change this ABI to not queue #UD on emulation error,
we will definitely break userspace VMMs that rely on it when they re-enter into
guest
in this scenario and expect #UD to be injected.
Therefore, the only way to change this behaviour is to introduce a new KVM_CAP
that needs to be explicitly enabled from userspace.
But because most likely most userspace VMMs just terminate guest in case
of emulation-failure, it’s probably not worth it and Sean’s commit is good
enough.
For the commit itself:
Reviewed-by: Liran Alon <liran.a...@oracle.com>
-Liran
