Re: [PATCH] hw/intc/ioapic: Update KVM routes before redelivering IRQ, on RTE update

[David Woodhouse]

On Mon, 2023-03-06 at 11:39 -0500, Peter Xu wrote:
> On Mon, Mar 06, 2023 at 09:25:35AM +0000, David Woodhouse wrote:
> > I think if we want to fix the lack of IR translation faults from the
> > IOMMU, we have to change this anyway.
> > 
> > At the very least, it can only use KVM to deliver the IRQ if there *is*
> > a valid translation. And if not, it needs to ask the IOMMU to
> > retranslate, with a 'delivering_now' flag indicating that the fault
> > should be raised because this isn't a preemptive translation in
> > advance.
> 
> I see that as a separate problem of what this patch wants to (rightfully)
> fix. 
> 

Agreed. Which is why I didn't just rip the kvm_set_irq() out and call
it fixed, and put that discussion below the ^--- cutoff.

>  I also agree we'll need that if e.g. we want to support ioapic in
> kvm.
>
> Sorry in advancie since I don't think I have the whole picture here.  Could
> you remind me the whole purpose of having such?  Is that for part of Xen's
> effort?
> 
> It was a long time ago, but IIRC such mechanism wasn't implemented when we
> worked on vIOMMU IR initially, because at least at that time the "keeping
> kvm irq routes always updated when IRQ entries modified" was good enough
> for qemu to work with IR.
> 
> The only outlier was in-kernel ioapic, but there was a talk from Google
> showing that in kernel ioapic brought mostly nothing good but more exposure
> on attack surface.  It does sound reasonable since fast devices shouldn't
> use ioapic at all to me, so IIRC the plan was split irqchip should be
> always optimal hence no immediate concern of not supporting IR with
> in-kernel ioapic.  But I definitely can miss important things along the
> way.

Indeed, I don't think we care about the in-kernel I/OAPIC. I don't
think we care about the kernel knowing about e.g. "GSI #11" at all. We
can just deliver it as MSI (for the I/OAPIC) or using KVM_INTERRUPT and
the interrupt window as we do for the PIC. Which is why I'd happily rip
that out and let it be delivered via the APIC intercept at 0xfeexxxxx.

The existing code which just keeps IRQ routes updated when they're
valid is kind of OK, and well-behaved guests can function. But it isn't
*right* in the case where they aren't valid.

What *ought* to happen is that the IOMMU should raise a fault at the
moment the interrupt occurs, if the translation isn't valid. And we
don't have that at all.

As for why I care? I don't really *need* it, as I have everything I
need for Xen PIRQ support already merged in 
https://gitlab.com/qemu-project/qemu/-/commit/6096cf7877

So while the thread at
https://lore.kernel.org/qemu-devel/aaef9961d210ac1873153bf3cf01d984708744fc.camel@infradead.org/
was partly driven by expecting to need this for Xen PIRQ support
(because in $DAYJOB I did those things in the other order and the PIRQ
support ended up just being a trivial different translator like the
IOMMU's IR)... I'd still quite like to fix it up in QEMU anyway, just
for correctness and fidelity in the faulting cases.

We can do more efficient invalidation too, rather than blowing away the
entire routing table every time. Just disconnect the IRQFD for the
specific interrupts that get invalidated, and let them get fixed up
again next time they occur.

smime.p7s
Description: S/MIME cryptographic signature