* [Qemu-devel] [PATCH v4 0/1] intel_iommu: Add support for translation for devices behind bridges
@ 2015-09-26 6:09 Knut Omang
2015-09-26 6:09 ` [Qemu-devel] [PATCH v4 1/1] " Knut Omang
0 siblings, 1 reply; 4+ messages in thread
From: Knut Omang @ 2015-09-26 6:09 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Williamson, Eduardo Habkost, Michael S. Tsirkin, Knut Omang,
Alexander Graf, Le Tan, Andreas Färber, qemu-ppc, Jan Kiszka,
Paolo Bonzini, Richard Henderson, David Gibson
This patch set has been completely reimplemented according to ideas from the
discussion of v2.
It still solves the same problem, but does so only within the Intel IOMMU code and Q35,
without changing the IOMMU interface. This eliminates the need for any separate
interface change patch.
This is the thread following v2 of the patch set:
http://thread.gmane.org/gmane.comp.emulators.qemu/358525
This is the thread following the initial patch set:
http://thread.gmane.org/gmane.comp.emulators.qemu/302246
The patch set was also discussed in this thread:
http://thread.gmane.org/gmane.comp.emulators.qemu/316949
Changes from v3:
- Replaced use of g_hash_table_add with g_hash_table_insert
to support compiling on older versions of glib.
Changes from v2:
- Completely reimplemented fix to avoid API change and further
logical deviation from how hardware works.
API change no longer necessary, so just a single patch.
Changes from v1:
- Rebased to current master
- Fixed minor syntax issues
Knut Omang (1):
intel_iommu: Add support for translation for devices behind bridges
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(-)
--
2.4.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges
2015-09-26 6:09 [Qemu-devel] [PATCH v4 0/1] intel_iommu: Add support for translation for devices behind bridges Knut Omang
@ 2015-09-26 6:09 ` Knut Omang
2015-09-27 10:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Knut Omang @ 2015-09-26 6:09 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Williamson, Eduardo Habkost, Michael S. Tsirkin, Knut Omang,
Alexander Graf, Le Tan, Andreas Färber, qemu-ppc, Jan Kiszka,
Paolo Bonzini, Richard Henderson, David Gibson
- 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>
---
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
--
2.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges
2015-09-26 6:09 ` [Qemu-devel] [PATCH v4 1/1] " Knut Omang
@ 2015-09-27 10:07 ` Michael S. Tsirkin
2015-10-04 13:19 ` Knut Omang
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2015-09-27 10:07 UTC (permalink / raw)
To: Knut Omang
Cc: Alex Williamson, Eduardo Habkost, qemu-devel, Alexander Graf,
Andreas Färber, qemu-ppc, Le Tan, Jan Kiszka, Paolo Bonzini,
Richard Henderson, David Gibson
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.
> ---
> 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
> --
> 2.4.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges
2015-09-27 10:07 ` Michael S. Tsirkin
@ 2015-10-04 13:19 ` Knut Omang
0 siblings, 0 replies; 4+ messages in thread
From: Knut Omang @ 2015-10-04 13:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Eduardo Habkost, qemu-devel, Alexander Graf,
Andreas Färber, qemu-ppc, Le Tan, Jan Kiszka, Paolo Bonzini,
Richard Henderson, David Gibson
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-04 13:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-26 6:09 [Qemu-devel] [PATCH v4 0/1] intel_iommu: Add support for translation for devices behind bridges Knut Omang
2015-09-26 6:09 ` [Qemu-devel] [PATCH v4 1/1] " Knut Omang
2015-09-27 10:07 ` Michael S. Tsirkin
2015-10-04 13:19 ` Knut Omang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).