Re: [PATCH v1.1 4/5] KVM: Kick resamplefd for split kernel irqchip

[Peter Xu <peterx@redhat.com>]

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