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.
