>-----Original Message-----
>From: Cédric Le Goater <c...@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....@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....@intel.com>
>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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
