[PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Oliver Upton]

vmx_guest_apic_has_interrupt implicitly depends on the virtual APIC
page being present + mapped into the kernel address space. Normally,
upon VMLAUNCH/VMRESUME, we get the vmcs12 pages directly. However, if a
live migration were to occur before reaching vcpu_block, the virtual
APIC will not be restored on the target host.

Fix this by getting vmcs12 pages before inspecting the virtual APIC
page.

Cc: kvm@vger.kernel.org
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Oliver Upton <oupton@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---

 Parent commit: 7c67f54661fc ("KVM: SVM: do not allow VMRUN inside SMM")

 arch/x86/kvm/x86.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8c0b77ac8dc6..edd3b75ad578 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8494,6 +8494,16 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu)
 {
+	/*
+	 * We must first get the vmcs12 pages before checking for interrupts
+	 * (done in kvm_arch_vcpu_runnable) in case L1 is using
+	 * virtual-interrupt delivery.
+	 */
+	if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) {
+		if (unlikely(!kvm_x86_ops.nested_ops->get_vmcs12_pages(vcpu)))
+			return 0;
+	}
+
 	if (!kvm_arch_vcpu_runnable(vcpu) &&
 	    (!kvm_x86_ops.pre_block || kvm_x86_ops.pre_block(vcpu) == 0)) {
 		srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
-- 
2.26.2.526.g744177e7f7-goog

Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Paolo Bonzini]

On 06/05/20 01:22, Oliver Upton wrote:
> vmx_guest_apic_has_interrupt implicitly depends on the virtual APIC
> page being present + mapped into the kernel address space. Normally,
> upon VMLAUNCH/VMRESUME, we get the vmcs12 pages directly. However, if a
> live migration were to occur before reaching vcpu_block, the virtual
> APIC will not be restored on the target host.
> 
> Fix this by getting vmcs12 pages before inspecting the virtual APIC
> page.

Do you have a selftests testcase?

> 
> +	/*
> +	 * We must first get the vmcs12 pages before checking for interrupts
> +	 * (done in kvm_arch_vcpu_runnable) in case L1 is using
> +	 * virtual-interrupt delivery.
> +	 */
> +	if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) {
> +		if (unlikely(!kvm_x86_ops.nested_ops->get_vmcs12_pages(vcpu)))
> +			return 0;
> +	}
> +


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).

Thanks,

Paolo

Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Sean Christopherson]

On Wed, May 06, 2020 at 02:07:17PM +0200, Paolo Bonzini wrote:
> On 06/05/20 01:22, Oliver Upton wrote:
> > +	/*
> > +	 * We must first get the vmcs12 pages before checking for interrupts
> > +	 * (done in kvm_arch_vcpu_runnable) in case L1 is using
> > +	 * virtual-interrupt delivery.
> > +	 */
> > +	if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) {
> > +		if (unlikely(!kvm_x86_ops.nested_ops->get_vmcs12_pages(vcpu)))
> > +			return 0;
> > +	}
> > +
> 
> 
> 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().

I was very tempted to dive into that mess when working on the nested events
stuff, but was afraid that I would be opening up pandora's box.

Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Paolo Bonzini]

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.

Paolo

> I was very tempted to dive into that mess when working on the nested events
> stuff, but was afraid that I would be opening up pandora's box.

Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Sean Christopherson]

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.

Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Oliver Upton]

On Wed, May 6, 2020 at 9:49 AM Sean Christopherson
<sean.j.christopherson@intel.com> 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

Re: [PATCH] kvm: x86: get vmcs12 pages before checking pending interrupts

[Paolo Bonzini]

On 06/05/20 18:48, Sean Christopherson wrote:
>>
>> 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.

That wouldn't be a problem, because you'd get out of HLT if needed via
stimer_expiration (and then stimer_notify_direct->kvm_apic_set_irq or
stimer_send_msg->synic_deliver_msg->synic_set_irq->kvm_irq_delivery_to_apic)
which is called from kvm_hv_process_stimers.

Paolo