Re: [Xen-devel] [PATCH 1/2] nvmx: fix handling of interrupts

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 03, 2020 at 07:24:04AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Sent: Thursday, January 23, 2020 8:32 PM
> > 
> > On Tue, Jan 21, 2020 at 03:34:13AM +0000, Tian, Kevin wrote:
> > > > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > Sent: Monday, January 20, 2020 6:19 PM
> > > >
> > > > On Sun, Jan 19, 2020 at 04:15:04AM +0000, Tian, Kevin wrote:
> > > > > > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > > > > > Sent: Wednesday, January 8, 2020 6:39 PM
> > > > > >
> > > > > > When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> > > > > > interrupts shouldn't be injected using the virtual interrupt 
> > > > > > delivery
> > > > > > mechanism, and instead should be signaled in the vmcs using the exit
> > > > > > reason and the interruption-information field if the "Acknowledge
> > > > > > interrupt on exit" vmexit control is set.
> > > > > >
> > > > > > Remove the nvmx_update_apicv helper: it's bogus to attempt to inject
> > > > > > interrupts on virtual vmexit using the virtual interrupt delivery
> > > > > > assistance, and it's also bogus to ack interrupts without checking 
> > > > > > if
> > > > > > the vmexit "Acknowledge interrupt on exit" vmexit control is set.
> > > > > > nvmx_intr_intercept already handles interrupts correctly on virtual
> > > > > > vmexit.
> > > > > >
> > > > > > Note that this fixes the usage of x2APIC by the L1 VMM, at least 
> > > > > > when
> > > > > > the L1 VMM is Xen.
> > > > >
> > > > > while this fix makes sense to me, can you also test other L1 VMMs,
> > > > > so we don't overlook some other intentions covered or hidden by
> > > > > removed logic?
> > > >
> > > > I could test other hypervisors, but do we really expect anything
> > > > that's not Xen on Xen to work?
> > > >
> > > > I'm asking because that's the only combination that's actually tested
> > > > by osstest.
> > > >
> > > > Thanks, Roger.
> > >
> > > If others are OK with your assumption, then it's fine. I didn't tightly
> > > follow the nested virtualization requirements in Xen.
> > >
> > > On the other hand, I think this patch needs a revision. It is not bogus
> > > to use virtual interrupt delivery on virtual VMexit, if "Ack interrupt
> > > on exit" is off. In such case, the delivery doesn't happen until L1
> > > hypervisor enables interrupt to clear interrupt window. Then it does
> > > save one exit. The only bogus point is that nvmx_udpate_apicv doesn't
> > > check "Ack interrupt on exit". So I prefer to add such check there
> > > instead of completely removing this optimization.
> > 
> > I went back over this, and I'm still not sure calling
> > nvmx_update_apicv is actually required: AFAICT vmx_intr_assist will
> > already inject the interrupt correctly using virtual interrupt
> > delivery if left pending on the vlapic. I guess the code in
> > nvmx_update_apicv doesn't hurt as long as it's gated on "Ack on exit"
> > not being enabled, but it's likely redundant.
> 
> It's not redundant. If you look at the code sequence, vmx_intr_assist
> is invoked before nvmx_switch_guest. At that time, the L1 vCPU is still
> in nested guest mode, thereby nvmx_intr_intercept takes effect which
> injects the pending vector into vmcs02 and bypasses the remaining
> virtual interrupt delivery logic for vmcs01. That is the main reason, imo,
> why nvmx_update_apicv is introduced.
> 
> iiuc, nvmx_intr_intercept and nvmx_update_apicv work together to
> complete nested interrupt injection:
> 
> (1) If "Ack interrupt on exit" is on, the pending vector is acked by 
> the former and delivered in vvmexit information field.
> (2) If "Ack interrupt on exit" is off and no virtual interrupt delivery, 
> no ack and interrupt window is enabled by the former.
> (3) Otherwise, the vector is acked by the latter and delivered through
> virtual interrupt delivery (where vmcs01 has been switched in). 
> 
> However, there are two issues in current code. One is about (3), i.e.,
> as you identified nvmx_update_apicv shouldn't blindly enable the
> optimization without checking the Ack setting. the other is new 
> about (2) - currently nvmx_intr_interrupt always enables interrupt 
> window when the Ack setting is off, which actually negates the 
> optimization of nvmx_update_apicv. Both should be fixed.

OK, I think I got it. It's likely however that vmx_intr_assist is also
called with the vmcs already switched to vmcs01 (if there's a pending
softirq for example), I guess vmx_intr_assist also copes correctly
when called with the vmcs already switched?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel