Hello Julien,
> On 14 Feb 2021, at 2:35 pm, Julien Grall <jul...@xen.org> wrote:
>
> From: Julien Grall <jgr...@amazon.com>
>
> ... are the same.
>
> When the IOMMU is enabled and the domain is direct mapped (e.g. Dom0),
> Xen will insert a 1:1 mapping for each grant mapping in the P2M to
> allow DMA.
>
> This works quite well when the grantee and granter and
/s/and/are
> not the same
> because the GFN in the P2M should not be mapped. However, if they are
> the same, we will overwrite the mapping. Worse, it will be completely
> removed when the grant is unmapped.
>
> As the domain is direct mapped, a 1:1 mapping should always present in
> the P2M. This is not 100% guaranteed if the domain decides to mess with
> the P2M. However, such domain would already end up in trouble as the
> page would be soon be freed (when the last reference dropped).
>
> Add an additional check in arm_iommu_{,un}map_page() to check whether
> the page belongs to the domain. If it is belongs to it, then ignore the
> request.
>
> Note that we don't need to hold an extra reference on the page because
> the grant code will already do it for us.
>
> Reported-by: Rahul Singh <rahul.si...@arm.com>
> Fixes: 552710b38863 ("xen/arm: grant: Add another entry to map MFN 1:1 in
> dom0 p2m")
> Signed-off-by: Julien Grall <jgr...@amazon.com>
With the typo fixed.
Reviewed-by: Rahul Singh <rahul.si...@arm.com>
Tested-by: Rahul Singh <rahul.si...@arm.com>
Regards,
Rahul
>
> ---
>
> This is a candidate for 4.15. Without the patch, it would not be
> possible to get the frontend and backend in the same domain (useful when
> trying to map the guest block device in dom0).
> ---
> xen/drivers/passthrough/arm/iommu_helpers.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c
> b/xen/drivers/passthrough/arm/iommu_helpers.c
> index a36e2b8e6c42..35a813308b8c 100644
> --- a/xen/drivers/passthrough/arm/iommu_helpers.c
> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
> @@ -53,6 +53,17 @@ int __must_check arm_iommu_map_page(struct domain *d,
> dfn_t dfn, mfn_t mfn,
>
> t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
>
> + /*
> + * The granter and grantee can be the same domain. In normal
> + * condition, the 1:1 mapping should already present in the P2M so
> + * we want to avoid overwriting it.
> + *
> + * Note that there is no guarantee the 1:1 mapping will be present
> + * if the domain decides to mess with the P2M.
> + */
> + if ( page_get_owner(mfn_to_page(mfn)) == d )
> + return 0;
> +
> /*
> * The function guest_physmap_add_entry replaces the current mapping
> * if there is already one...
> @@ -71,6 +82,13 @@ int __must_check arm_iommu_unmap_page(struct domain *d,
> dfn_t dfn,
> if ( !is_domain_direct_mapped(d) )
> return -EINVAL;
>
> + /*
> + * When the granter and grantee are the same, we didn't insert a
> + * mapping. So ignore the unmap request.
> + */
> + if ( page_get_owner(mfn_to_page(_mfn(dfn_x(dfn)))) == d )
> + return 0;
> +
> return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
> 0);
> }
>
> --
> 2.17.1
>
