Re: [Qemu-devel] [PATCH v10 16/26] intel_iommu: add support for split irqchip

[Peter Xu <peterx@redhat.com>]

On Sat, Jun 25, 2016 at 10:08:10AM +0200, Jan Kiszka wrote:

[...]

> For successful remappings, this is fine - it just caches the result in
> an interrupt route. But what will happen with invalid interrupts?
> 
> My current understanding is that, because the translation happens on
> activation of that interrupt source, not on actual signalling, the IOMMU
> will report an error too early and none when the interrupt is actually
> sent. That will lead to unwanted results, in the worst case
> false-positiv IR error reports to the guest, no?
> 
> I think we need to do this:
> - silently remap broken sources to an error sink
> - hook up the error sink with the actual IOMMU model (Intel or AMD)
> - when that source actually fires, let the sink report an IR
>   translation error to the guest
> 
> Am I right?

Right. I totally missed this one. :(

Currently when split irqchip is specified, IOAPIC interrupts are
cached in kernel with type KVM_IRQ_ROUTING_MSI (which is the same as
irqfds). When guest specify a fault interrupt entry, it is possible
that we silently fail the update, and all further interrupts are still
the old and correct one.

I agree with your solution on this. First of all we update the
interrupt even if it's faulty, but we should mark it out. After that,
we should fire QEMU from kernel side when the fault interrupt is
triggered, so that QEMU IOMMU can still generate corresponding fault
report interrupt to guest (though for Intel IOMMU IR, we still haven't
handled any fault report yet, but we should be prepared for it).

So it seems that finally we cannot avoid touching KVM this time.

I have a thought on how to implement the "sink" you have mentioned:

First of all, in KVM, we provide a new KVM_IRQ_ROUTING_* type, maybe
called:

  KVM_IRQ_ROUTING_EVENTFD

When KVM got this kind of interrupt, KVM does not trigger any real
interrupt to guest. Instead, it just do eventfd_signal() to a
pre-defined fd (maybe also with some data along with the notification,
so that we can put the error inside?), which is set during
KVM_SET_GSI_ROUTING ioctl().

After that, QEMU register all fault interrupts using this new
KVM_IRQ_ROUTING_EVENTFD type (rather than original
KVM_IRQ_ROUTING_MSI), assign a specific handler to handle the events
from these interrupts, and trigger IOMMU fault report path in that
handler.

(Here I used KVM_IRQ_ROUTING_EVENTFD rather than something like
 KVM_IRQ_ROUTING_FAULT_MSI to make the API a more general one, in case
 we can leverage it in other cases in the future)

Do you think the above workable?

No matter which solution we will have, I would still suggest we add
this as an "enhancement" after this series, since:

- there are works that depend on this series, so I would appreciate if
  this series can be merged first, so that other people can have a
  good basement (Radim's x2apic, David's AMD IOMMU). Though this is
  based on the assumption that the basic design of this series is
  workable...

- this problem will only exist for guest driver developers and should
  not happen for generic users (right?), so only a small subset of
  users might be affected.

Thanks,

-- peterx