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

[Jan Kiszka]

On 2016-06-25 15:18, Peter Xu wrote:
> 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

Not really, because all sources are either using eventfds, which you can
also terminate in user space (already done for vhost and vfio in certain
scenarios - IIRC) or originate there anyway (IOAPIC).

> 
> 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...

I understand, and it is probably safe...

> 
> - 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.

...provided there is only little risk that some guest programs some
half-backed or stale message that would be rejected prematurely. But
that risk is most likely low.

Jan

signature.asc
Description: OpenPGP digital signature