On Thu, Feb 27, 2020 at 10:14:47PM +0100, Auger Eric wrote: > Hi Peter, Hi, Eric,
[...] > >>>>> + * the KVM resample fd kick is skipped. The userspace > >>>>> + * needs to remember the resamplefd and kick it when we > >>>>> + * receive EOI of this IRQ. > >>>> Practically we now talk about a VFIO ACTION_UNMASK classical eventfd > >>>> As such isn't it a bit weird to handle those normal UNMASK eventfds in > >>>> the KVM code? > >>> > >>> I'm not sure I completely get the question, but this should be > >>> something general to KVM resamplefd support. In other words, this > >>> should also fix other devices (besides VFIO) when they're using the > >>> KVM resamplefd, because IMHO it's the resamplefd and split irqchip > >>> which is really broken here. > >> Here is my understanding (& memories): the KVM resamplefd is an eventfd > >> you register to KVM so that KVM triggers the resamplefd when KVM traps > >> the EOI. Here I understand this is the userspace IOAPIC that traps the > >> EOI and not the in-kernel virtual interrupt controller. So I would have > >> expected you just need to signal the VFIO UNMASK eventfd to re-enable > >> the physical IRQ (which was automasked). This is no more a KVM > >> resamplefd strictly speaking as KVM is not involved anymore in the > >> deactivation process. > > > > Yes KVM kernel side should not be involed when we're using split > > irqchip in this case. However it should still belongs to the work of > > the userspace KVM module (kvm-all.c) so that it can still "mimic" the > > resamplefd feature that KVM_IRQFD provides. > OK. So that what my actual question. Should this be handled by kvm-all.c? It should fix KVM split irqchip with resamplefd, so I think it's natural to do this in kvm-all.c (I'm a bit puzzled on where else we can put this... :). Or did I misunderstood your question? > > > >>> > >>> With that in mind, I think KVM should not need to even know what's > >>> behind the resamplefd (in VFIO's case, it's the UNMASK eventfd). It > >>> just needs to kick it when IOAPIC EOI comes for the specific IRQ > >> But above the userspace directly calls > >> event_notifier_set(rfd->resample_event); > >> > >> This is not KVM anymore that "kicks it". Or maybe I miss something. So > >> my comment was, why is it handled in the QEMU KVM layer? > > > > It's my fault to be unclear on using "KVM" above. I should really say > > it as kvm-all.c, say, the QEMU layer for the kernel KVM module. > > > > Indeed this problem is complicated... let me try to summarize. > > > > Firstly KVM split irqchip and resamplefd is not really going to work > > in the kernel (I think we just overlooked that when introducing the > > 2nd feature, no matter which one comes first), because the resample > > operation should be part of IOAPIC EOI, nevertheless when using split > > irqchip IOAPIC is in userspace. > > > > After we noticed this, Alex somewhere proposed to disable that in KVM, > > which is actually the 1st kernel patch (654f1f13ea56). > > > > We should (at the same time) propose patch 1 too in this series but I > > guess everybody just forgot this afterwards (Paolo actually proposed > > mostly the whole solution but I guess it got forgotten too)... > > > > About the fast path speedup: the main logic should be to mimic the > > same resamplefd feature as provided by KVM_IRQFD but this time only in > > the userspace. However now we're implementing the same logic only > > within userspace kvm-all.c, and the kernel KVM should be totally not > > aware of this. Doing that benefits us in that the KVM interface in > > QEMU does not need to be changed (majorly kvm_irqchip_assign_irqfd()). > > What we need to do is just to wire up the userspace IOAPIC with these > > resamplefds. And the idea is actually the same too - someone (VFIO) > > wants to have one fd (which is the resamplefd) kicked when EOI comes > > when requesting for a KVM irqfd, no matter who's going to kick it > > (kernel KVM or userspace). That's all. > > Yep I think it makes sense to accelerate the trigger path. And for the > EOI path if you have means to trap this on the userspace irqchip it > looks better than doing the map/unmap dance. So it looks a good iead to > me. Now shall it be in kvm-all.c or elsewhere, to me it is not the most > important, as long as we reach a consensus and the scheme gets > documented somewhere. Sure. For documentation: as mentioned above, I think the irqfd users will always use the interface just like before, and the resamplefd should work exactly like what KVM_IRQFD and kvm_irqchip_assign_irqfd() was offering before this patch too. IMO it'll just start to work even for split irqchips which was silently broken without being noticed. Thanks, -- Peter Xu
