>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Subject: Re: [PATCH v6 18/19] intel_iommu: Implement
>[set|unset]_iommu_device() callbacks
>
>
>
>On 6/3/24 08:10, Zhenzhong Duan wrote:
>> From: Yi Liu <yi.l....@intel.com>
>>
>> Implement [set|unset]_iommu_device() callbacks in Intel vIOMMU.
>> In set call, a new structure VTDHostIOMMUDevice which holds
>> a reference to HostIOMMUDevice is stored in hash table
>> indexed by PCI BDF.
>>
>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>> ---
>> hw/i386/intel_iommu_internal.h | 9 ++++
>> include/hw/i386/intel_iommu.h | 2 +
>> hw/i386/intel_iommu.c | 76
>++++++++++++++++++++++++++++++++++
>> 3 files changed, 87 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h
>b/hw/i386/intel_iommu_internal.h
>> index f8cf99bddf..b800d62ca0 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -28,6 +28,7 @@
>> #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"
>>
>> /*
>> * Intel IOMMU register specification
>> @@ -537,4 +538,12 @@ 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;
>> + 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 7d694b0813..2bbde41e45 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -293,6 +293,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 519063c8f8..747c988bc4 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,70 @@ VTDAddressSpace
>*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>> return vtd_dev_as;
>> }
>>
>> +static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int
>devfn,
>> + HostIOMMUDevice *hiod, Error **errp)
>> +{
>> + IntelIOMMUState *s = opaque;
>> + VTDHostIOMMUDevice *vtd_hdev;
>> + struct vtd_as_key key = {
>> + .bus = bus,
>> + .devfn = devfn,
>> + };
>> + struct vtd_as_key *new_key;
>> +
>> + assert(hiod);
>> +
>> + 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 false;
>> + }
>> +
>> + vtd_hdev = g_malloc0(sizeof(VTDHostIOMMUDevice));
>> + vtd_hdev->bus = bus;
>> + vtd_hdev->devfn = (uint8_t)devfn;
>> + vtd_hdev->iommu_state = s;
>> + vtd_hdev->dev = hiod;
>I am still not totally clear about why we couldn't reuse VTDAddressSpace
>instance for this bus/devid. Is it a matter of aliased versus non
>aliased bus/devfn, or a matter of pasid diff. An AddressSpace could back
>an assigned device in which case a HostIOMMUDevice could be added to
>this latter. I think this should be explained in the commit msg
Yes, as you said, it's a matter of aliased vs non aliased BDF.
VTDAddressSpace is per aliased BDF while VTDHostIOMMUDevice is per non aliased
BDF.
There can be multiple assigned devices under same virtual iommu group and share
same
VTDAddressSpace, but they have their own VTDHostIOMMUDevice.
Will refine commit msg.
Thanks
Zhenzhong
>
>Eric
>> +
>> + new_key = g_malloc(sizeof(*new_key));
>> + new_key->bus = bus;
>> + new_key->devfn = devfn;
>> +
>> + object_ref(hiod);
>> + g_hash_table_insert(s->vtd_host_iommu_dev, new_key, vtd_hdev);
>> +
>> + vtd_iommu_unlock(s);
>> +
>> + return true;
>> +}
>> +
>> +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);
>> + object_unref(vtd_hdev->dev);
>> +
>> + vtd_iommu_unlock(s);
>> +}
>> +
>> /* Unmap the whole range in the notifier's scope. */
>> static void vtd_address_space_unmap(VTDAddressSpace *as,
>IOMMUNotifier *n)
>> {
>> @@ -4116,6 +4187,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)
>> @@ -4235,6 +4308,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. */
