Re: [PATCH v5 10/15] intel_iommu_accel: Handle PASID entry removal for pc_inv_dsc request

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

On 5/9/26 12:08, 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.
> 
> Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Tested-by: Xudong Hao <xudong.hao@intel.com>
> ---
>   hw/i386/intel_iommu_accel.c | 80 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
> index a66d63b4c8..82bfbdf484 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 int vtd_hiod_get_pe_from_pasid(VTDAccelPASIDCacheEntry *vtd_pce,

this helper is tricky. vtd_pce already has a pe cached, then why need
to do this? I think it's better to have a helper receives s, devfn, and
pasid instead of accepting vtd_pce. btw. I think the intel_iommu.c
should have such a helper. is it?

> +                                      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)
>   {
> @@ -280,6 +302,57 @@ static void vtd_accel_fill_pc(VTDHostIOMMUDevice *vtd_hiod, uint32_t pasid,
>       QLIST_INSERT_HEAD(&vtd_hiod->pasid_cache_list, vtd_pce, next);
>   }
>   
> +static void vtd_accel_delete_pc(VTDAccelPASIDCacheEntry *vtd_pce)
> +{
> +    QLIST_REMOVE(vtd_pce, next);
> +    g_free(vtd_pce);
> +}
> +
> +static void
> +vtd_accel_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->pasid_entry);
> +        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.
> +         */
> +        vtd_accel_delete_pc(vtd_pce);
> +    }
> +}
> +
> +static void vtd_accel_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_accel_pasid_cache_invalidate_one(vtd_pce, pc_info);
> +    }
> +}
> +
>   /*
>    * This function walks over PASID range within [start, end) in a single
>    * PASID table for entries matching @info type/did, then create
> @@ -411,6 +484,13 @@ void vtd_accel_pasid_cache_sync(IntelIOMMUState *s, VTDPASIDCacheInfo *pc_info)
>                                    TYPE_HOST_IOMMU_DEVICE_IOMMUFD)) {
>               continue;
>           }
> +
> +        /*
> +         * PASID entry removal is handled before addition intentionally,
> +         * because it's unnecessary to iterate on an entry that will be
> +         * removed.

how about below? :)

/*
  * The replay path inevitably needs to iterate through existing
  * PASID cache entries. Since cached PASID entries that are marked
  * for removal don't need to be iterated, we intentionally handle
  * removals before additions to optimize the replay process.
  */


> +         */
> +        vtd_accel_pasid_cache_invalidate(vtd_hiod, pc_info);
>           vtd_accel_replay_pasid_bind_for_dev(vtd_hiod, start, end, pc_info);
>       }
>   }