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

[Dmytro Maluka <dmy@semihalf.com>]

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

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