Re: [Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang <knut.omang@oracle.com>]

On Sun, 2015-09-27 at 13:07 +0300, Michael S. Tsirkin wrote:
> On Sat, Sep 26, 2015 at 08:09:56AM +0200, Knut Omang wrote:
> > - Use a hash table indexed on bus pointers to store information
> > about buses
> >   instead of using the bus numbers.
> >   Bus pointers are stored in a new VTDBus struct together with the
> > vector
> >   of device address space pointers indexed by devfn.
> > - The bus number is still used for lookup for selective SID based
> > invalidate,
> >   in which case the bus number is lazily resolved from the bus hash
> > table and
> >   cached in a separate index.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> 
> Fails on 32 bit:
> /scm/qemu/hw/i386/intel_iommu.c: In function ‘vtd_find_add_as’:
> /scm/qemu/hw/i386/intel_iommu.c:1869:20: error: cast from pointer to
> integer of different size [-Werror=pointer-to-int-cast]
>      uint64_t key = (uint64_t)bus;
>                     ^
> /scm/qemu/hw/i386/intel_iommu.c:1877:15: error: cast from pointer to
> integer of different size [-Werror=pointer-to-int-cast]
>          key = (uint64_t)bus;
>                ^
> 
> You need to cast things to uintptr_t.

Sorry - everything around me has become 64 bit these days, will be more
careful - I'll post a v5 with this.

Knut

> > ---
> >  hw/i386/intel_iommu.c         | 90
> > +++++++++++++++++++++++++++++++++++--------
> >  hw/pci-host/q35.c             | 25 ++----------
> >  include/hw/i386/intel_iommu.h | 16 +++++++-
> >  3 files changed, 91 insertions(+), 40 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 08055a8..d677a28 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -22,6 +22,7 @@
> >  #include "hw/sysbus.h"
> >  #include "exec/address-spaces.h"
> >  #include "intel_iommu_internal.h"
> > +#include "hw/pci/pci.h"
> >  
> >  /*#define DEBUG_INTEL_IOMMU*/
> >  #ifdef DEBUG_INTEL_IOMMU
> > @@ -166,19 +167,17 @@ static gboolean
> > vtd_hash_remove_by_page(gpointer key, gpointer value,
> >   */
> >  static void vtd_reset_context_cache(IntelIOMMUState *s)
> >  {
> > -    VTDAddressSpace **pvtd_as;
> >      VTDAddressSpace *vtd_as;
> > -    uint32_t bus_it;
> > +    VTDBus *vtd_bus;
> > +    GHashTableIter bus_it;
> >      uint32_t devfn_it;
> >  
> > +    g_hash_table_iter_init(&bus_it, s->vtd_as_by_busptr);
> > +
> >      VTD_DPRINTF(CACHE, "global context_cache_gen=1");
> > -    for (bus_it = 0; bus_it < VTD_PCI_BUS_MAX; ++bus_it) {
> > -        pvtd_as = s->address_spaces[bus_it];
> > -        if (!pvtd_as) {
> > -            continue;
> > -        }
> > +    while (g_hash_table_iter_next (&bus_it, NULL,
> > (void**)&vtd_bus)) {
> >          for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX;
> > ++devfn_it) {
> > -            vtd_as = pvtd_as[devfn_it];
> > +            vtd_as = vtd_bus->dev_as[devfn_it];
> >              if (!vtd_as) {
> >                  continue;
> >              }
> > @@ -754,12 +753,13 @@ static inline bool
> > vtd_is_interrupt_addr(hwaddr addr)
> >   * @is_write: The access is a write operation
> >   * @entry: IOMMUTLBEntry that contain the addr to be translated
> > and result
> >   */
> > -static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as,
> > uint8_t bus_num,
> > +static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus
> > *bus,
> >                                     uint8_t devfn, hwaddr addr,
> > bool is_write,
> >                                     IOMMUTLBEntry *entry)
> >  {
> >      IntelIOMMUState *s = vtd_as->iommu_state;
> >      VTDContextEntry ce;
> > +    uint8_t bus_num = pci_bus_num(bus);
> >      VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
> >      uint64_t slpte;
> >      uint32_t level;
> > @@ -874,6 +874,30 @@ static void
> > vtd_context_global_invalidate(IntelIOMMUState *s)
> >      }
> >  }
> >  
> > +
> > +/* Find the VTD address space currently associated with a given
> > bus number,
> > + */
> > +static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s,
> > uint8_t bus_num)
> > +{
> > +    VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> > +    if (!vtd_bus) {
> > +        /* Iterate over the registered buses to find the one
> > +         * which currently hold this bus number, and update the
> > bus_num lookup table:
> > +         */
> > +        GHashTableIter iter;
> > +        uint64_t key;
> > +
> > +        g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> > +        while (g_hash_table_iter_next (&iter, (void**)&key,
> > (void**)&vtd_bus)) {
> > +            if (pci_bus_num(vtd_bus->bus) == bus_num) {
> > +                s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> > +                return vtd_bus;
> > +            }
> > +        }
> > +    }
> > +    return vtd_bus;
> > +}
> > +
> >  /* Do a context-cache device-selective invalidation.
> >   * @func_mask: FM field after shifting
> >   */
> > @@ -882,7 +906,7 @@ static void
> > vtd_context_device_invalidate(IntelIOMMUState *s,
> >                                            uint16_t func_mask)
> >  {
> >      uint16_t mask;
> > -    VTDAddressSpace **pvtd_as;
> > +    VTDBus *vtd_bus;
> >      VTDAddressSpace *vtd_as;
> >      uint16_t devfn;
> >      uint16_t devfn_it;
> > @@ -903,11 +927,11 @@ static void
> > vtd_context_device_invalidate(IntelIOMMUState *s,
> >      }
> >      VTD_DPRINTF(INV, "device-selective invalidation source
> > 0x%"PRIx16
> >                      " mask %"PRIu16, source_id, mask);
> > -    pvtd_as = s->address_spaces[VTD_SID_TO_BUS(source_id)];
> > -    if (pvtd_as) {
> > +    vtd_bus = vtd_find_as_from_bus_num(s,
> > VTD_SID_TO_BUS(source_id));
> > +    if (vtd_bus) {
> >          devfn = VTD_SID_TO_DEVFN(source_id);
> >          for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX;
> > ++devfn_it) {
> > -            vtd_as = pvtd_as[devfn_it];
> > +            vtd_as = vtd_bus->dev_as[devfn_it];
> >              if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
> >                  VTD_DPRINTF(INV, "invalidate context-cahce of
> > devfn 0x%"PRIx16,
> >                              devfn_it);
> > @@ -1805,11 +1829,11 @@ static IOMMUTLBEntry
> > vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
> >          return ret;
> >      }
> >  
> > -    vtd_do_iommu_translate(vtd_as, vtd_as->bus_num, vtd_as->devfn,
> > addr,
> > +    vtd_do_iommu_translate(vtd_as, vtd_as->bus, vtd_as->devfn,
> > addr,
> >                             is_write, &ret);
> >      VTD_DPRINTF(MMU,
> >                  "bus %"PRIu8 " slot %"PRIu8 " func %"PRIu8 " devfn
> > %"PRIu8
> > -                " gpa 0x%"PRIx64 " hpa 0x%"PRIx64, vtd_as
> > ->bus_num,
> > +                " gpa 0x%"PRIx64 " hpa 0x%"PRIx64,
> > pci_bus_num(vtd_as->bus),
> >                  VTD_PCI_SLOT(vtd_as->devfn), VTD_PCI_FUNC(vtd_as
> > ->devfn),
> >                  vtd_as->devfn, addr, ret.translated_addr);
> >      return ret;
> > @@ -1839,6 +1863,38 @@ static Property vtd_properties[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > +
> > +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> > int devfn)
> > +{
> > +    uint64_t key = (uint64_t)bus;
> > +    VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr,
> > &key);
> > +    VTDAddressSpace *vtd_dev_as;
> > +
> > +    if (!vtd_bus) {
> > +        /* No corresponding free() */
> > +        vtd_bus = g_malloc0(sizeof(VTDBus) +
> > sizeof(VTDAddressSpace *) * VTD_PCI_DEVFN_MAX);
> > +        vtd_bus->bus = bus;
> > +        key = (uint64_t)bus;
> > +        g_hash_table_insert(s->vtd_as_by_busptr, &key, vtd_bus);
> > +    }
> > +
> > +    vtd_dev_as = vtd_bus->dev_as[devfn];
> > +
> > +    if (!vtd_dev_as) {
> > +        vtd_bus->dev_as[devfn] = vtd_dev_as =
> > g_malloc0(sizeof(VTDAddressSpace));
> > +
> > +        vtd_dev_as->bus = bus;
> > +        vtd_dev_as->devfn = (uint8_t)devfn;
> > +        vtd_dev_as->iommu_state = s;
> > +        vtd_dev_as->context_cache_entry.context_cache_gen = 0;
> > +        memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
> > +                                 &s->iommu_ops, "intel_iommu",
> > UINT64_MAX);
> > +        address_space_init(&vtd_dev_as->as,
> > +                           &vtd_dev_as->iommu, "intel_iommu");
> > +    }
> > +    return vtd_dev_as;
> > +}
> > +
> >  /* Do the initialization. It will also be called when reset, so
> > pay
> >   * attention when adding new initialization stuff.
> >   */
> > @@ -1931,13 +1987,15 @@ static void vtd_realize(DeviceState *dev,
> > Error **errp)
> >      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
> >  
> >      VTD_DPRINTF(GENERAL, "");
> > -    memset(s->address_spaces, 0, sizeof(s->address_spaces));
> > +    memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
> >      memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
> >                            "intel_iommu", DMAR_REG_SIZE);
> >      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->csrmem);
> >      /* No corresponding destroy */
> >      s->iotlb = g_hash_table_new_full(vtd_uint64_hash,
> > vtd_uint64_equal,
> >                                       g_free, g_free);
> > +    s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash,
> > vtd_uint64_equal,
> > +                                              g_free, g_free);
> >      vtd_init(s);
> >  }
> >  
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index bd74094..c81507d 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -426,31 +426,12 @@ static void mch_reset(DeviceState *qdev)
> >  static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque,
> > int devfn)
> >  {
> >      IntelIOMMUState *s = opaque;
> > -    VTDAddressSpace **pvtd_as;
> > -    int bus_num = pci_bus_num(bus);
> > +    VTDAddressSpace *vtd_as;
> >  
> > -    assert(0 <= bus_num && bus_num <= VTD_PCI_BUS_MAX);
> >      assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
> >  
> > -    pvtd_as = s->address_spaces[bus_num];
> > -    if (!pvtd_as) {
> > -        /* No corresponding free() */
> > -        pvtd_as = g_malloc0(sizeof(VTDAddressSpace *) *
> > VTD_PCI_DEVFN_MAX);
> > -        s->address_spaces[bus_num] = pvtd_as;
> > -    }
> > -    if (!pvtd_as[devfn]) {
> > -        pvtd_as[devfn] = g_malloc0(sizeof(VTDAddressSpace));
> > -
> > -        pvtd_as[devfn]->bus_num = (uint8_t)bus_num;
> > -        pvtd_as[devfn]->devfn = (uint8_t)devfn;
> > -        pvtd_as[devfn]->iommu_state = s;
> > -        pvtd_as[devfn]->context_cache_entry.context_cache_gen = 0;
> > -        memory_region_init_iommu(&pvtd_as[devfn]->iommu,
> > OBJECT(s),
> > -                                 &s->iommu_ops, "intel_iommu",
> > UINT64_MAX);
> > -        address_space_init(&pvtd_as[devfn]->as,
> > -                           &pvtd_as[devfn]->iommu, "intel_iommu");
> > -    }
> > -    return &pvtd_as[devfn]->as;
> > +    vtd_as = vtd_find_add_as(s, bus, devfn);
> > +    return &vtd_as->as;
> >  }
> >  
> >  static void mch_init_dmar(MCHPCIState *mch)
> > diff --git a/include/hw/i386/intel_iommu.h
> > b/include/hw/i386/intel_iommu.h
> > index e321ee4..5dbadb7 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -49,6 +49,7 @@ typedef struct VTDContextCacheEntry
> > VTDContextCacheEntry;
> >  typedef struct IntelIOMMUState IntelIOMMUState;
> >  typedef struct VTDAddressSpace VTDAddressSpace;
> >  typedef struct VTDIOTLBEntry VTDIOTLBEntry;
> > +typedef struct VTDBus VTDBus;
> >  
> >  /* Context-Entry */
> >  struct VTDContextEntry {
> > @@ -65,7 +66,7 @@ struct VTDContextCacheEntry {
> >  };
> >  
> >  struct VTDAddressSpace {
> > -    uint8_t bus_num;
> > +    PCIBus *bus;
> >      uint8_t devfn;
> >      AddressSpace as;
> >      MemoryRegion iommu;
> > @@ -73,6 +74,11 @@ struct VTDAddressSpace {
> >      VTDContextCacheEntry context_cache_entry;
> >  };
> >  
> > +struct VTDBus {
> > +    PCIBus* bus;		/* A reference to the bus to
> > provide translation for */
> > +    VTDAddressSpace *dev_as[0];	/* A table of
> > VTDAddressSpace objects indexed by devfn */
> > +};
> > +
> >  struct VTDIOTLBEntry {
> >      uint64_t gfn;
> >      uint16_t domain_id;
> > @@ -114,7 +120,13 @@ struct IntelIOMMUState {
> >      GHashTable *iotlb;              /* IOTLB */
> >  
> >      MemoryRegionIOMMUOps iommu_ops;
> > -    VTDAddressSpace **address_spaces[VTD_PCI_BUS_MAX];
> > +    GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by
> > PCIBus* reference */
> > +    VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects
> > indexed by bus number */
> >  };
> >  
> > +/* Find the VTD Address space associated with the given bus
> > pointer,
> > + * create a new one if none exists
> > + */
> > +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> > int devfn);
> > +
> >  #endif