Re: [PATCH v3] intel_iommu: Keep PASID cache consistent with PT fast path

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

On 11/12/26 16:15, Zhenzhong Duan wrote:
> With caching mode disabled, a guest can install a scalable-mode
> pass-through PASID entry without issuing a PASID-cache invalidation. The
> first DMA enables the PT fast path, but leaves QEMU's PASID cache invalid.
> 
> When the guest tears down that entry to switch back to a DMA domain, the
> invalidation handler sees both a non-present guest entry and an invalid
> cached entry and returns without re-enabling the IOMMU memory region.
> Subsequent DMA therefore continues to bypass translation.
> 
> The issue can be reproduced with an emulated device in scalable mode
> with caching-mode=off:
> 
> 1. Unbind the guest device driver and change the group's type to
>     identity through /sys/kernel/iommu_groups/<group>/type.
> 2. Rebind the driver and issue a DMA write.
> 3. Unbind the driver, change the group's type back to DMA-FQ, and
>     rebind the driver.
> 4. Issue another DMA write using an IOVA mapped to a different GPA.
> 
> The second write incorrectly bypasses translation instead of reaching
> the mapped GPA.
> 
> Save the PASID entry when enabling the scalable-mode PT fast path so
> teardown invalidation can restore the IOMMU address space. Return after
> handling a non-present entry, and refresh the cached entry when a present
> entry changes, pass vtd_as to vtd_try_enable_pt_fast_path() instead of
> pick it from PASID 0.
> 
> Opportunistically clarify the invalidation modes in the fast-path comment
> and merge trace event vtd_translate_pt and vtd_pt_enable_fast_path into
> vtd_fast_path_translate_pt.
> 
> Fixes: c9cf33075a4c ("intel_iommu: Fix DMA failure when guest switches IOMMU domain")
> Reported-by: Clement Mathieu--Drif <clement.mathieu--drif@bull.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Tested-by: Clement Mathieu--Drif <clement.mathieu--drif@bull.com>
> ---
> v3:
> - introduce a new helper vtd_try_enable_pt_fast_path (Clement)
> v2:
> - move "VTDPASIDCacheEntry *pc_entry" in the branch it is used (Clement)

