>-----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/4/24 07:46, Duan, Zhenzhong wrote:
>>
>>> -----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.
>
>OK thank you for the confirmation. A general concern is this is the kind
>of code we are going to duplicate in each vIOMMU.
The hash table code can be common, but will we have other duplicate code?
I feel most of the codes are VTD specific.
> This is beyond the
>scope of this series but we shall really think about introducing a
>common base object for vIOMMU. Unfortunately there are issues related to
>multiple inheritence that may prevent us from using usual QOM
>inheritence but just as we have done for backends and HostIOMMUDevice
>we may implement inheritence another way.
Yes, virtio-iommu is different from others, it inherits from TYPE_VIRTIO_DEVICE.
Not get about the another way, could you explain a bit?
Thanks
Zhenzhong
