[RFC PATCH 0/2] map grant refs at pfn = mfn

[RFC PATCH 0/2] map grant refs at pfn = mfn

[Stefano Stabellini]

Hi all,
this patch series introduces a second p2m mapping of grant reference on
ARM at guest physical address == machine address of the grant ref.  It
is safe because dom0 is already mapped 1:1. We export
XENFEAT_grant_map_11 to signal the guest that this second p2m mapping is
available.

One reason for wanting the second p2m mapping is to avoid tracking mfn
to pfn mappings in the guest kernel. Since the same mfn can be granted
multiple times to the backend, finding the right pfn corresponding to a
given mfn can be difficult and expensive. Providing a second mapping at
a known address allow the kernel to access the page without knowing the
pfn.


Julien Grall (1):
      xen: introduce arch_iommu_grant_(un)map_page

Stefano Stabellini (1):
      xen/arm: introduce XENFEAT_grant_map_11

 xen/common/grant_table.c            |   10 +++----
 xen/common/kernel.c                 |    4 +++
 xen/drivers/passthrough/arm/iommu.c |   49 +++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/arm/smmu.c  |   42 ------------------------------
 xen/drivers/passthrough/x86/iommu.c |   11 ++++++++
 xen/include/asm-arm/grant_table.h   |    3 +--
 xen/include/public/features.h       |    3 +++
 xen/include/xen/iommu.h             |    4 +++
 8 files changed, 77 insertions(+), 49 deletions(-)

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

[Stefano Stabellini]

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,
-- 
1.7.10.4

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

[Julien Grall]

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

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

[Ian Campbell]

On Tue, 2014-07-08 at 16:53 +0100, Stefano Stabellini wrote:
> +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));

What stops a non-1:1 guest from trying to map a grant reference and
ending up here?

Ian.

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

[Julien Grall]

Hi Ian,

On 16/07/14 16:56, Ian Campbell wrote:
> On Tue, 2014-07-08 at 16:53 +0100, Stefano Stabellini wrote:
>> +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));
>
> What stops a non-1:1 guest from trying to map a grant reference and
> ending up here?

The function is protected by gnttab_need_iommu_mapping which check if 
the domain use direct mapping for the memory.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 08.07.14 at 17:53, <stefano.stabellini@eu.citrix.com> wrote:
> --- 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);

This (and further similar code) sitting in (from an x86 perspective)
PV code path makes the naming together with the hiding of the
1:1-ness quite undesirable. I.e. without enough care one may be
tempted to call this seemingly generic function from a non-PV
context.

> --- 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);
> +}

Is there no way to make these simple wrappers inline functions?

Jan

[RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Stefano Stabellini]

The flag specifies that the hypervisor maps a grant page to guest
physical address == machine address of the page in addition to the
normal grant mapping address.

Frontends are allowed to map the same page multiple times using multiple
grant references. On the backend side it can be difficult to find out
the physical address corresponding to a particular machine address,
especially at the completation of a dma operation. To simplify address
translations, we introduce a second mapping of the grant at physical
address == machine address so that dom0 can issue cache maintenance
operations without having to find the pfn.

On ARM enable the flag and this behaviour by default if the domain is
directly mapped.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/common/kernel.c               |    4 ++++
 xen/include/asm-arm/grant_table.h |    3 +--
 xen/include/public/features.h     |    3 +++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 7e83353..556ad14 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -325,6 +325,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                 break;
             }
 #endif
