>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>Subject: Re: [PATCH v6 18/19] intel_iommu: Implement
>[set|unset]_iommu_device() callbacks
>
>On 6/3/24 13:02, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com>
>>> Subject: Re: [PATCH v6 18/19] intel_iommu: Implement
>>> [set|unset]_iommu_device() callbacks
>>>
>>>
>>> On 03/06/2024 08:10, Zhenzhong Duan wrote:
>>>> Caution: External email. Do not open attachments or click links, unless
>this
>>> email comes from a known sender and you know the content is safe.
>>>>
>>>>
>>>> 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;
>>>> +
>>>> + 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);
>>> Not sure but isn't that a potential use after free?
>>
>> Good catch! Will fix. Should be:
>>
>> object_unref(vtd_hdev->dev);
>> g_hash_table_remove(s->vtd_host_iommu_dev, &key);
>
>you could also implement a custom destroy hash function.
Yes, but I'd like to have it to match object_ref() call in
vtd_dev_set_iommu_device()
Thanks
Zhenzhong
