Hi, On 7/12/22 18:02, Liu, Rong L wrote: > Hi Sean and Dmytro, > >> -----Original Message----- >> From: Dmytro Maluka <dmy@xxxxxxxxxxxx> >> Sent: Thursday, July 7, 2022 10:39 AM >> To: Christopherson,, Sean <seanjc@xxxxxxxxxx> >> Cc: Auger Eric <eric.auger@xxxxxxxxxx>; Micah Morton >> <mortonm@xxxxxxxxxxxx>; Alex Williamson >> <alex.williamson@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; Paolo Bonzini >> <pbonzini@xxxxxxxxxx>; Liu, Rong L <rong.l.liu@xxxxxxxxx>; Tomasz >> Nowicki <tn@xxxxxxxxxxxx>; Grzegorz Jaszczyk <jaz@xxxxxxxxxxxx>; >> Dmitry Torokhov <dtor@xxxxxxxxxx> >> 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
