On Wed, May 6, 2020 at 9:49 AM Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote: > > On Wed, May 06, 2020 at 06:00:03PM +0200, Paolo Bonzini wrote: > > On 06/05/20 17:25, Sean Christopherson wrote: > > >> > > >> The patch is a bit ad hoc, I'd rather move the whole "if > > >> (kvm_request_pending(vcpu))" from vcpu_enter_guest to vcpu_run (via a > > >> new function). > > > It might make sense to go with an ad hoc patch to get the thing fixed, then > > > worry about cleaning up the pending request crud. It'd be nice to get rid > > > of the extra nested_ops->check_events() call in kvm_vcpu_running(), as well > > > as all of the various request checks in (or triggered by) vcpu_block(). > > > > Yes, I agree that there are unnecessary tests in kvm_vcpu_running() if > > requests are handled before vcpu_block and that would be a nice cleanup, > > but I'm asking about something less ambitious. > > > > Can you think of something that can go wrong if we just move all > > requests, except for KVM_REQ_EVENT, up from vcpu_enter_guest() to > > vcpu_run()? That might be more or less as ad hoc as Oliver's patch, but > > without the code duplication at least. > > I believe the kvm_hv_has_stimer_pending() check in kvm_vcpu_has_events() > will get messed up, e.g. handling KVM_REQ_HV_STIMER will clear the pending > bit. No idea if that can interact with HLT though. > > Everything else looks ok, but I didn't exactly do a thorough audit. > > My big concern is that we'd break something and never notice because the > failure mode would be a delayed interrupt or poor performance in various > corner cases. Don't get me wrong, I'll all for hoisting request handling > out of vcpu_enter_guest(), but if we're goint to risk breaking things I'd > prefer to commit to a complete cleanup. My main motivation for adding the duplicate code was to avoid introducing new failures. I agree that a larger cleanup is in order, but didn't want to unintentionally break things at the moment :) -- Thanks, Oliver
