Re: [Xen-devel] [PATCH v8 4/4] xen/arm: grant: Add another entry to map MFN 1:1 in dom0 p2m

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-05-19 at 17:24 +0100, Julien Grall wrote:
> Grant mapping can be used for DMA request. The dev_bus_addr returned by the

                                             ^Currently (?)

> hypercall is the MFN (not the IPA). Currently Linux is using this address (via
> swiotlb) to program the DMA.

Rather than talking specifically about Linux and swiotlb I think it is
correct to say "Guests expect to be able to use the returned address for
DMA".

> When the device is protected by IOMMU the request will fail. We have to
> add 1:1 mapping in the domain p2m to allow DMA request working.
> 
> This is valid because DOM0 has its memory mapped 1:1 and therefore we know
> that RAM and devices cannot clash.

Is it worth mentioning now that in the future when a domain only has
access to protected I/O devices we would instead return
dev_bus_addr==IPA and intend to drop this extra 1:1 mapping?

> The grant mapping code already handle this case for x86 PV guests. Reuse the
> same code path for ARM guest.

In particular do you mean that iommu_{,un}map_page handles the reference
counting needed when an mfn is mapped via multiple grant mapping? I
think it must be the callers of those functions. Could you say that here
please?

> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 21b4572..9f85800 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1536,6 +1536,48 @@ static void arm_smmu_iommu_domain_teardown(struct 
> domain *d)
>      xfree(smmu_domain);
>  }
>  
> +static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
> +                             unsigned long mfn, unsigned int flags)
> +{
> +    p2m_type_t t;
> +
> +    /* This function should only be used by gnttab code when the domain
> +     * is direct mapped and gfn == mfn.

Is gfn !+ mfn an ASSERT-worthy condition?

Is gnttab the only possible user?

> +     */
> +    if ( !is_domain_direct_mapped(d) || gfn != mfn )
> +        return -EINVAL;
> +
> +    /* We only support readable and writable flags */
> +    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
> +        return -EINVAL;
> +
> +    /* The function guest_physmap_add_entry replace the current mapping

"replaces"

> +     * if there is already one...

... I feel like you intended to describe a consequence of that here. I
can't see the relationship between the comment and the selection of rw
vs ro mappings.

> +     */
> +    t = (flags & IOMMUF_writable)? p2m_iommu_map_rw : p2m_iommu_map_ro;
> +
> +    /* Grant mapping can be used for DMA request. The dev_bus_addr returned 
> by
> +     * the hypercall is the MFN (not the IPA). For device protected by

"Grant mappings... DMA requests... For devices" (all plural)

> +     * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
> +     * allow DMA request working.

"to allow DMA requests to work"

> +     * This is only valid when the domain is directed mapped
> +     */
> +    return guest_physmap_add_entry(d, gfn, mfn, 0, t);
> +}
> +
> +static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
> +{
> +    /* This function should only be used by gnttab code when the domain
> +     * is direct mapped
> +     */
> +    if ( !is_domain_direct_mapped(d) )
> +        return -EINVAL;
> +
> +    guest_physmap_remove_page(d, gfn, gfn, 0);

I think 0 here is really PAGE_ORDER_4K, is it? (other callers of this
function seem to be inconsistent about this)

> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index bd71abe..b68d5b8 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -45,6 +45,8 @@ typedef enum {
>      p2m_map_foreign,    /* Ram pages from foreign domain */
>      p2m_grant_map_rw,   /* Read/write grant mapping */
>      p2m_grant_map_ro,   /* Read-only grant mapping */
> +    p2m_iommu_map_rw,   /* Read/write iommu mapping */
> +    p2m_iommu_map_ro,   /* Read-only iommu mapping */

Why do we need new p2m types, rather than using e.g. the grant map type
(which I suppose is what the non-1:1 map uses)?

Could you explain the reason in the commit log too please.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel