On Thu, Sep 24, 2015 at 04:20:53PM +0300, Michael S. Tsirkin wrote:
> From: Knut Omang <knut.om...@oracle.com>
>
> - 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.om...@oracle.com>
> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
This fails to build with our minimal glib version:
Undefined symbols for architecture x86_64:
"_g_hash_table_add", referenced from:
_vtd_find_add_as in intel_iommu.o
SETFILE lm32-softmmu/qemu-system-lm32
g_hash_table_add only appeared in glib 2.32; our minimum
is 2.22.
Dropped this patch for now, please fix and repost.
> ---
> include/hw/i386/intel_iommu.h | 16 +++++++-
> hw/i386/intel_iommu.c | 90
> +++++++++++++++++++++++++++++++++++--------
> hw/pci-host/q35.c | 25 ++----------
> 3 files changed, 91 insertions(+), 40 deletions(-)
>
> 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
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 08055a8..da67c36 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_add(s->vtd_as_by_busptr, 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)
> --
> MST
>
