Re: [PATCH v1 08/13] intel_iommu: Handle PASID entry removal for pc_inv_dsc request

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

On 3/23/26 14:08, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Subject: Re: [PATCH v1 08/13] intel_iommu: Handle PASID entry removal for
>> pc_inv_dsc request
>>
>> On 3/6/26 11:44, Zhenzhong Duan wrote:
>>> When guest deletes PASID entries, QEMU will capture the pasid cache
>>> invalidation request, walk through pasid_cache_list in each passthrough
>>> device to find stale VTDACCELPASIDCacheEntry and delete them.
>>
>> As the comment to the last patch, I think this patch should handle both
>> update and removal of PASID entry.
>>
>>> This happen before the PASID entry addition, because a new added entry
>>> should never be removed.
>>
>> This is confusing. TBH. this order is not quite important as the current
>> implementation, we don't have multiple pasid entires share same cache
>> entry, so a given pasid entry, we either add new cache entry or
>> update/delete cache entry. So I would just drop this line.
> 
> OK, will remove it.
> 
> In fact, the order is for performance reason. Imaging there are lots of pasid entires
> in pasid_cache_list, we remove stale entries first, then in addition code when we
> loop on pasid_cache_list to see if a new pasid entry already exists, we can iterate
> less entries.

got it. you can mark it in the code.

> Thanks
> Zhenzhong
> 
>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/i386/intel_iommu_accel.c | 70
>> +++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 70 insertions(+)
>>>
>>> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
>>> index 0acf3ae77f..5a956af916 100644
>>> --- a/hw/i386/intel_iommu_accel.c
>>> +++ b/hw/i386/intel_iommu_accel.c
>>> @@ -16,6 +16,28 @@
>>>    #include "hw/pci/pci_bus.h"
>>>    #include "trace.h"
>>>
>>> +static inline int vtd_hiod_get_pe_from_pasid(VTDACCELPASIDCacheEntry
>> *vtd_pce,
>>> +                                             VTDPASIDEntry *pe)
>>> +{
>>> +    VTDHostIOMMUDevice *vtd_hiod = vtd_pce->vtd_hiod;
>>> +    IntelIOMMUState *s = vtd_hiod->iommu_state;
>>> +    uint32_t pasid = vtd_pce->pasid;
>>> +    VTDContextEntry ce;
>>> +    int ret;
>>> +
>>> +    if (!s->dmar_enabled || !s->root_scalable) {
>>> +        return -VTD_FR_RTADDR_INV_TTM;
>>> +    }
>>> +
>>> +    ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_hiod->bus),
>>> +                                   vtd_hiod->devfn, &ce);
>>> +    if (ret) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return vtd_ce_get_pasid_entry(s, &ce, pe, pasid);
>>> +}
>>> +
>>>    bool vtd_check_hiod_accel(IntelIOMMUState *s, VTDHostIOMMUDevice
>> *vtd_hiod,
>>>                              Error **errp)
>>>    {
>>> @@ -257,6 +279,52 @@ void
>> vtd_flush_host_piotlb_all_locked(IntelIOMMUState *s, uint16_t domain_id,
>>>                             vtd_flush_host_piotlb_locked, &piotlb_info);
>>>    }
>>>
>>> +static void vtd_pasid_cache_invalidate_one(VTDACCELPASIDCacheEntry
>> *vtd_pce,
>>> +                                           VTDPASIDCacheInfo *pc_info)
>>> +{
>>> +    VTDPASIDEntry pe;
>>> +    uint16_t did;
>>> +
>>> +    /*
>>> +     * VTD_INV_DESC_PASIDC_G_DSI and VTD_INV_DESC_PASIDC_G_PASID_SI
>> require
>>> +     * DID check. If DID doesn't match the value in cache or memory, then
>>> +     * it's not a pasid entry we want to invalidate.
>>> +     */
>>> +    switch (pc_info->type) {
>>> +    case VTD_INV_DESC_PASIDC_G_PASID_SI:
>>> +        if (pc_info->pasid != vtd_pce->pasid) {
>>> +            return;
>>> +        }
>>> +        /* Fall through */
>>> +    case VTD_INV_DESC_PASIDC_G_DSI:
>>> +        did = VTD_SM_PASID_ENTRY_DID(&vtd_pce->pe);
>>> +        if (pc_info->did != did) {
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    if (vtd_hiod_get_pe_from_pasid(vtd_pce, &pe)) {
>>> +        /*
>>> +         * No valid pasid entry in guest memory. e.g. pasid entry was modified
>>> +         * to be either all-zero or non-present. Either case means existing
>>> +         * pasid cache should be invalidated.
>>> +         */
>>> +        QLIST_REMOVE(vtd_pce, next);
>>> +        g_free(vtd_pce);
>>
>> call vtd_accel_update_pc() and vtd_accel_delete_pc() accordingly.
>>
>>> +    }
>>> +}
>>> +
>>> +/* Delete invalid pasid cache entry from pasid_cache_list */
>>> +static void vtd_pasid_cache_invalidate(VTDHostIOMMUDevice *vtd_hiod,
>>> +                                       VTDPASIDCacheInfo *pc_info)
>>> +{
>>> +    VTDACCELPASIDCacheEntry *vtd_pce, *next;
>>> +
>>> +    QLIST_FOREACH_SAFE(vtd_pce, &vtd_hiod->pasid_cache_list, next, next) {
>>> +        vtd_pasid_cache_invalidate_one(vtd_pce, pc_info);
>>> +    }
>>> +}
>>> +
>>>    static void vtd_find_add_pc(VTDHostIOMMUDevice *vtd_hiod, uint32_t pasid,
>>>                                VTDPASIDEntry *pe)
>>>    {
>>> @@ -423,6 +491,8 @@ void vtd_pasid_cache_sync_accel(IntelIOMMUState *s,
>> VTDPASIDCacheInfo *pc_info)
>>>                                     TYPE_HOST_IOMMU_DEVICE_IOMMUFD)) {
>>>                continue;
>>>            }
>>> +
>>> +        vtd_pasid_cache_invalidate(vtd_hiod, pc_info);
>>>            vtd_replay_pasid_bind_for_dev(vtd_hiod, start, end, pc_info);
>>>        }
>>>    }
>