Re: [RFC PATCH 1/2] xen: introduce arch_iommu_grant_(un)map_page

[Julien Grall <julien.grall@linaro.org>]

Hi Stefano,

You forgot to add Jan Beulich for the generic IOMMU part.

Regards,

On 07/08/2014 04:53 PM, Stefano Stabellini wrote:
> From: Julien Grall <julien.grall@citrix.com>
> 
> From: Julien Grall <julien.grall@citrix.com>
> 
> Introduce two arch specific iommu grant mapping and unmapping functions,
> they are called from __gnttab_map_grant_ref and __gnttab_unmap_common.
> 
> The x86 implementation simply calls iommu_(un)map_page.
> The ARM implementation is based on arm_smmu_(un)map_page.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/common/grant_table.c            |   10 +++----
>  xen/drivers/passthrough/arm/iommu.c |   49 +++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/arm/smmu.c  |   42 ------------------------------
>  xen/drivers/passthrough/x86/iommu.c |   11 ++++++++
>  xen/include/xen/iommu.h             |    4 +++
>  5 files changed, 69 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 464007e..6ea1c2f 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -738,13 +738,13 @@ __gnttab_map_grant_ref(
>               !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
>          {
>              if ( wrc == 0 )
> -                err = iommu_map_page(ld, frame, frame,
> -                                     IOMMUF_readable|IOMMUF_writable);
> +                err = arch_iommu_grant_map_page(ld, frame,
> +                                                IOMMUF_readable|IOMMUF_writable);
>          }
>          else if ( act_pin && !old_pin )
>          {
>              if ( (wrc + rdc) == 0 )
> -                err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
> +                err = arch_iommu_grant_map_page(ld, frame, IOMMUF_readable);
>          }
>          if ( err )
>          {
> @@ -941,9 +941,9 @@ __gnttab_unmap_common(
>          int err = 0;
>          mapcount(lgt, rd, op->frame, &wrc, &rdc);
>          if ( (wrc + rdc) == 0 )
> -            err = iommu_unmap_page(ld, op->frame);
> +            err = arch_iommu_grant_unmap_page(ld, op->frame);
>          else if ( wrc == 0 )
> -            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> +            err = arch_iommu_grant_map_page(ld, op->frame, IOMMUF_readable);
>          if ( err )
>          {
>              rc = GNTST_general_error;
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 3007b99..6460b7e 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -18,6 +18,8 @@
>  #include <xen/lib.h>
>  #include <xen/iommu.h>
>  #include <xen/device_tree.h>
> +#include <xen/sched.h>
> +#include <asm/p2m.h>
>  #include <asm/device.h>
>  
>  static const struct iommu_ops *iommu_ops;
> @@ -68,3 +70,50 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>      iommu_dt_domain_destroy(d);
>  }
> +
> +int arch_iommu_grant_map_page(struct domain *d, unsigned long frame,
> +                              unsigned flags)
> +{
> +    p2m_type_t t;
> +
> +    /* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
> +     * the hypercall is the MFN (not the IPA). For device protected by
> +     * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
> +     * allow DMA request to work.
> +     * This is only valid when the domain is directed mapped.
> +     */
> +    BUG_ON(!is_domain_direct_mapped(d));
> +
> +    /* We only support readable and writable flags */
> +    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
> +        return -EINVAL;
> +
> +    t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
> +
> +    /* The function guest_physmap_add_entry replaces the current mapping
> +     * if there is already one...
> +     */
> +    return guest_physmap_add_entry(d, frame, frame, 0, t);
> +}
> +
> +int arch_iommu_grant_unmap_page(struct domain *d, unsigned long frame)
> +{
> +    /* 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, frame, frame, 0);
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index f4eb2a2..21b4572 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1536,46 +1536,6 @@ 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;
> -
> -    /* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
> -     * the hypercall is the MFN (not the IPA). For device protected by
> -     * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
> -     * allow DMA request to work.
> -     * This is only valid when the domain is directed mapped. Hence this
> -     * function should only be used by gnttab code with gfn == mfn.
> -     */
> -    BUG_ON(!is_domain_direct_mapped(d));
> -    BUG_ON(mfn != gfn);
> -
> -    /* We only support readable and writable flags */
> -    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
> -        return -EINVAL;
> -
> -    t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
> -
> -    /* The function guest_physmap_add_entry replaces the current mapping
> -     * if there is already one...
> -     */
> -    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);
> -
> -    return 0;
> -}
> -
>  static const struct iommu_ops arm_smmu_iommu_ops = {
>      .init = arm_smmu_iommu_domain_init,
>      .hwdom_init = arm_smmu_iommu_hwdom_init,
> @@ -1584,8 +1544,6 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>      .iotlb_flush_all = arm_smmu_iotlb_flush_all,
>      .assign_dt_device = arm_smmu_attach_dev,
>      .reassign_dt_device = arm_smmu_reassign_dt_dev,
> -    .map_page = arm_smmu_map_page,
> -    .unmap_page = arm_smmu_unmap_page,
>  };
>  
>  static int __init smmu_init(struct dt_device_node *dev,
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index ce0ca5a..a88626a 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -135,6 +135,17 @@ void arch_iommu_domain_destroy(struct domain *d)
>      }
>  }
>  
> +int arch_iommu_grant_map_page(struct domain *d, unsigned long frame,
> +                              unsigned flags)
> +{
> +    return iommu_map_page(d, frame, frame, flags);
> +}
> +
> +int arch_iommu_grant_unmap_page(struct domain *d, unsigned long frame)
> +{
> +    return iommu_unmap_page(d, frame);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 8eb764a..0dd6987 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -76,6 +76,10 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
>                     unsigned int flags);
>  int iommu_unmap_page(struct domain *d, unsigned long gfn);
>  
> +int arch_iommu_grant_map_page(struct domain *d, unsigned long frame,
> +                              unsigned flags);
> +int arch_iommu_grant_unmap_page(struct domain *d, unsigned long frame);
> +
>  enum iommu_feature
>  {
>      IOMMU_FEAT_COHERENT_WALK,
> 


-- 
Julien Grall