| Message ID | 20190823010709.24879-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Remove emulation_result enums | expand |
Sean Christopherson <sean.j.christopherson@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.christopherson@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 <vkuznets@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'?
> On 23 Aug 2019, at 12:23, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Sean Christopherson <sean.j.christopherson@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.christopherson@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 <vkuznets@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.alon@oracle.com> -Liran
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,
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.christopherson@intel.com> --- arch/x86/kvm/x86.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)