From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Oliver Upton <oupton@google.com>,
kvm@vger.kernel.org, Jim Mattson <jmattson@google.com>,
Peter Shier <pshier@google.com>
Subject: Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts
Date: Wed, 6 May 2020 09:48:56 -0700 [thread overview]
Message-ID: <20200506164856.GE3329@linux.intel.com> (raw)
In-Reply-To: <1f91d445-c3f3-fe35-3d65-0b7e0a6ff699@redhat.com>
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.
next prev parent reply other threads:[~2020-05-06 16:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-05 23:22 [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts Oliver Upton
2020-05-06 12:07 ` Paolo Bonzini
2020-05-06 15:25 ` Sean Christopherson
2020-05-06 16:00 ` Paolo Bonzini
2020-05-06 16:48 ` Sean Christopherson [this message]
2020-05-06 17:19 ` Oliver Upton
2020-05-06 17:43 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200506164856.GE3329@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=jmattson@google.com \
--cc=kvm@vger.kernel.org \
--cc=oupton@google.com \
--cc=pbonzini@redhat.com \
--cc=pshier@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.