RE: [PATCH for-4.15 v5 2/3] xen/x86: iommu: Ignore IOMMU mapping requests when a domain is dying

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Julien Grall <julien@xxxxxxx>
> Sent: Friday, February 26, 2021 6:57 PM
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> The new x86 IOMMU page-tables allocator will release the pages when
> relinquishing the domain resources. However, this is not sufficient
> when the domain is dying because nothing prevents page-table to be
> allocated.
> 
> As the domain is dying, it is not necessary to continue to modify the
> IOMMU page-tables as they are going to be destroyed soon.
> 
> At the moment, page-table allocates will only happen when iommu_map().
> So after this change there will be no more page-table allocation
> happening because we don't use superpage mappings yet when not sharing
> page tables.
> 
> In order to observe d->is_dying correctly, we need to rely on per-arch
> locking, so the check to ignore IOMMU mapping is added on the per-driver
> map_page() callback.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> ---
> 
> As discussed in v3, this is only covering 4.15. We can discuss
> post-4.15 how to catch page-table allocations if another caller (e.g.
> iommu_unmap() if we ever decide to support superpages) start to use the
> page-table allocator.
> 
> Changes in v5:
>     - Clarify in the commit message why fixing iommu_map() is enough
>     - Split "if ( !is_iommu_enabled(d) )"  in a separate patch
>     - Update the comment on top of the spin_barrier()
> 
> Changes in v4:
>     - Move the patch to the top of the queue
>     - Reword the commit message
> 
> Changes in v3:
>     - Patch added. This is a replacement of "xen/iommu: iommu_map: Don't
>     crash the domain if it is dying"
> ---
>  xen/drivers/passthrough/amd/iommu_map.c | 12 ++++++++++++
>  xen/drivers/passthrough/vtd/iommu.c     | 12 ++++++++++++
>  xen/drivers/passthrough/x86/iommu.c     |  3 +++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> index d3a8b1aec766..560af54b765b 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -285,6 +285,18 @@ int amd_iommu_map_page(struct domain *d, dfn_t
> dfn, mfn_t mfn,
> 
>      spin_lock(&hd->arch.mapping_lock);
> 
> +    /*
> +     * IOMMU mapping request can be safely ignored when the domain is
> dying.
> +     *
> +     * hd->arch.mapping_lock guarantees that d->is_dying will be observed
> +     * before any page tables are freed (see iommu_free_pgtables()).
> +     */
> +    if ( d->is_dying )
> +    {
> +        spin_unlock(&hd->arch.mapping_lock);
> +        return 0;
> +    }
> +
>      rc = amd_iommu_alloc_root(d);
>      if ( rc )
>      {
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index d136fe36883b..b549a71530d5 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1762,6 +1762,18 @@ static int __must_check
> intel_iommu_map_page(struct domain *d, dfn_t dfn,
> 
>      spin_lock(&hd->arch.mapping_lock);
> 
> +    /*
> +     * IOMMU mapping request can be safely ignored when the domain is
> dying.
> +     *
> +     * hd->arch.mapping_lock guarantees that d->is_dying will be observed
> +     * before any page tables are freed (see iommu_free_pgtables())
> +     */
> +    if ( d->is_dying )
> +    {
> +        spin_unlock(&hd->arch.mapping_lock);
> +        return 0;
> +    }
> +
>      pg_maddr = addr_to_dma_page_maddr(d, dfn_to_daddr(dfn), 1);
>      if ( !pg_maddr )
>      {
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index 58a330e82247..ad19b7dd461c 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -270,6 +270,9 @@ int iommu_free_pgtables(struct domain *d)
>      if ( !is_iommu_enabled(d) )
>          return 0;
> 
> +    /* After this barrier, no new IOMMU mappings can be inserted. */
> +    spin_barrier(&hd->arch.mapping_lock);
> +
>      while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) )
>      {
>          free_domheap_page(pg);
> --
> 2.17.1