Re: [PATCH] xen/arm: smmuv3: invalidate TLB when freeing a VMID

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Leonid,

Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:

> arm_smmu_domain_free() releases the VMID without invalidating the SMMU
> TLB entries tagged with it. The VMID allocator (arm_smmu_bitmap_alloc)
> always returns the first free VMID, so a new domain usually gets the
> same VMID as the domain that was just destroyed. As a result, DMA from
> a device assigned to the new domain can use stale TLB entries and access
> memory of the old domain, which may already be in use by Xen or by
> another domain.
>
> In Linux, this invalidation is done by free_io_pgtable_ops(), which
> calls the tlb_flush_all callback (arm_smmu_tlb_inv_context()) before
> the VMID is released. The call was dropped when the io-pgtable code
> was removed during the port to Xen, and nothing replaced it.
>
> Linux guests that modify their P2M early in boot (e.g. by mapping
> shared_info in the xen_guest_init initcall) hide the problem, because
> the resulting IOTLB flush clears the stale entries. However, guests
> without Xen support (e.g. Linux with CONFIG_XEN disabled) do not modify
> the P2M early in boot, so the device keeps using stale SMMU TLB entries.
> This was observed as GPU rendering corruption after "xl destroy" and "xl
> create" of the same guest with CONFIG_XEN disabled.
>
> To fix the problem, issue a TLB invalidation for the VMID before
> releasing it.
>
> Fixes: b3be6ffb009c ("xen/arm: smmuv3: Remove Linux specific code that is not 
> usable in XEN")
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
>  xen/drivers/passthrough/arm/smmu-v3.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index bf153227dbd..93176e2fb21 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1153,8 +1153,14 @@ static void arm_smmu_domain_free(struct iommu_domain 
> *domain)
>       struct arm_smmu_device *smmu = smmu_domain->smmu;
>       struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
>  
> -     if (cfg->vmid)
> +     if (cfg->vmid) {
> +             /*
> +              * Invalidate all entries tagged with this VMID
> +              * before it can be handed to another domain.
> +              */
> +             arm_smmu_tlb_inv_context(smmu_domain);
>               arm_smmu_bitmap_free(smmu->vmid_map, cfg->vmid);
> +     }
>  
>       xfree(smmu_domain);
>  }

-- 
WBR, Volodymyr