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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

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

> 
> I will send an updated patch anyway, since I would like to get this
> sorted out sooner rather than later and will follow your advise of
> leaving nvmx_update_apicv in place gated by a check of whether "Ack on
> exit" is not enabled.
> 
> Thanks, Roger.

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