+#ifdef CONFIG_ARM
+            if ( is_domain_direct_mapped(d) )
+                fi.submap |= 1U << XENFEAT_grant_map_11;
+#endif
             break;
         default:
             return -EINVAL;
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index eac8a70..91ba230 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -33,8 +33,7 @@ static inline int replace_grant_supported(void)
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
      (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
 
-#define gnttab_need_iommu_mapping(d)                    \
-    (is_domain_direct_mapped(d) && need_iommu(d))
+#define gnttab_need_iommu_mapping(d)            (is_domain_direct_mapped(d))
 
 #endif /* __ASM_GRANT_TABLE_H__ */
 /*
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index a149aa6..95b87fd 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -94,6 +94,9 @@
 /* operation as Dom0 is supported */
 #define XENFEAT_dom0                      11
 
+/* Xen also maps grant references at pfn = mfn */
+#define XENFEAT_grant_map_11              12
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
1.7.10.4

Re: [RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Ian Campbell]

On Tue, 2014-07-08 at 16:53 +0100, Stefano Stabellini wrote:
> +/* Xen also maps grant references at pfn = mfn */
> +#define XENFEAT_grant_map_11              12

David also mentioned this but "11" is not a very good rendering of 1:1.
I know we use this internally within both Xen and Linux but perhaps at
the hypercall API layer we could use "identity" or something instead?

It might be worth trying to include a word which implies "in addition to
the mapping you asked for" as well, for clarity.

> +
>  #define XENFEAT_NR_SUBMAPS 1
>  
>  #endif /* __XEN_PUBLIC_FEATURES_H__ */

Re: [RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Stefano Stabellini]

On Wed, 16 Jul 2014, Ian Campbell wrote:
> On Tue, 2014-07-08 at 16:53 +0100, Stefano Stabellini wrote:
> > +/* Xen also maps grant references at pfn = mfn */
> > +#define XENFEAT_grant_map_11              12
> 
> David also mentioned this but "11" is not a very good rendering of 1:1.
> I know we use this internally within both Xen and Linux but perhaps at
> the hypercall API layer we could use "identity" or something instead?

I am fine with XENFEAT_grant_map_identity.
I'll wait for Jan's review before reposting.


> It might be worth trying to include a word which implies "in addition to
> the mapping you asked for" as well, for clarity.

Re: [RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Jan Beulich]

>>> On 08.07.14 at 17:53, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -325,6 +325,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>                  break;
>              }
>  #endif
> +#ifdef CONFIG_ARM
> +            if ( is_domain_direct_mapped(d) )

The #ifdef and if() seem kind of redundant - can't x86 simply get a
stub always returning false?

> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -33,8 +33,7 @@ static inline int replace_grant_supported(void)
>      ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
>       (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
>  
> -#define gnttab_need_iommu_mapping(d)                    \
> -    (is_domain_direct_mapped(d) && need_iommu(d))
> +#define gnttab_need_iommu_mapping(d)            (is_domain_direct_mapped(d))

How is this change related to the subject of the patch?

> --- a/xen/include/public/features.h
> +++ b/xen/include/public/features.h
> @@ -94,6 +94,9 @@
>  /* operation as Dom0 is supported */
>  #define XENFEAT_dom0                      11
>  
> +/* Xen also maps grant references at pfn = mfn */
> +#define XENFEAT_grant_map_11              12

As already said by others, this needs a better name.

Jan

Re: [RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Stefano Stabellini]

On Wed, 23 Jul 2014, Jan Beulich wrote:
> >>> On 08.07.14 at 17:53, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -325,6 +325,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >                  break;
> >              }
> >  #endif
> > +#ifdef CONFIG_ARM
> > +            if ( is_domain_direct_mapped(d) )
> 
> The #ifdef and if() seem kind of redundant - can't x86 simply get a
> stub always returning false?

Yes, you are right


