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
