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

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

> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Sent: Wednesday, January 22, 2020 12:47 AM
> 
> 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.
> 
> I can try KVM or bhyve on top of Xen, but I'm not sure whether anyone
> has actually tested this, so I could be triggering other bugs in the
> nested code.
> 
> > 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.
> 
> Right, if "Ack interrupt on exit" is off the interrupt will trigger a
> vmexit, but it won't be acked and the vmexit interrupt information
> should have bit 31 set to 0, which I think we don't set correctly.
> 
> The Intel SDM states:
> 
> "For other VM exits (including those due to external interrupts when
> the “acknowledge interrupt on exit” VM-exit control is 0), the field
> is marked invalid (by clearing bit 31) and the remainder of the field
> is undefined."
> 
> AFAICT sync_exception_state also needs to check if VM_EXIT_CONTROLS
> has VM_EXIT_ACK_INTR_ON_EXIT set, and only set VM_EXIT_INTR_INFO in
> that case, do you agree?
> 

nice catch.

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