> > --- a/xen/include/asm-arm/grant_table.h
> > +++ b/xen/include/asm-arm/grant_table.h
> > @@ -33,8 +33,7 @@ static inline int replace_grant_supported(void)
> >      ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
> >       (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
> >  
> > -#define gnttab_need_iommu_mapping(d)                    \
> > -    (is_domain_direct_mapped(d) && need_iommu(d))
> > +#define gnttab_need_iommu_mapping(d)            (is_domain_direct_mapped(d))
> 
> How is this change related to the subject of the patch?

This change is the one that actually enables the second mapping on ARM.
gnttab_need_iommu_mapping needs to return true for dom0, so that
arch_iommu_grant_map_page gets called by __gnttab_map_grant_ref.

I understand that actually XENFEAT_grant_map_11 doesn't have much to do
with IOMMUs or SMMUs, however it would still be nice to share the code
under:

if ( gnttab_need_iommu_mapping(ld) )
{
         
in __gnttab_map_grant_ref.
An alternative to this change that would also get rid of patch #1 of
this series could be something like:

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 464007e..e4e945a 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -727,7 +727,7 @@ __gnttab_map_grant_ref(
 
     double_gt_lock(lgt, rgt);
 
-    if ( gnttab_need_iommu_mapping(ld) )
+    if ( gnttab_need_iommu_mapping(ld) || is_domain_direct_mapped(ld) )
     {
         unsigned int wrc, rdc;
         int err = 0;
@@ -738,13 +738,23 @@ __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);
+            {
+                if ( gnttab_need_iommu_mapping(ld) )
+                    err = iommu_map_page(ld, frame, frame,
+                            IOMMUF_readable|IOMMUF_writable);
+                else
+                    err = arch_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);
+            {
+                if ( gnttab_need_iommu_mapping(ld) )
+                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
+                else
+                    err = arch_grant_map_page(ld, frame, IOMMUF_readable);
+            }
         }
         if ( err )
         {

do you think this would be better?


> > --- a/xen/include/public/features.h
> > +++ b/xen/include/public/features.h
> > @@ -94,6 +94,9 @@
> >  /* operation as Dom0 is supported */
> >  #define XENFEAT_dom0                      11
> >  
> > +/* Xen also maps grant references at pfn = mfn */
> > +#define XENFEAT_grant_map_11              12
> 
> As already said by others, this needs a better name.

I'll rename it to XENFEAT_grant_map_identity

Re: [RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Jan Beulich]

>>> On 23.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 23 Jul 2014, Jan Beulich wrote:
>> >>> On 08.07.14 at 17:53, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/xen/include/asm-arm/grant_table.h
>> > +++ b/xen/include/asm-arm/grant_table.h
>> > @@ -33,8 +33,7 @@ static inline int replace_grant_supported(void)
>> >      ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
>> >       (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
>> >  
>> > -#define gnttab_need_iommu_mapping(d)                    \
>> > -    (is_domain_direct_mapped(d) && need_iommu(d))
>> > +#define gnttab_need_iommu_mapping(d)            
> (is_domain_direct_mapped(d))
>> 
>> How is this change related to the subject of the patch?
> 
> This change is the one that actually enables the second mapping on ARM.
> gnttab_need_iommu_mapping needs to return true for dom0, so that
> arch_iommu_grant_map_page gets called by __gnttab_map_grant_ref.
> 
> I understand that actually XENFEAT_grant_map_11 doesn't have much to do
> with IOMMUs or SMMUs, however it would still be nice to share the code
> under:
> 
> if ( gnttab_need_iommu_mapping(ld) )
> {
>          
> in __gnttab_map_grant_ref.
> An alternative to this change that would also get rid of patch #1 of
> this series could be something like:
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 464007e..e4e945a 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -727,7 +727,7 @@ __gnttab_map_grant_ref(
>  
>      double_gt_lock(lgt, rgt);
>  
> -    if ( gnttab_need_iommu_mapping(ld) )
> +    if ( gnttab_need_iommu_mapping(ld) || is_domain_direct_mapped(ld) )
>      {
>          unsigned int wrc, rdc;
>          int err = 0;
> @@ -738,13 +738,23 @@ __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);
> +            {
> +                if ( gnttab_need_iommu_mapping(ld) )
> +                    err = iommu_map_page(ld, frame, frame,
> +                            IOMMUF_readable|IOMMUF_writable);
> +                else
> +                    err = arch_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);
> +            {
> +                if ( gnttab_need_iommu_mapping(ld) )
> +                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
> +                else
> +                    err = arch_grant_map_page(ld, frame, IOMMUF_readable);
> +            }
>          }
>          if ( err )
>          {
> 
> do you think this would be better?

Hmm, overall readability suffers, but at least it makes more obvious
what arch_grant_map_page() is about.

Jan

Re: [RFC PATCH 0/2] map grant refs at pfn = mfn

[Jan Beulich]

>>> On 08.07.14 at 17:52, <stefano.stabellini@eu.citrix.com> wrote:
> One reason for wanting the second p2m mapping is to avoid tracking mfn
> to pfn mappings in the guest kernel. Since the same mfn can be granted
> multiple times to the backend, finding the right pfn corresponding to a
> given mfn can be difficult and expensive. Providing a second mapping at
> a known address allow the kernel to access the page without knowing the
> pfn.

Isn't this just extending the 1:1 mapping hack, further special casing
handling on ARM rather than trying to use architecture independent
code in generic operations like grant (un)mapping as much as possible?

Jan