LGTM.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>   hw/i386/intel_iommu.c | 62 ++++++++++++++++++++++++-------------------
>   hw/i386/trace-events  |  3 +--
>   2 files changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b7da5d086d..a5ff89a11a 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1649,7 +1649,7 @@ static int vtd_address_space_sync(VTDAddressSpace *vtd_as)
>    * or second stage translation, it depends on PGTT setting.
>    */
>   static bool vtd_dev_pt_enabled(IntelIOMMUState *s, VTDContextEntry *ce,
> -                               uint32_t pasid)
> +                               uint32_t pasid, VTDPASIDEntry *pasid_entry)
>   {
>       VTDPASIDEntry pe;
>       int ret;
> @@ -1663,6 +1663,11 @@ static bool vtd_dev_pt_enabled(IntelIOMMUState *s, VTDContextEntry *ce,
>                */
>               return false;
>           }
> +
> +        if (pasid_entry) {
> +            *pasid_entry = pe;
> +        }
> +
>           return vtd_pe_pgtt_is_pt(&pe);
>       }
>   
> @@ -1689,7 +1694,7 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as)
>           return false;
>       }
>   
> -    return vtd_dev_pt_enabled(s, &ce, as->pasid);
> +    return vtd_dev_pt_enabled(s, &ce, as->pasid, NULL);
>   }
>   
>   /* Return whether the device is using IOMMU translation. */
> @@ -1701,8 +1706,8 @@ static bool vtd_switch_address_space(VTDAddressSpace *as)
>       assert(as);
>   
>       s = as->iommu_state;
> -    use_iommu = s->dmar_enabled && !vtd_as_pt_enabled(as);
>       pt = s->dmar_enabled && vtd_as_pt_enabled(as);
> +    use_iommu = s->dmar_enabled && !pt;
>   
>       /*
>        * When guest enables scalable mode and sets up first stage page table,
> @@ -1887,23 +1892,34 @@ VTDAddressSpace *vtd_get_as_by_sid(IntelIOMMUState *s, uint16_t sid)
>       return vtd_get_as_by_sid_and_pasid(s, sid, IOMMU_NO_PASID);
>   }
>   
> -static void vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)
> +static bool vtd_try_enable_pt_fast_path(IntelIOMMUState *s,
> +                                        VTDAddressSpace *vtd_as,
> +                                        VTDContextEntry *ce, uint32_t pasid)
>   {
> -    VTDAddressSpace *vtd_as;
> -    bool success = false;
> +    VTDPASIDEntry pe;
>   
> -    vtd_as = vtd_get_as_by_sid(s, source_id);
> -    if (!vtd_as) {
> -        goto out;
> +    if (!vtd_dev_pt_enabled(s, ce, pasid, &pe)) {
> +        return false;
>       }
>   
> -    if (vtd_switch_address_space(vtd_as) == false) {
> -        /* We switched off IOMMU region successfully. */
> -        success = true;
> +    /*
> +     * When this happens, it means firstly caching-mode is not
> +     * enabled, and this is the first passthrough translation for
> +     * the device. Let's enable the fast path for passthrough.
> +     *
> +     * When passthrough is disabled again for the device, we can
> +     * capture it via context-cache invalidation in legacy mode or
> +     * PASID-cache invalidation in scalable mode, then the IOMMU
> +     * region can be swapped back.
> +     */
> +    if (!vtd_switch_address_space(vtd_as) && s->root_scalable) {
> +        VTDPASIDCacheEntry *pc_entry = &vtd_as->pasid_cache_entry;
> +
> +        pc_entry->pasid_entry = pe;
> +        pc_entry->valid = true;
>       }
>   
> -out:
> -    trace_vtd_pt_enable_fast_path(source_id, success);
> +    return true;
>   }
>   
>   /*
> @@ -2167,23 +2183,12 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>        * We don't need to translate for pass-through context entries.
>        * Also, let's ignore IOTLB caching as well for PT devices.
>        */
> -    if (vtd_dev_pt_enabled(s, &ce, pasid)) {
> +    if (vtd_try_enable_pt_fast_path(s, vtd_as, &ce, pasid)) {
>           entry->iova = addr & VTD_PAGE_MASK_4K;
>           entry->translated_addr = entry->iova;
>           entry->addr_mask = ~VTD_PAGE_MASK_4K;
>           entry->perm = IOMMU_RW;
> -        trace_vtd_translate_pt(source_id, entry->iova);
> -
> -        /*
> -         * When this happens, it means firstly caching-mode is not
> -         * enabled, and this is the first passthrough translation for
> -         * the device. Let's enable the fast path for passthrough.
> -         *
> -         * When passthrough is disabled again for the device, we can
> -         * capture it via the context entry invalidation, then the
> -         * IOMMU region can be swapped back.
> -         */
> -        vtd_pt_enable_fast_path(s, source_id);
> +        trace_vtd_fast_path_translate_pt(source_id, entry->iova);
>           vtd_iommu_unlock(s);
>           return true;
>       }
> @@ -3133,6 +3138,7 @@ static void vtd_pasid_cache_sync_locked(gpointer key, gpointer value,
>               vtd_address_space_unmap(vtd_as, n);
>           }
>           vtd_switch_address_space(vtd_as);
> +        return;
>       }
>   
>       /*
> @@ -3158,11 +3164,11 @@ static void vtd_pasid_cache_sync_locked(gpointer key, gpointer value,
>       }
>   
>       if (!pc_entry->valid) {
> -        pc_entry->pasid_entry = pe;
>           pc_entry->valid = true;
>       } else if (!vtd_pasid_entry_compare(&pe, &pc_entry->pasid_entry)) {
>           return;
>       }
> +    pc_entry->pasid_entry = pe;
>   
>       vtd_switch_address_space(vtd_as);
>       vtd_address_space_sync(vtd_as);
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 7bf4cddd38..3337a7e19c 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -47,8 +47,7 @@ vtd_page_walk_skip_read(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"P
>   vtd_page_walk_skip_reserve(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"PRIx64" - 0x%"PRIx64" due to rsrv set"
>   vtd_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, bool on) "Device %02x:%02x.%x switching address space (iommu enabled=%d)"
>   vtd_as_unmap_whole(uint8_t bus, uint8_t slot, uint8_t fn, uint64_t iova, uint64_t size) "Device %02x:%02x.%x start 0x%"PRIx64" size 0x%"PRIx64
> -vtd_translate_pt(uint16_t sid, uint64_t addr) "source id 0x%"PRIx16", iova 0x%"PRIx64
> -vtd_pt_enable_fast_path(uint16_t sid, bool success) "sid 0x%"PRIx16" %d"
> +vtd_fast_path_translate_pt(uint16_t sid, uint64_t addr) "source id 0x%"PRIx16", iova 0x%"PRIx64
>   vtd_irq_generate(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64
>   vtd_reg_read(uint64_t addr, uint64_t size) "addr 0x%"PRIx64" size 0x%"PRIx64
>   vtd_reg_write(uint64_t addr, uint64_t size, uint64_t val) "addr 0x%"PRIx64" size 0x%"PRIx64" value 0x%"PRIx64