Hi Micah, On 1/26/21 4:31 PM, Micah Morton wrote: > On Tue, Jan 26, 2021 at 3:48 AM Auger Eric <eric.auger@xxxxxxxxxx> wrote: >> >> Hi Micah, >> >> On 1/25/21 7:32 PM, Micah Morton wrote: >>> On Mon, Jan 25, 2021 at 12:31 PM Auger Eric <eric.auger@xxxxxxxxxx> wrote: >>>> >>>> Hi Micah, >>>> >>>> On 1/25/21 4:46 PM, Micah Morton wrote: >>>>> Hi Eric, >>>>> >>>>> I was recently looking into some vfio-platform passthrough stuff and >>>>> came across a device I wanted to assign to a guest that uses a ONESHOT >>>>> type interrupt (these type of interrupts seem to be quite common, on >>>>> ARM at least). The semantics for ONESHOT interrupts are a bit >>>>> different from regular level triggered interrupts as I'll describe >>>>> here: >>>>> >>>>> The normal generic code flow for level-triggered interrupts is as follows: >>>>> >>>>> - regular type[1]: mask[2] the irq, then run the handler, then >>>>> unmask[3] the irq and done >>>> >>>> VFIO level sensitive interrupts are "automasked". See slide 10 of >>>> https://www.linux-kvm.org/images/a/a8/01x04-ARMdevice.pdf if this can help. >>> >>> When you say "automasked" / "mask and deactivate" are you referring to >>> the disable_irq_nosync() call in vfio_platform >>> (https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L154)? >> Yes that's what I meant >>> >>>> >>>> When the guest deactivates the virtual IRQ, this causes a maintenance >>>> interrupt on host. This occurence causes kvm_notify_acked_irq() to be >>>> called and this latter unmasks the physical IRQ. >>> >>> Are you saying kvm_notify_acked_irq() causes >>> vfio_platform_unmask_handler() >>> (https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L94) >>> to be called? Makes sense if so. >> Yes this ends up calling irqfd_resampler_ack which toggles the virtual >> irq line down and calls the unmask eventfd >>> >>>>> >>>>> - fasteoi type[4]: run the handler, then eoi[5] the irq and done >>>>> >>>>> Note: IIUC the fasteoi type doesn't do any irq masking/unmasking >>>>> because that is assumed to be handled transparently by "modern forms >>>>> of interrupt handlers, which handle the flow details in hardware" >>>>> >>>>> ONESHOT type interrupts are a special case of the fasteoi type >>>>> described above. They rely on the driver registering a threaded >>>>> handler for the interrupt and assume the irq line will remain masked >>>>> until the threaded handler completes, at which time the line will be >>>>> unmasked. TL;DR: >>>>> >>>>> - mask[6] the irq, run the handler, and potentially eoi[7] the irq, >>>>> then unmask[8] later when the threaded handler has finished running. >>>> >>>> could you point me to the exact linux handler (is it >>>> handle_fasteoi_irq?) or Why do you say "potentially". Given the details >>>> above, the guest EOI would unmask the IRQ at physical level. We do not >>>> have any hook KVM/VFIO on the guest unmap. >>> >>> Yep. handle_fasteoi_irq(). By "potentially" I just meant the eoi is >>> only called for ONESHOT irqs if this check passes: !(chip->flags & >>> IRQCHIP_EOI_THREADED). Not actually sure what this check is about >>> since I didn't look into it. >> I checked IRQCHIP_EOI_THREADED and it does not seem to be supported by >> GIC irqchip. so this would mean that the EOI is not called in our case? > > Sounds like it, thanks for looking into that. > >> >>> >>> So far I was just talking about irq operations that happen in the >>> host. Not sure I quite understand your last two sentences about guest >>> EOI and guest unmap. >> sorry I meant unmask instead of unmap :-( The closure of the physical >> IRQ is triggered by the deactivation of the guest virtual IRQ. >> Let's first assume you keep the existing "auto-masked" level sensitive >> IRQ on host side (vfio platform driver), on the guest driver side you >> are going to have a oneshot IRQ. What needs to be understood is when >> does the deactivation happen on guest side. > > That's a good question, I need to go back and look at that. > >>> >>>>> >>>>> For vfio-platform irq forwarding, there is no existing function in >>>>> drivers/vfio/platform/vfio_platform_irq.c[9] that is a good candidate >>>>> for registering as the threaded handler for a ONESHOT interrupt in the >>>>> case we want to request the ONESHOT irq with >>>>> request_threaded_irq()[10]. Moreover, we can't just register a >>>>> threaded function that simply immediately returns IRQ_HANDLED (as is >>>>> done in vfio_irq_handler()[11] and vfio_automasked_irq_handler()[12]), >>>>> since that would cause the IRQ to be unmasked[13] immediately, before >>>> sorry I know you reworked that several times for style issue but [13] >>>> does not match unmask(). >>> >>> That's actually the link I intended, irq_finalize_oneshot() will be >>> called after the threaded interrupt handler returns, and that's when >>> the IRQ will be unmasked. >> Hum this points to something within irq_thread_check_affinity(). Anyway. > > Sorry I was pointing the URL at a specific line in > irq_finalize_oneshot() but it was probably confusing. > https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/manage.c#L984 > is the function I'm talking about. > >>> >>>>> the userspace/guest driver has had any chance to service the >>>>> interrupt. >>>>> >>>>> The most obvious way I see to handle this is to add a threaded handler >>>>> to vfio_platform_irq.c that waits until the userspace/guest driver has >>>>> serviced the interrupt and the unmask_handler[14] has been called, at >>>>> which point it returns IRQ_HANDLED so the generic IRQ code in the host >>>>> can finally unmask the interrupt. >>>> this is done on guest EOI at the moment. >>> >>> By "this", I think you mean enable_irq() in vfio_platform_irq.c? >> right >>> >>>>> >>>>> Does this sound like a reasonable approach and something you would be >>>>> fine with adding to vfio-platform? >>>> Well I think it is interesting to do a pre-study and make sure we agree >>>> on the terminology, IRQ flow and problems that would need to be solved. >>>> Then we need to determine if it is worth the candle, if this support >>>> would speed up the vfio-platform usage (I am not sure at this point as I >>>> think there are more important drags like the lack of specification/dt >>>> integration for instance). >>>> >>>> Besides we need to make sure we are not launching into a huge effort for >>>> attempting to assign a device that does not fit well into the vfio >>>> platform scope (dma capable, reset, device tree). >>> >>> Yes, agreed. >>> >>>> >>>> If so I could get started looking >>>>> at the implementation for how to sleep in the threaded handler in >>>>> vfio-platform until the unmask_handler is called. The most tricky/ugly >>>>> part of this is that DT has no knowledge of irq ONESHOT-ness, as it >>>>> only contains info regarding active-low vs active-high and edge vs >>>>> level trigger. That means that vfio-platform can't figure out that a >>>>> device uses a ONESHOT irq in a similar way to how it queries[15] the >>>>> trigger type, and by extension QEMU can't learn this information >>>>> through the VFIO_DEVICE_GET_IRQ_INFO ioctl, but must have another way >>>>> of knowing (i.e. command line option to QEMU). >>>> Indeed that's not really appealing. >>>>> >>>>> I guess potentially another option would be to treat ONESHOT >>>>> interrupts like regular level triggered interrupts from the >>>>> perspective of vfio-platform, but somehow ensure the interrupt stays >>>>> masked during injection to the guest, rather than just disabled. >>>> I need this to be clarified actually. I am confused by the automasked >>>> terminology now. >>> >>> My note below tries to explain a bit but AFAICT there's some confusion >>> in the VFIO code (both platform and PCI) mixing the IRQ terms >>> mask/unmask with disable/enable, which I think are not quite >>> interchangeable concepts. IIUC irq enable/disable is a high(er) level >>> concept that means that even if the irqchip HW sees interrupts coming >>> in, the kernel refrains from calling the interrupt handler for the IRQ >>> -- but the kernel keeps track of interrupts that come in while an IRQ >>> is disabled, in order to know (once the IRQ gets enabled again by the >>> driver) that there are pending interrupts from the HW. On the other >>> hand, IRQ masking is a lower level irqchip operation that truly masks >>> the IRQs at the hardware level, so the kernel doesn't even know or >>> care if the line is being activated/deactivated while the IRQ is >>> masked. >> >> Looking again at the genirq code, irq/chip.c __irq_disable, actual >> implementation at HW level depends on the irqchip caps. GIC does not >> implement the irq_enable() cb so this ends up calling mask_irq which >> eventually calls gic_mask_irq and toggles the enable state for the given >> IRQ. > > Are you talking about the lazy disable approach > (https://elixir.bootlin.com/linux/v5.10.10/source/kernel/irq/chip.c#L371)? > Sounds like you're implying IRQ_DISABLE_UNLAZY will be true for the > irq when we're doing VFIO forwarding? Or else we would see an extra > pending interrupt on the line during IRQ forwarding. I tried to remember the path from __irq_disable SW flow downto the GIC SPI programming. I was not implying the IRQ_DISABLE_UNLAZY flag. As fas as I remember lazy disable approach was in use when I did the integration. But that's just an optimization that avoids to access the HW unnecessarily (ie. the handler may be invoked even if the irq is disabled). Thanks Eric > > Maybe setting the IRQ_DISABLE_UNLAZY flag is the solution for ONESHOT > irq forwarding actually, so we guarantee the IRQ is masked immediately > and avoid the lazy approach that could lead to an erroneous pending > interrupt? > >> >> Thanks >> >> Eric >>> >>> I don't have a great explanation for why ONESHOT interrupts are even a >>> thing in the first place, maybe the HW design means you're going to >>> get a bunch of garbage/meaningless ups and downs on the interrupt line >>> while the interrupt is being serviced and they should never be taken >>> into consideration in any way by the kernel until the IRQ is unmasked >>> again? >>> >>>> >>>> Thanks >>>> >>>> Eric >>>> >>>> I'm >>>>> not sure whether this could cause legitimate interrupts coming from >>>>> devices to be missed while the injection for an existing interrupt is >>>>> underway, but maybe this is a rare enough scenario that we wouldn't >>>>> care. The main issue with this approach is that handle_level_irq()[16] >>>>> will try to unmask the irq out from under us after we start the >>>>> injection (as it is already masked before >>>>> vfio_automasked_irq_handler[17] runs anyway). Not sure if masking at >>>>> the irqchip level supports nesting or not. >>>>> >>>>> Let me know if you think either of these are viable options for adding >>>>> ONESHOT interrupt forwarding support to vfio-platform? >>>>> >>>>> Thanks, >>>>> Micah >>>>> >>>>> >>>>> >>>>> >>>>> Additional note about level triggered vs ONESHOT irq forwarding: >>>>> For the regular type of level triggered interrupt described above, the >>>>> vfio handler will call disable_irq_nosync()[18] before the >>>>> handle_level_irq() function unmasks the irq and returns. This ensures >>>>> if new interrupts come in on the line while the existing one is being >>>>> handled by the guest (and the irq is therefore disabled), that the >>>>> vfio_automasked_irq_handler() isn’t triggered again until the >>>>> vfio_platform_unmask_handler() function has been triggered by the >>>>> guest (causing the irq to be re-enabled[19]). In other words, the >>>>> purpose of the irq enable/disable that already exists in vfio-platform >>>>> is a higher level concept that delays handling of additional >>>>> level-triggered interrupts in the host until the current one has been >>>>> handled in the guest. >>>>> >>>>> This means that the existing level triggered interrupt forwarding >>>>> logic in vfio/vfio-platform is not sufficient for handling ONESHOT >>>>> interrupts (i.e. we can’t just treat a ONESHOT interrupt like a >>>>> regular level triggered interrupt in the host and use the existing >>>>> vfio forwarding code). The masking that needs to happen for ONESHOT >>>>> interrupts is at the lower level of the irqchip mask/unmask in that >>>>> the ONESHOT irq needs to remain masked (not just disabled) until the >>>>> driver’s threaded handler has completed. >>>>> >>>>> >>>>> >>>>> >>>>> [1] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L642 >>>>> [2] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L414 >>>>> [3] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L619 >>>>> [4] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L702 >>>>> [5] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L688 >>>>> [6] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L724 >>>>> [7] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L688 >>>>> [8] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/manage.c#L1028 >>>>> [9] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c >>>>> [10] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/manage.c#L2038 >>>>> [11] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L167 >>>>> [12] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L142 >>>>> [13] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/manage.c#L1028 >>>>> [14] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L94 >>>>> [15] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L310 >>>>> [16] https://elixir.bootlin.com/linux/v5.10.7/source/kernel/irq/chip.c#L642 >>>>> [17] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L142 >>>>> [18] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L154 >>>>> [19] https://elixir.bootlin.com/linux/v5.10.7/source/drivers/vfio/platform/vfio_platform_irq.c#L87 >>>>> >>>> >>> >> >
