RE: [PATCH rfcv1 3/6] intel_iommu: add set/unset_iommu_device callback

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH rfcv1 3/6] intel_iommu: add set/unset_iommu_device
>callback
>
>On 1/15/24 11:13, Zhenzhong Duan wrote:
>> From: Yi Liu <yi.l.liu@intel.com>
>>
>> This adds set/unset_iommu_device() implementation in Intel vIOMMU.
>> In set call, IOMMUFDDevice is recorded 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>
>> ---
>>   include/hw/i386/intel_iommu.h | 10 +++++
>>   hw/i386/intel_iommu.c         | 79
>+++++++++++++++++++++++++++++++++++
>>   2 files changed, 89 insertions(+)
>>
>> diff --git a/include/hw/i386/intel_iommu.h
>b/include/hw/i386/intel_iommu.h
>> index 7fa0a695c8..c65fdde56f 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -62,6 +62,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
>>   typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
>>   typedef struct VTDPASIDDirEntry VTDPASIDDirEntry;
>>   typedef struct VTDPASIDEntry VTDPASIDEntry;
>> +typedef struct VTDIOMMUFDDevice VTDIOMMUFDDevice;
>>
>>   /* Context-Entry */
>>   struct VTDContextEntry {
>> @@ -148,6 +149,13 @@ struct VTDAddressSpace {
>>       IOVATree *iova_tree;
>>   };
>>
>> +struct VTDIOMMUFDDevice {
>> +    PCIBus *bus;
>> +    uint8_t devfn;
>> +    IOMMUFDDevice *idev;
>> +    IntelIOMMUState *iommu_state;
>> +};
>
>Does the VTDIOMMUFDDevice definition need to be public ?

No need, will move it to hw/i386/intel_iommu_internal.h
It looks I need to do the same for other definitions in nesting series.

>
>>   struct VTDIOTLBEntry {
>>       uint64_t gfn;
>>       uint16_t domain_id;
>> @@ -292,6 +300,8 @@ struct IntelIOMMUState {
>>       /* list of registered notifiers */
>>       QLIST_HEAD(, VTDAddressSpace) vtd_as_with_notifiers;
>>
>> +    GHashTable *vtd_iommufd_dev;             /* VTDIOMMUFDDevice */
>> +
>>       /* 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 ed5677c0ae..95faf697eb 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,74 @@ VTDAddressSpace
>*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>       return vtd_dev_as;
>>   }
>>
>> +static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque,
>int32_t devfn,
>> +                                    IOMMUFDDevice *idev, Error **errp)
>> +{
>> +    IntelIOMMUState *s = opaque;
>> +    VTDIOMMUFDDevice *vtd_idev;
>> +    struct vtd_as_key key = {
>> +        .bus = bus,
>> +        .devfn = devfn,
>> +    };
>> +    struct vtd_as_key *new_key;
>> +
>> +    assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
>
>Can we move the assert earlier in the call stack ?
>pci_device_get_iommu_bus_devfn() looks like a good place.

Sure.

>
>> +
>> +    /* None IOMMUFD case */
>> +    if (!idev) {
>> +        return 0;
>> +    }
>
>Can we move this test in the helper ? (Looks like an error to me).

We need to pass in NULL idev to do further check in nesting series.
See 
https://github.com/yiliu1765/qemu/commit/7f0bb59575bb5cf38618ae950f68a8661676e881

Thanks
Zhenzhong