| Message ID | 20240201072818.327930-12-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Check and sync host IOMMU cap/ecap with vIOMMU | expand |
On 2/1/24 08:28, Zhenzhong Duan wrote: > From: Yi Liu <yi.l.liu@intel.com> > > This adds set/unset_iommu_device() implementation in Intel vIOMMU. > In set call, a pointer to host IOMMU device info is stored in hash > table indexed by PCI BDF. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/i386/intel_iommu_internal.h | 14 +++++++ > include/hw/i386/intel_iommu.h | 2 + > hw/i386/intel_iommu.c | 74 ++++++++++++++++++++++++++++++++++ > 3 files changed, 90 insertions(+) > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index f8cf99bddf..3301f54b35 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -28,6 +28,8 @@ > #ifndef HW_I386_INTEL_IOMMU_INTERNAL_H > #define HW_I386_INTEL_IOMMU_INTERNAL_H > #include "hw/i386/intel_iommu.h" > +#include "sysemu/host_iommu_device.h" > +#include "hw/vfio/vfio-common.h" > > /* > * Intel IOMMU register specification > @@ -537,4 +539,16 @@ typedef struct VTDRootEntry VTDRootEntry; > #define VTD_SL_IGN_COM 0xbff0000000000000ULL > #define VTD_SL_TM (1ULL << 62) > > + > +typedef struct VTDHostIOMMUDevice { > + IntelIOMMUState *iommu_state; > + PCIBus *bus; > + uint8_t devfn; > + union { > + HostIOMMUDevice *dev; > + IOMMULegacyDevice *ldev; > + IOMMUFDDevice *idev; > + }; again this looks really weird to me. Why don't we simply have HostIOMMUDevice *dev; > + QLIST_ENTRY(VTDHostIOMMUDevice) next; > +} VTDHostIOMMUDevice; > #endif > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 7fa0a695c8..bbc7b96add 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -292,6 +292,8 @@ struct IntelIOMMUState { > /* list of registered notifiers */ > QLIST_HEAD(, VTDAddressSpace) vtd_as_with_notifiers; > > + GHashTable *vtd_host_iommu_dev; /* VTDHostIOMMUDevice */ > + > /* interrupt remapping */ > bool intr_enabled; /* Whether guest enabled IR */ > dma_addr_t intr_root; /* Interrupt remapping table pointer */ > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1a07faddb4..9b62441439 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -237,6 +237,13 @@ static gboolean vtd_as_equal(gconstpointer v1, gconstpointer v2) > (key1->pasid == key2->pasid); > } > > +static gboolean vtd_as_idev_equal(gconstpointer v1, gconstpointer v2) > +{ > + const struct vtd_as_key *key1 = v1; > + const struct vtd_as_key *key2 = v2; > + > + return (key1->bus == key2->bus) && (key1->devfn == key2->devfn); > +} > /* > * Note that we use pointer to PCIBus as the key, so hashing/shifting > * based on the pointer value is intended. Note that we deal with > @@ -3812,6 +3819,68 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, > return vtd_dev_as; > } > > +static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > + HostIOMMUDevice *base_dev, Error **errp) > +{ > + IntelIOMMUState *s = opaque; > + VTDHostIOMMUDevice *vtd_hdev; > + struct vtd_as_key key = { > + .bus = bus, > + .devfn = devfn, > + }; > + struct vtd_as_key *new_key; > + > + assert(base_dev); > + > + vtd_iommu_lock(s); > + > + vtd_hdev = g_hash_table_lookup(s->vtd_host_iommu_dev, &key); > + > + if (vtd_hdev) { > + error_setg(errp, "IOMMUFD device already exist"); > + vtd_iommu_unlock(s); > + return -EEXIST; > + } > + > + vtd_hdev = g_malloc0(sizeof(VTDHostIOMMUDevice)); > + vtd_hdev->bus = bus; > + vtd_hdev->devfn = (uint8_t)devfn; > + vtd_hdev->iommu_state = s; > + vtd_hdev->dev = base_dev; and here you set the base pointer. > + > + new_key = g_malloc(sizeof(*new_key)); > + new_key->bus = bus; > + new_key->devfn = devfn; > + > + g_hash_table_insert(s->vtd_host_iommu_dev, new_key, vtd_hdev); > + > + vtd_iommu_unlock(s); > + > + return 0; > +} > + > +static void vtd_dev_unset_iommu_device(PCIBus *bus, void *opaque, int devfn) > +{ > + IntelIOMMUState *s = opaque; > + VTDHostIOMMUDevice *vtd_hdev; > + struct vtd_as_key key = { > + .bus = bus, > + .devfn = devfn, > + }; > + > + vtd_iommu_lock(s); > + > + vtd_hdev = g_hash_table_lookup(s->vtd_host_iommu_dev, &key); > + if (!vtd_hdev) { > + vtd_iommu_unlock(s); > + return; > + } > + > + g_hash_table_remove(s->vtd_host_iommu_dev, &key); > + > + vtd_iommu_unlock(s); > +} > + > /* Unmap the whole range in the notifier's scope. */ > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) > { > @@ -4107,6 +4176,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > > static PCIIOMMUOps vtd_iommu_ops = { > .get_address_space = vtd_host_dma_iommu, > + .set_iommu_device = vtd_dev_set_iommu_device, > + .unset_iommu_device = vtd_dev_unset_iommu_device, > }; > > static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > @@ -4230,6 +4301,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) > g_free, g_free); > s->vtd_address_spaces = g_hash_table_new_full(vtd_as_hash, vtd_as_equal, > g_free, g_free); > + s->vtd_host_iommu_dev = g_hash_table_new_full(vtd_as_hash, > + vtd_as_idev_equal, > + g_free, g_free); > vtd_init(s); > pci_setup_iommu(bus, &vtd_iommu_ops, dev); > /* Pseudo address space under root PCI bus. */ Eric
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH rfcv2 11/18] intel_iommu: Add >set/unset_iommu_device callback > > > >On 2/1/24 08:28, Zhenzhong Duan wrote: >> From: Yi Liu <yi.l.liu@intel.com> >> >> This adds set/unset_iommu_device() implementation in Intel vIOMMU. >> In set call, a pointer to host IOMMU device info is stored in hash >> table indexed by PCI BDF. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/i386/intel_iommu_internal.h | 14 +++++++ >> include/hw/i386/intel_iommu.h | 2 + >> hw/i386/intel_iommu.c | 74 >++++++++++++++++++++++++++++++++++ >> 3 files changed, 90 insertions(+) >> >> diff --git a/hw/i386/intel_iommu_internal.h >b/hw/i386/intel_iommu_internal.h >> index f8cf99bddf..3301f54b35 100644 >> --- a/hw/i386/intel_iommu_internal.h >> +++ b/hw/i386/intel_iommu_internal.h >> @@ -28,6 +28,8 @@ >> #ifndef HW_I386_INTEL_IOMMU_INTERNAL_H >> #define HW_I386_INTEL_IOMMU_INTERNAL_H >> #include "hw/i386/intel_iommu.h" >> +#include "sysemu/host_iommu_device.h" >> +#include "hw/vfio/vfio-common.h" >> >> /* >> * Intel IOMMU register specification >> @@ -537,4 +539,16 @@ typedef struct VTDRootEntry VTDRootEntry; >> #define VTD_SL_IGN_COM 0xbff0000000000000ULL >> #define VTD_SL_TM (1ULL << 62) >> >> + >> +typedef struct VTDHostIOMMUDevice { >> + IntelIOMMUState *iommu_state; >> + PCIBus *bus; >> + uint8_t devfn; >> + union { >> + HostIOMMUDevice *dev; >> + IOMMULegacyDevice *ldev; >> + IOMMUFDDevice *idev; >> + }; >again this looks really weird to me. Why don't we simply have > >HostIOMMUDevice *dev; Sure, will do. Thanks Zhenzhong
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h index f8cf99bddf..3301f54b35 100644 --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -28,6 +28,8 @@ #ifndef HW_I386_INTEL_IOMMU_INTERNAL_H #define HW_I386_INTEL_IOMMU_INTERNAL_H #include "hw/i386/intel_iommu.h" +#include "sysemu/host_iommu_device.h" +#include "hw/vfio/vfio-common.h" /* * Intel IOMMU register specification @@ -537,4 +539,16 @@ typedef struct VTDRootEntry VTDRootEntry; #define VTD_SL_IGN_COM 0xbff0000000000000ULL #define VTD_SL_TM (1ULL << 62) + +typedef struct VTDHostIOMMUDevice { + IntelIOMMUState *iommu_state; + PCIBus *bus; + uint8_t devfn; + union { + HostIOMMUDevice *dev; + IOMMULegacyDevice *ldev; + IOMMUFDDevice *idev; + }; + QLIST_ENTRY(VTDHostIOMMUDevice) next; +} VTDHostIOMMUDevice; #endif diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 7fa0a695c8..bbc7b96add 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -292,6 +292,8 @@ struct IntelIOMMUState { /* list of registered notifiers */ QLIST_HEAD(, VTDAddressSpace) vtd_as_with_notifiers; + GHashTable *vtd_host_iommu_dev; /* VTDHostIOMMUDevice */ + /* interrupt remapping */ bool intr_enabled; /* Whether guest enabled IR */ dma_addr_t intr_root; /* Interrupt remapping table pointer */ diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 1a07faddb4..9b62441439 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -237,6 +237,13 @@ static gboolean vtd_as_equal(gconstpointer v1, gconstpointer v2) (key1->pasid == key2->pasid); } +static gboolean vtd_as_idev_equal(gconstpointer v1, gconstpointer v2) +{ + const struct vtd_as_key *key1 = v1; + const struct vtd_as_key *key2 = v2; + + return (key1->bus == key2->bus) && (key1->devfn == key2->devfn); +} /* * Note that we use pointer to PCIBus as the key, so hashing/shifting * based on the pointer value is intended. Note that we deal with @@ -3812,6 +3819,68 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, return vtd_dev_as; } +static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, + HostIOMMUDevice *base_dev, Error **errp) +{ + IntelIOMMUState *s = opaque; + VTDHostIOMMUDevice *vtd_hdev; + struct vtd_as_key key = { + .bus = bus, + .devfn = devfn, + }; + struct vtd_as_key *new_key; + + assert(base_dev); + + vtd_iommu_lock(s); + + vtd_hdev = g_hash_table_lookup(s->vtd_host_iommu_dev, &key); + + if (vtd_hdev) { + error_setg(errp, "IOMMUFD device already exist"); + vtd_iommu_unlock(s); + return -EEXIST; + } + + vtd_hdev = g_malloc0(sizeof(VTDHostIOMMUDevice)); + vtd_hdev->bus = bus; + vtd_hdev->devfn = (uint8_t)devfn; + vtd_hdev->iommu_state = s; + vtd_hdev->dev = base_dev; + + new_key = g_malloc(sizeof(*new_key)); + new_key->bus = bus; + new_key->devfn = devfn; + + g_hash_table_insert(s->vtd_host_iommu_dev, new_key, vtd_hdev); + + vtd_iommu_unlock(s); + + return 0; +} + +static void vtd_dev_unset_iommu_device(PCIBus *bus, void *opaque, int devfn) +{ + IntelIOMMUState *s = opaque; + VTDHostIOMMUDevice *vtd_hdev; + struct vtd_as_key key = { + .bus = bus, + .devfn = devfn, + }; + + vtd_iommu_lock(s); + + vtd_hdev = g_hash_table_lookup(s->vtd_host_iommu_dev, &key); + if (!vtd_hdev) { + vtd_iommu_unlock(s); + return; + } + + g_hash_table_remove(s->vtd_host_iommu_dev, &key); + + vtd_iommu_unlock(s); +} + /* Unmap the whole range in the notifier's scope. */ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) { @@ -4107,6 +4176,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) static PCIIOMMUOps vtd_iommu_ops = { .get_address_space = vtd_host_dma_iommu, + .set_iommu_device = vtd_dev_set_iommu_device, + .unset_iommu_device = vtd_dev_unset_iommu_device, }; static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) @@ -4230,6 +4301,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) g_free, g_free); s->vtd_address_spaces = g_hash_table_new_full(vtd_as_hash, vtd_as_equal, g_free, g_free); + s->vtd_host_iommu_dev = g_hash_table_new_full(vtd_as_hash, + vtd_as_idev_equal, + g_free, g_free); vtd_init(s); pci_setup_iommu(bus, &vtd_iommu_ops, dev); /* Pseudo address space under root PCI bus. */