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

[Eric Auger <eric.auger@redhat.com>]

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