RE: Add vfio-platform support for ONESHOT irq forwarding?

["Liu, Rong L" <rong.l.liu@intel.com>]

Hi Eric,

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Thursday, August 4, 2022 7:45 AM
> To: Liu, Rong L <rong.l.liu@intel.com>; Dmytro Maluka
> <dmy@semihalf.com>; Christopherson,, Sean <seanjc@google.com>
> Cc: Micah Morton <mortonm@chromium.org>; Alex Williamson
> <alex.williamson@redhat.com>; kvm@vger.kernel.org; Paolo Bonzini
> <pbonzini@redhat.com>; Tomasz Nowicki <tn@semihalf.com>; Grzegorz
> Jaszczyk <jaz@semihalf.com>; Dmitry Torokhov <dtor@google.com>
> Subject: Re: Add vfio-platform support for ONESHOT irq forwarding?
> 
> Hi,
> On 7/12/22 18:02, Liu, Rong L wrote:
> > Hi Sean and Dmytro,
> >
> >> -----Original Message-----
> >> From: Dmytro Maluka <dmy@semihalf.com>
> >> Sent: Thursday, July 7, 2022 10:39 AM
> >> To: Christopherson,, Sean <seanjc@google.com>
> >> Cc: Auger Eric <eric.auger@redhat.com>; Micah Morton
> >> <mortonm@chromium.org>; Alex Williamson
> >> <alex.williamson@redhat.com>; kvm@vger.kernel.org; Paolo Bonzini
> >> <pbonzini@redhat.com>; Liu, Rong L <rong.l.liu@intel.com>; Tomasz
> >> Nowicki <tn@semihalf.com>; Grzegorz Jaszczyk <jaz@semihalf.com>;
> >> Dmitry Torokhov <dtor@google.com>
> >> Subject: Re: Add vfio-platform support for ONESHOT irq forwarding?
> >>
> >> On 7/7/22 17:00, Sean Christopherson wrote:
> >>> On Thu, Jul 07, 2022, Dmytro Maluka wrote:
> >>>> Hi Sean,
> >>>>
> >>>> On 7/6/22 10:39 PM, Sean Christopherson wrote:
> >>>>> On Wed, Jul 06, 2022, Dmytro Maluka wrote:
> >>>>>> This is not a problem on native, since for oneshot irq we keep the
> >>>>>> interrupt masked until the thread exits, so that the EOI at the end
> >>>>>> of hardirq doesn't result in immediate re-assert. In vfio + KVM
> >>>>>> case, however, the host doesn't check that the interrupt is still
> >>>>>> masked in the guest, so
> >>>>>> vfio_platform_unmask() is called regardless.
> >>>>> Isn't not checking that an interrupt is unmasked the real bug?
> >>>>> Fudging around vfio (or whatever is doing the premature
> unmasking)
> >>>>> bugs by delaying an ack notification in KVM is a hack, no?
> >>>> Yes, not checking that an interrupt is unmasked is IMO a bug, and
> my
> >>>> patch actually adds this missing checking, only that it adds it in
> >>>> KVM, not in VFIO. :)
> >>>>
> >>>> Arguably it's not a bug that VFIO is not checking the guest interrupt
> >>>> state on its own, provided that the resample notification it receives
> >>>> is always a notification that the interrupt has been actually acked.
> >>>> That is the motivation behind postponing ack notification in KVM in
> >>>> my patch: it is to ensure that KVM "ack notifications" are always
> >>>> actual ack notifications (as the name suggests), not just "eoi
> >> notifications".
> >>> But EOI is an ACK.  It's software saying "this interrupt has been
> >> consumed".
> >>
> >> Ok, I see we've had some mutual misunderstanding of the term "ack"
> >> here.
> >> EOI is an ACK in the interrupt controller sense, while I was talking
> about
> >> an ACK in the device sense, i.e. a device-specific action, done in a
> device
> >> driver's IRQ handler, which makes the device de-assert the IRQ line (so
> >> that the IRQ will not get re-asserted after an EOI or unmask).
> >>
> >> So the problem I'm trying to fix stems from the peculiarity of
> "oneshot"
> >> interrupts: an ACK in the device sense is done in a threaded handler,
> i.e.
> >> after an ACK in the interrupt controller sense, not before it.
> >>
> >> In the meantime I've realized one more reason why my patch is wrong.
> >> kvm_notify_acked_irq() is an internal KVM thing which is supposed to
> >> notify interested parts of KVM about an ACK rather in the interrupt
> >> controller sense, i.e. about an EOI as it is doing already.
> >>
> >> VFIO, on the other hand, rather expects a notification about an ACK in
> the
> >> device sense. So it still seems a good idea to me to postpone sending
> >> notifications to VFIO until an IRQ gets unmasked, but this postponing
> >> should be done not for the entire kvm_notify_acked_irq() but only for
> >> eventfd_signal() on resamplefd in irqfd_resampler_ack().
> >>
> >> Thanks for making me think about that.
> >>
> >>>> That said, your idea of checking the guest interrupt status in VFIO
> >>>> (or whatever is listening on the resample eventfd) makes sense to
> me
> >>>> too. The problem, though, is that it's KVM that knows the guest
> >>>> interrupt status, so KVM would need to let VFIO/whatever know it
> >>>> somehow. (I'm assuming we are focusing on the case of KVM kernel
> >>>> irqchip, not userspace or split irqchip.) So do you have in mind
> >>>> adding something like "maskfd" and "unmaskfd" to KVM IRQFD
> >> interface,
> >>>> in addition to resamplefd? If so, I'm actually in favor of such an
> >>>> idea, as I think it would be also useful for other purposes, regardless
> >> of oneshot interrupts.
> >>> Unless I'm misreading things, KVM already provides a mask notifier,
> >>> irqfd just needs to be wired up to use
> >> kvm_(un)register_irq_mask_notifier().
> >>
> > Interesting...  I initially thought that kvm doesn't "trap" on ioapic's
> mmio
> > write.  However, I just traced kvm/ioapic.c and it turns out
> > ioapic_write_indirect() was called many times.   Does trapping on
> ioapic's mmio
> > write cause vmexit on edge-triggered interrupt exit?  It seems the case
> because
> > IOREGSEL and IOWIN of IOAPIC are memory mapped but not the
> redirection entry
> > register for each IRQ (that is why the name indirect_write), in order to
> unmask
> > redirection entry register on the exit of each interrupt (edge-triggered
> or
> > level-triggered), kernel needs to write to IORESEL, which means vmexit
> if kvm
> > traps on ioapic's mmio write.  However, for pass-thru device which
> uses
> > edge-triggered interrupt (handled by vfio or something similar),
> interrupt
> > (pIRQ) is enabled by vfio and it seems unnecessary to cause a vmexit
> when guest
> > updates virtual ioapic.  I think the situation is similar for level-triggered
> > interrupt.  So 2 vmexits for each level-triggered interrupt completion,
> one for
> > EOI on lapic and another for unmask of IOAPIC register.  Does this
> sound right?
> > I thought with vfio (or similar architecture), there is no vmexit
> necessary on
> > edge-triggered interrupt completion and only one vmexit for level
> triggered
> > interrupt completion, except the caveats of oneshot interrupt.  Maybe I
> > misunderstand something?
> Currently, no vmexit for edge-sensitive and 1 vmexit for level-sensitive
> is what happens on ARM shared peripheral interrupts at least.
> Note there is one setup that could remove the need for the vmexit on
> vEOI: irq_bypass mode
> (https://www.linux-kvm.org/images/a/a8/01x04-ARMdevice.pdf slide
> 12-14):
> on GIC you have a mode that allows automatic completion of the
> physical
> IRQ when the corresponding vIRQ is completed. This mode would not be
> compatible with oneshort_irq.
> At some point we worked on this enablement but given the lack of
> vfio-platform customers this work was paused so we still have the
> mask/unmask vfio dance.
> 
> Thanks
> 
> Eric
> 

Thanks for the info.  I am not familiar with ARM but it is interesting to know
the difference between 2 architectures.

> >
> >> Thanks for the tip. I'll take a look into implementing this idea.
> >>
> >> It seems you agree that fixing this issue via a change in KVM (in irqfd,
> not
> >> in ioapic) seems to be the way to go.
> >>
> >> An immediate problem I see with
> kvm_(un)register_irq_mask_notifier()
> >> is that it is currently available for x86 only. Also, mask notifiers are
> called
> >> under ioapic->lock (I'm not sure yet if that is good or bad news for us).
> >>
> >> Thanks,
> >> Dmytro