Re: [Qemu-devel] [PULL v5 26/57] x86-iommu: introduce IEC notifiers

[Peter Xu]

On Sat, Jul 30, 2016 at 09:52:48AM +0200, Jan Kiszka wrote:

[...]

> > +/**
> > + * x86_iommu_iec_notify_all - Notify IEC invalidations
> > + * @iommu: IOMMU device that sends the notification
> > + * @global: whether this is a global invalidation. If true, @index
> > + *          and @mask are undefined.
> > + * @index: starting index of interrupt entry to invalidate
> > + * @mask: index mask for the invalidation
> 
> This is Intel'ish: index and mask refer to the single Intel IR table.
> AMD has per-device tables.

Actually I was trying to consider this before when designing about the
interface...

> 
> But even for Intel: Would the index make any sense to the callbacks? KVM
> uses (virtual and real) GSIs to address its routing entries, no?
> 
> I suspect we will have to redesign this once we want to make use of
> non-global invalidation.

IIUC here the problem is how we should manage the mapping between GSI
routes and IRTE index (or device IDs, but let's talk later about
device IDs, since we can map a device-id invalidation into several
standalone index invalidations)? Or say, who should maintain this? IEC
invalidation consumers (e.g., IRQFD logic, IOAPIC, ...), or IOMMU?

IMHO, I would prefer the consumers to maintain this, not IOMMU. So I
would prefer a raw notification interface (index, mask, device-id,
etc. rather than GSI route index), and the consumers are responsible
to translate this message for their own sake.

The reason is simple: what if we have some other components (besides
GSI routes) that will register to this notifier? Though I am not sure
whether there would be one in the future, but letting IOMMU knowing
about something like GSI route index is a little bit odd to me.

Take irqfd as an example, currently MSIRouteEntry is defined as:

    struct MSIRouteEntry {
        PCIDevice *dev;             /* Device pointer */
        int vector;                 /* MSI/MSIX vector index */
        int virq;                   /* Virtual IRQ index */
        QLIST_ENTRY(MSIRouteEntry) list;
    };

When we want to support explicit IEC invalidation, we may need an
extra field like:

        uint32_t index;             /* IRTE index */

So when MSI routes are invalidated, we can translated the raw index
information into virq by simply looking up the MSIRouteEntry list.

Regarding to AMD's device-id interface...

I see that AMD IOMMUs do not have a global IRTE index table, not sure
whether we can "define" one? E.g. IIUC AMD IOMMU IRTE will have 13
bits index for each device, so how about making a global index like:

  device-id (16 bits) + 000b (3 bits) + index_per_device (13 bits)

to form a 32 bit index. So when AMD IOMMUs got a invalidation request,
IOMMU can translate this per-device invalidation into several
invalidations for specific IRTE entries? Not sure whether this would
work.

Thanks,

-- peterx