在 2022/1/13 下午12:12, Peter Xu 写道:
On Wed, Jan 05, 2022 at 12:19:44PM +0800, Jason Wang wrote:We introduce VTDBus structure as an intermediate step for searching the address space. This works well with SID based matching/lookup. But when we want to support SID plus PASID based address space lookup, this intermediate steps turns out to be a burden. So the patch simply drops the VTDBus structure and use the PCIBus and devfn as the key for the g_hash_table(). This simplifies the codes and the future PASID extension. This may case slight slower for the vtd_find_as_from_bus_num() callers but since they are all called from the control path, we can afford it.The only one I found is vtd_process_device_iotlb_desc() that may got affected the most; the rest look mostly always traversing all the address space anyway so shouldn't hurt. I think dev-iotlb can be invalidated in IO path too when kernel device drivers are used? It shouldn't affect much when the VM has a few devices, but IIUC it'll further slow down the kernel drivers on vIOMMU. Maybe it's not a huge problem either.
Maybe we can keep maintaining a cache for some speedup for the searching for NO_PASID.
Signed-off-by: Jason Wang <jasow...@redhat.com> --- hw/i386/intel_iommu.c | 183 +++++++++++++--------------------- include/hw/i386/intel_iommu.h | 10 +- 2 files changed, 69 insertions(+), 124 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index f2c7a23712..58c682097b 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -61,6 +61,11 @@ } \ }+struct vtd_as_key {+ PCIBus *bus; + uint8_t devfn; +}; + static void vtd_address_space_refresh_all(IntelIOMMUState *s); static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);@@ -190,12 +195,18 @@ static inline gboolean vtd_as_has_map_notifier(VTDAddressSpace *as)/* GHashTable functions */ static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2) { - return *((const uint64_t *)v1) == *((const uint64_t *)v2); + const struct vtd_as_key *key1 = v1; + const struct vtd_as_key *key2 = v2; + + return (key1->bus == key2->bus) && (key1->devfn == key2->devfn); }static guint vtd_uint64_hash(gconstpointer v){ - return (guint)*(const uint64_t *)v; + const struct vtd_as_key *key = v; + guint value = (guint)(uintptr_t)key->bus; + + return (guint)(value << 8 | key->devfn);Note that value is a pointer to PCIBus*. Just want to check with you that it's intended to use this hash value (or maybe you wanted to use Source ID so it is bus number to use not the bus pointer)?
Right, SID should be used here.
}static gboolean vtd_hash_remove_by_domain(gpointer key, gpointer value,@@ -236,22 +247,14 @@ static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value, static void vtd_reset_context_cache_locked(IntelIOMMUState *s) { VTDAddressSpace *vtd_as; - VTDBus *vtd_bus; - GHashTableIter bus_it; - uint32_t devfn_it; + GHashTableIter as_it;trace_vtd_context_cache_reset(); - g_hash_table_iter_init(&bus_it, s->vtd_as_by_busptr);+ g_hash_table_iter_init(&as_it, s->vtd_as);- while (g_hash_table_iter_next (&bus_it, NULL, (void**)&vtd_bus)) {- for (devfn_it = 0; devfn_it < PCI_DEVFN_MAX; ++devfn_it) { - vtd_as = vtd_bus->dev_as[devfn_it]; - if (!vtd_as) { - continue; - } - vtd_as->context_cache_entry.context_cache_gen = 0; - } + while (g_hash_table_iter_next (&as_it, NULL, (void**)&vtd_as)) { + vtd_as->context_cache_entry.context_cache_gen = 0; } s->context_cache_gen = 1; } @@ -986,32 +989,6 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level) return slpte & rsvd_mask; }-/* Find the VTD address space 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]; - GHashTableIter iter; - - if (vtd_bus) { - return vtd_bus; - } - - /* - * Iterate over the registered buses to find the one which - * currently holds this bus number and update the bus_num - * lookup table. - */ - g_hash_table_iter_init(&iter, s->vtd_as_by_busptr); - while (g_hash_table_iter_next(&iter, NULL, (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 NULL; -} - /* Given the @iova, get relevant @slptep. @slpte_level will be the last level * of the translation, can be used for deciding the size of large page. */ @@ -1604,18 +1581,12 @@ static bool vtd_switch_address_space(VTDAddressSpace *as)static void vtd_switch_address_space_all(IntelIOMMUState *s){ + VTDAddressSpace *vtd_as; GHashTableIter iter; - VTDBus *vtd_bus; - int i; - - g_hash_table_iter_init(&iter, s->vtd_as_by_busptr); - while (g_hash_table_iter_next(&iter, NULL, (void **)&vtd_bus)) { - for (i = 0; i < PCI_DEVFN_MAX; i++) { - if (!vtd_bus->dev_as[i]) { - continue; - } - vtd_switch_address_space(vtd_bus->dev_as[i]); - } + + g_hash_table_iter_init(&iter, s->vtd_as); + while (g_hash_table_iter_next(&iter, NULL, (void **)&vtd_as)) { + vtd_switch_address_space(vtd_as); } }@@ -1659,16 +1630,11 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr) static void vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id){ - VTDBus *vtd_bus; VTDAddressSpace *vtd_as; bool success = false; + uintptr_t key = (uintptr_t)source_id;- vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id));- if (!vtd_bus) { - goto out; - } - - vtd_as = vtd_bus->dev_as[VTD_SID_TO_DEVFN(source_id)]; + vtd_as = g_hash_table_lookup(s->vtd_as, &key);I'm slightly confused - what I read below tells me that the key is "struct vtd_as_key" at [1] below, but here it's a uintptr_t contains the SID. I don't think they match.. Did I misread something?
Nope, it looks like a bug, it looks to me we can't simply use SID here since the key requires a pointer to PCIBus. The reason that we can't simply use SID here is that the dma_as is initialized before the guest can initialize the root port where we may end up with wrong bus number. I will fix this by using g_hash_table_find() which could be slow but consider this function is not a frequent operation it should be acceptable.
if (!vtd_as) { goto out; } @@ -1876,11 +1842,10 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, uint16_t source_id, uint16_t func_mask) { + GHashTableIter as_it; uint16_t mask; - VTDBus *vtd_bus; VTDAddressSpace *vtd_as; uint8_t bus_n, devfn; - uint16_t devfn_it;trace_vtd_inv_desc_cc_devices(source_id, func_mask); @@ -1903,32 +1868,31 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,mask = ~mask;bus_n = VTD_SID_TO_BUS(source_id);- vtd_bus = vtd_find_as_from_bus_num(s, bus_n); - if (vtd_bus) { - devfn = VTD_SID_TO_DEVFN(source_id); - for (devfn_it = 0; devfn_it < PCI_DEVFN_MAX; ++devfn_it) { - vtd_as = vtd_bus->dev_as[devfn_it]; - if (vtd_as && ((devfn_it & mask) == (devfn & mask))) { - trace_vtd_inv_desc_cc_device(bus_n, VTD_PCI_SLOT(devfn_it), - VTD_PCI_FUNC(devfn_it)); - vtd_iommu_lock(s); - vtd_as->context_cache_entry.context_cache_gen = 0; - vtd_iommu_unlock(s); - /* - * Do switch address space when needed, in case if the - * device passthrough bit is switched. - */ - vtd_switch_address_space(vtd_as); - /* - * So a device is moving out of (or moving into) a - * domain, resync the shadow page table. - * This won't bring bad even if we have no such - * notifier registered - the IOMMU notification - * framework will skip MAP notifications if that - * happened. - */ - vtd_sync_shadow_page_table(vtd_as); - } + devfn = VTD_SID_TO_DEVFN(source_id); + + g_hash_table_iter_init(&as_it, s->vtd_as); + while (g_hash_table_iter_next(&as_it, NULL, (void**)&vtd_as)) { + if ((pci_bus_num(vtd_as->bus) == bus_n) && + (vtd_as->devfn & mask) == (devfn & mask)) { + trace_vtd_inv_desc_cc_device(bus_n, VTD_PCI_SLOT(vtd_as->devfn), + VTD_PCI_FUNC(vtd_as->devfn)); + vtd_iommu_lock(s); + vtd_as->context_cache_entry.context_cache_gen = 0; + vtd_iommu_unlock(s); + /* + * Do switch address space when needed, in case if the + * device passthrough bit is switched. + */ + vtd_switch_address_space(vtd_as); + /* + * So a device is moving out of (or moving into) a + * domain, resync the shadow page table. + * This won't bring bad even if we have no such + * notifier registered - the IOMMU notification + * framework will skip MAP notifications if that + * happened. + */ + vtd_sync_shadow_page_table(vtd_as); } } } @@ -2442,18 +2406,14 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, { VTDAddressSpace *vtd_dev_as; IOMMUTLBEvent event; - struct VTDBus *vtd_bus; + uintptr_t key; hwaddr addr; uint64_t sz; uint16_t sid; - uint8_t devfn; bool size; - uint8_t bus_num;addr = VTD_INV_DESC_DEVICE_IOTLB_ADDR(inv_desc->hi);sid = VTD_INV_DESC_DEVICE_IOTLB_SID(inv_desc->lo); - devfn = sid & 0xff; - bus_num = sid >> 8; size = VTD_INV_DESC_DEVICE_IOTLB_SIZE(inv_desc->hi);if ((inv_desc->lo & VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO) ||@@ -2464,12 +2424,8 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, return false; }- vtd_bus = vtd_find_as_from_bus_num(s, bus_num);- if (!vtd_bus) { - goto done; - } - - vtd_dev_as = vtd_bus->dev_as[devfn]; + key = (uintptr_t)sid; + vtd_dev_as = g_hash_table_lookup(s->vtd_as, &key);Same question here.
See above reply. Thanks
if (!vtd_dev_as) { goto done; } @@ -3390,27 +3346,22 @@ static const MemoryRegionOps vtd_mem_ir_ops = {VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn){ - uintptr_t key = (uintptr_t)bus; - VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); - VTDAddressSpace *vtd_dev_as; + struct vtd_as_key key = { + .bus = bus, + .devfn = devfn, + }; + VTDAddressSpace *vtd_dev_as = g_hash_table_lookup(s->vtd_as, &key); char name[128];- if (!vtd_bus) {- uintptr_t *new_key = g_malloc(sizeof(*new_key)); - *new_key = (uintptr_t)bus; - /* No corresponding free() */ - vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \ - PCI_DEVFN_MAX); - vtd_bus->bus = bus; - g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus); - } + if (!vtd_dev_as) { + struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));- vtd_dev_as = vtd_bus->dev_as[devfn];+ new_key->bus = bus; + new_key->devfn = devfn;- if (!vtd_dev_as) {snprintf(name, sizeof(name), "vtd-%02x.%x", PCI_SLOT(devfn), PCI_FUNC(devfn)); - vtd_bus->dev_as[devfn] = vtd_dev_as = g_malloc0(sizeof(VTDAddressSpace)); + vtd_dev_as = g_malloc0(sizeof(VTDAddressSpace));vtd_dev_as->bus = bus;vtd_dev_as->devfn = (uint8_t)devfn; @@ -3466,6 +3417,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) &vtd_dev_as->nodmar, 0);vtd_switch_address_space(vtd_dev_as);+ + g_hash_table_insert(s->vtd_as, new_key, vtd_dev_as);[1]} return vtd_dev_as; } @@ -3750,6 +3703,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) assert(0 <= devfn && devfn < PCI_DEVFN_MAX);vtd_as = vtd_find_add_as(s, bus, devfn);+ return &vtd_as->as; }@@ -3835,7 +3789,6 @@ static void vtd_realize(DeviceState *dev, Error **errp) QLIST_INIT(&s->vtd_as_with_notifiers);qemu_mutex_init(&s->iommu_lock); - 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);@@ -3857,8 +3810,8 @@ static void vtd_realize(DeviceState *dev, Error **errp)/* 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); + s->vtd_as = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, + g_free, g_free); vtd_init(s); sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); pci_setup_iommu(bus, vtd_host_dma_iommu, dev); diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 41783ee46d..014d77dc2a 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -58,7 +58,6 @@ typedef struct VTDContextEntry VTDContextEntry; typedef struct VTDContextCacheEntry VTDContextCacheEntry; typedef struct VTDAddressSpace VTDAddressSpace; typedef struct VTDIOTLBEntry VTDIOTLBEntry; -typedef struct VTDBus VTDBus; typedef union VTD_IR_TableEntry VTD_IR_TableEntry; typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; typedef struct VTDPASIDDirEntry VTDPASIDDirEntry; @@ -111,12 +110,6 @@ struct VTDAddressSpace { IOVATree *iova_tree; /* Traces mapped IOVA ranges */ };-struct VTDBus {- PCIBus* bus; /* A reference to the bus to provide translation for */ - /* A table of VTDAddressSpace objects indexed by devfn */ - VTDAddressSpace *dev_as[]; -}; - struct VTDIOTLBEntry { uint64_t gfn; uint16_t domain_id; @@ -252,8 +245,7 @@ struct IntelIOMMUState { uint32_t context_cache_gen; /* Should be in [1,MAX] */ GHashTable *iotlb; /* IOTLB */- 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 */ + GHashTable *vtd_as; /* VTD address space indexed by source id*/ /* list of registered notifiers */ QLIST_HEAD(, VTDAddressSpace) vtd_as_with_notifiers;--2.25.1
