Re: [PATCH v1 07/13] intel_iommu: Handle PASID entry addition for pc_inv_dsc request

[Yi Liu <yi.l.liu@intel.com>]

On 3/23/26 13:50, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Subject: Re: [PATCH v1 07/13] intel_iommu: Handle PASID entry addition for
>> pc_inv_dsc request
>>
>> On 3/6/26 11:44, Zhenzhong Duan wrote:
>>> Structure VTDAddressSpace includes some elements suitable for emulated
>>> device and passthrough device without PASID, e.g., address space,
>>> different memory regions, etc, it is also protected by vtd iommu lock,
>>> all these are useless and become a burden for passthrough device with
>>> PASID.
>>>
>>> When there are lots of PASIDs used in one device, the AS and MRs are
>>> all registered to memory core and impact the whole system performance.
>>>
>>> So instead of using VTDAddressSpace to cache pasid entry for each pasid
>>> of a passthrough device, we define a light weight structure
>>> VTDACCELPASIDCacheEntry with only necessary elements for each pasid. We
>>> will use this struct as a parameter to conduct binding/unbinding to
>>> nested hwpt and to record the current binded nested hwpt. It's also
>>
>> s/binded/bound/
> 
> OK.
> 
>>
>>> designed to support PASID_0.
>>>
>>> When guest creates new PASID entries, QEMU will capture the pc_inv_dsc
>>> (pasid cache invalidation) request, walk through each pasid in each
>>> passthrough device for valid pasid entries, create a new
>>> VTDACCELPASIDCacheEntry if not existing yet.
>>
>> I think some tweak is preferred w.r.t. this and the next patch.
>>
>> In this patch you only need to handle the PASID entry addition. Hence
>> you assume no existing VTDACCELPASIDCacheEntry yet.
> 
> [...]
> 
>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/i386/intel_iommu_accel.h    |  13 +++
>>>    hw/i386/intel_iommu_internal.h |   8 ++
>>>    hw/i386/intel_iommu.c          |   3 +
>>>    hw/i386/intel_iommu_accel.c    | 170 +++++++++++++++++++++++++++++++++
>>>    4 files changed, 194 insertions(+)
>>>
>>> diff --git a/hw/i386/intel_iommu_accel.h b/hw/i386/intel_iommu_accel.h
>>> index e5f0b077b4..a77fd06fe0 100644
>>> --- a/hw/i386/intel_iommu_accel.h
>>> +++ b/hw/i386/intel_iommu_accel.h
>>> @@ -12,6 +12,13 @@
>>>    #define HW_I386_INTEL_IOMMU_ACCEL_H
>>>    #include CONFIG_DEVICES
>>>
>>> +typedef struct VTDACCELPASIDCacheEntry {
>>> +    VTDHostIOMMUDevice *vtd_hiod;
>>> +    VTDPASIDEntry pe;
>>> +    uint32_t pasid;
>>> +    QLIST_ENTRY(VTDACCELPASIDCacheEntry) next;
>>> +} VTDACCELPASIDCacheEntry;
>>
>> btw. s/VTDACCELPASIDCacheEntry/VTDAccelPASIDCacheEntry/ looks better. :)
> 
> Sure.
> 
>>> +
>>>    #ifdef CONFIG_VTD_ACCEL
>>>    bool vtd_check_hiod_accel(IntelIOMMUState *s, VTDHostIOMMUDevice
>> *vtd_hiod,
>>>                              Error **errp);
>>> @@ -20,6 +27,7 @@ bool vtd_propagate_guest_pasid(VTDAddressSpace
>> *vtd_as, Error **errp);
>>>    void vtd_flush_host_piotlb_all_locked(IntelIOMMUState *s, uint16_t
>> domain_id,
>>>                                          uint32_t pasid, hwaddr addr,
>>>                                          uint64_t npages, bool ih);
>>> +void vtd_pasid_cache_sync_accel(IntelIOMMUState *s, VTDPASIDCacheInfo
>> *pc_info);
>>>    void vtd_iommu_ops_update_accel(PCIIOMMUOps *ops);
>>>    #else
>>>    static inline bool vtd_check_hiod_accel(IntelIOMMUState *s,
>>> @@ -49,6 +57,11 @@ static inline void
>> vtd_flush_host_piotlb_all_locked(IntelIOMMUState *s,
>>>    {
>>>    }
>>>
>>> +static inline void vtd_pasid_cache_sync_accel(IntelIOMMUState *s,
>>> +                                              VTDPASIDCacheInfo *pc_info)
>>> +{
>>> +}
>>> +
>>>    static inline void vtd_iommu_ops_update_accel(PCIIOMMUOps *ops)
>>>    {
>>>    }
>>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>>> index c7e107fe87..ede4db6d2d 100644
>>> --- a/hw/i386/intel_iommu_internal.h
>>> +++ b/hw/i386/intel_iommu_internal.h
>>> @@ -616,6 +616,7 @@ typedef struct VTDRootEntry VTDRootEntry;
>>>    #define VTD_CTX_ENTRY_SCALABLE_SIZE   32
>>>
>>>    #define PASID_0                             0
>>> +#define VTD_SM_CONTEXT_ENTRY_PDTS(x)        extract64((x)->val[0], 9, 3)
>>>    #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL0(aw)  (0x1e0ULL |
>> ~VTD_HAW_MASK(aw))
>>>    #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL1      0xffffffffffe00000ULL
>>>    #define VTD_SM_CONTEXT_ENTRY_PRE            0x10ULL
>>> @@ -646,6 +647,7 @@ typedef struct VTDPIOTLBInvInfo {
>>>    #define VTD_PASID_DIR_BITS_MASK       (0x3fffULL)
>>>    #define VTD_PASID_DIR_INDEX(pasid)    (((pasid) >> 6) &
>> VTD_PASID_DIR_BITS_MASK)
>>>    #define VTD_PASID_DIR_FPD             (1ULL << 1) /* Fault Processing Disable */
>>> +#define VTD_PASID_TABLE_ENTRY_NUM     (1ULL << 6)
>>>    #define VTD_PASID_TABLE_BITS_MASK     (0x3fULL)
>>>    #define VTD_PASID_TABLE_INDEX(pasid)  ((pasid) &
>> VTD_PASID_TABLE_BITS_MASK)
>>>    #define VTD_PASID_ENTRY_FPD           (1ULL << 1) /* Fault Processing Disable
>> */
>>> @@ -711,6 +713,7 @@ typedef struct VTDHostIOMMUDevice {
>>>        PCIBus *bus;
>>>        uint8_t devfn;
>>>        HostIOMMUDevice *hiod;
>>> +    QLIST_HEAD(, VTDACCELPASIDCacheEntry) pasid_cache_list;
>>>    } VTDHostIOMMUDevice;
>>>
>>>    /*
>>> @@ -768,6 +771,11 @@ static inline int
>> vtd_pasid_entry_compare(VTDPASIDEntry *p1, VTDPASIDEntry *p2)
>>>        return memcmp(p1, p2, sizeof(*p1));
>>>    }
>>>
>>> +static inline uint32_t vtd_sm_ce_get_pdt_entry_num(VTDContextEntry *ce)
>>> +{
>>> +    return 1U << (VTD_SM_CONTEXT_ENTRY_PDTS(ce) + 7);
>>> +}
>>> +
>>>    int vtd_get_pdire_from_pdir_table(dma_addr_t pasid_dir_base, uint32_t pasid,
>>>                                      VTDPASIDDirEntry *pdire);
>>>    int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, uint32_t pasid,
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 744b5967b2..984adc639a 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -3202,6 +3202,8 @@ static void vtd_pasid_cache_sync(IntelIOMMUState
>> *s, VTDPASIDCacheInfo *pc_info)
>>>        g_hash_table_foreach(s->vtd_address_spaces,
>> vtd_pasid_cache_sync_locked,
>>>                             pc_info);
>>>        vtd_iommu_unlock(s);
>>> +
>>> +    vtd_pasid_cache_sync_accel(s, pc_info);
>>>    }
>>>
>>>    static void vtd_replay_pasid_bindings_all(IntelIOMMUState *s)
>>> @@ -4760,6 +4762,7 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus,
>> void *opaque, int devfn,
>>>        vtd_hiod->devfn = (uint8_t)devfn;
>>>        vtd_hiod->iommu_state = s;
>>>        vtd_hiod->hiod = hiod;
>>> +    QLIST_INIT(&vtd_hiod->pasid_cache_list);
>>>
>>>        if (!vtd_check_hiod(s, vtd_hiod, errp)) {
>>>            g_free(vtd_hiod);
>>> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
>>> index c2757f3bcd..0acf3ae77f 100644
>>> --- a/hw/i386/intel_iommu_accel.c
>>> +++ b/hw/i386/intel_iommu_accel.c
>>> @@ -257,6 +257,176 @@ void
>> vtd_flush_host_piotlb_all_locked(IntelIOMMUState *s, uint16_t domain_id,
>>>                             vtd_flush_host_piotlb_locked, &piotlb_info);
>>>    }
>>>
>>> +static void vtd_find_add_pc(VTDHostIOMMUDevice *vtd_hiod, uint32_t pasid,
>>> +                            VTDPASIDEntry *pe)
>>> +{
>>
>> then you can name this as vtd_accel_fill_pc(). And I think you would
>> have vtd_accel_update_pc() and vtd_accel_delete_pc() in the next patch.
> 
> Yes, in current implementation this patch handles pasid entry addition and update,
> next patch handles removal.
> What's the benefit if we move pasid entry update into next patch?

If no redundant flush, for a newly created pasid entry, you need not to
loop the pasid cache entry list. But, it's possible to have such guest. :)

>>
>>> +    VTDACCELPASIDCacheEntry *vtd_pce;
>>> +
>>> +    QLIST_FOREACH(vtd_pce, &vtd_hiod->pasid_cache_list, next) {
>>> +        if (vtd_pce->pasid == pasid) {
>>> +            if (vtd_pasid_entry_compare(pe, &vtd_pce->pe)) {
>>> +                vtd_pce->pe = *pe;
>>> +            }
>>> +            return;
>>> +        }
>>> +    }
>>
>> hence this loop can be avoided.
> 
> Hm, I think guest may send redundant pv_inv_dsc, so we need this loop
> to bypass existing pasid entry to avoid adding duplicate pasid entry.
> Let me know if I misunderstand what you mean.

Although I have an idea to handle it. But it relies on some kind of flag
returned by the removal/update processing path to tell no need to re-
create pasid cache entry in the vtd_replay_pasid_bind_for_dev() path. I
don't think we need to make things so complicated. So let's follow the
current splitting. But I think you still can name this helper as
vtd_accel_fill_pc(). "find" sometimes means return something back.