[PATCH v4 0/3] map grant refs at pfn = mfn

[PATCH v4 0/3] 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_identity 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.


Changes in v4:
- remove define is_domain_direct_mapped in common/memory.c;
- is_domain_direct_mapped as ((void)(d), 0);
- declare the x86 implementations of arch_grant_(un)map_page_identity as extern;
- add ASSERTs to ARM implementations of
  arch_grant_(un)map_page_identity;
- add XENFEAT_grant_map_identity if is_domain_direct_mapped;
- remove gnttab_need_identity_mapping, check is_domain_direct_mapped
  instead;
- define gnttab_need_iommu_mapping as is_domain_direct_mapped on arm.


Changes in v3:
- add "introduce is_domain_direct_mapped(d) as (0) on x86";
- use p2m_iommu types in arch_grant_(un)map_page_identity;
- call arch_grant_(un)map_page_identity functions from
arm_smmu_(un)map_page;
- introduce gnttab_need_identity_mapping;
- check gnttab_need_identity_mapping in __gnttab_map_grant_ref and
__gnttab_unmap_common.



Stefano Stabellini (3):
      xen/x86: introduce is_domain_direct_mapped(d) as ((void)(d), 0) on x86
      xen: introduce arch_grant_(un)map_page_identity
      xen/arm: introduce XENFEAT_grant_map_identity

 xen/arch/arm/p2m.c                 |   26 ++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c              |   13 +++++++++++++
 xen/common/grant_table.c           |   31 +++++++++++++++++++++++++------
 xen/common/kernel.c                |    2 ++
 xen/common/memory.c                |    4 ----
 xen/drivers/passthrough/arm/smmu.c |   13 ++-----------
 xen/include/asm-arm/grant_table.h  |    3 +--
 xen/include/asm-arm/p2m.h          |    4 ++++
 xen/include/asm-x86/domain.h       |    1 +
 xen/include/asm-x86/p2m.h          |    4 ++++
 xen/include/public/features.h      |    3 +++
 11 files changed, 81 insertions(+), 23 deletions(-)

[PATCH v4 1/3] xen/x86: introduce is_domain_direct_mapped(d) as ((void)(d), 0) on x86

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v4:
- remove define is_domain_direct_mapped in common/memory.c;
- define is_domain_direct_mapped as ((void)(d), 0)
---
 xen/common/memory.c          |    4 ----
 xen/include/asm-x86/domain.h |    1 +
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index c2dd31b..d69976f 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -29,10 +29,6 @@
 #include <xsm/xsm.h>
 #include <xen/trace.h>
 
-#ifndef is_domain_direct_mapped
-# define is_domain_direct_mapped(d) ((void)(d), 0)
-#endif
-
 struct memop_args {
     /* INPUT */
     struct domain *domain;     /* Domain to be affected. */
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index abf55fb..59a28ba 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -19,6 +19,7 @@
 #define is_hvm_pv_evtchn_domain(d) (has_hvm_container_domain(d) && \
         d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
 #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain))
+#define is_domain_direct_mapped(d) ((void)(d), 0)
 
 #define VCPU_TRAP_NMI          1
 #define VCPU_TRAP_MCE          2
-- 
1.7.10.4

Re: [PATCH v4 1/3] xen/x86: introduce is_domain_direct_mapped(d) as ((void)(d), 0) on x86

[Jan Beulich]

>>> On 28.07.14 at 12:16, <stefano.stabellini@eu.citrix.com> wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

> ---
> 
> Changes in v4:
> - remove define is_domain_direct_mapped in common/memory.c;
> - define is_domain_direct_mapped as ((void)(d), 0)
> ---
>  xen/common/memory.c          |    4 ----
>  xen/include/asm-x86/domain.h |    1 +
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index c2dd31b..d69976f 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -29,10 +29,6 @@
>  #include <xsm/xsm.h>
>  #include <xen/trace.h>
>  
> -#ifndef is_domain_direct_mapped
> -# define is_domain_direct_mapped(d) ((void)(d), 0)
> -#endif
> -
>  struct memop_args {
>      /* INPUT */
>      struct domain *domain;     /* Domain to be affected. */
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index abf55fb..59a28ba 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -19,6 +19,7 @@
>  #define is_hvm_pv_evtchn_domain(d) (has_hvm_container_domain(d) && \
>          d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
>  #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain))
> +#define is_domain_direct_mapped(d) ((void)(d), 0)
>  
>  #define VCPU_TRAP_NMI          1
>  #define VCPU_TRAP_MCE          2
> -- 
> 1.7.10.4

[PATCH v4 2/3] xen: introduce arch_grant_(un)map_page_identity

[Stefano Stabellini]

Introduce two arch specific functions to create a new p2m mapping of
granted pages at pfn == mfn.
The x86 implementation just returns ENOSYS.

Base the implementation of arm_smmu_(un)map_page on
arch_grant_(un)map_page_identity.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

---

Changes in v4:
- declare the x86 implementations as extern;
- add ASSERTs to ARM implementations.

Changes in v3:
- fix commit title;
- use p2m_iommu types;
- call arch_grant_(un)map_page_identity functions from
arm_smmu_(un)map_page.
---
 xen/arch/arm/p2m.c                 |   26 ++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c              |   13 +++++++++++++
 xen/drivers/passthrough/arm/smmu.c |   13 ++-----------
 xen/include/asm-arm/p2m.h          |    4 ++++
 xen/include/asm-x86/p2m.h          |    4 ++++
 5 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9960e17..c2634fc 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -555,6 +555,32 @@ void guest_physmap_remove_page(struct domain *d,
                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
 }
 
+int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
+                                 bool_t writeable)
+{
+    p2m_type_t t;
+
+    ASSERT(is_domain_direct_mapped(d));
+
+    /* This is not an IOMMU mapping but it is not a regular RAM p2m type
+     * either. We are using IOMMU p2m types here to prevent the pages
+     * from being used as grants. */
+    if ( writeable )
+        t = p2m_iommu_map_rw;
+    else
+        t = p2m_iommu_map_ro;
+
+    return guest_physmap_add_entry(d, frame, frame, 0, t);
+}
+
+int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
+{
+    ASSERT(is_domain_direct_mapped(d));
+
+    guest_physmap_remove_page(d, frame, frame, 0);
+    return 0;
+}
+
 int p2m_alloc_table(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 642ec28..c117d8d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1905,6 +1905,19 @@ out:
         rcu_unlock_domain(fdom);
     return rc;
 }
+
+
+int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
+                                 bool_t writeable)
+{
+    return -ENOSYS;
+}
+
+int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
+{
+    return -ENOSYS;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f4eb2a2..fb0c694 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1539,8 +1539,6 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
 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
@@ -1555,12 +1553,7 @@ static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
     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);
+    return arch_grant_map_page_identity(d, mfn, flags & IOMMUF_writable);
 }
 
 static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
@@ -1571,9 +1564,7 @@ static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
     if ( !is_domain_direct_mapped(d) )
         return -EINVAL;
 
-    guest_physmap_remove_page(d, gfn, gfn, 0);
-
-    return 0;
+    return arch_grant_unmap_page_identity(d, gfn);
 }
 
 static const struct iommu_ops arm_smmu_iommu_ops = {
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 911d32d..b682f51 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -181,6 +181,10 @@ static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
+int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
+                                 bool_t writeable);
+int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame);
+
 #endif /* _XEN_P2M_H */
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 0ddbadb..0cf03da 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -717,6 +717,10 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
 
     return flags;
 }
+ 
+int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
+                                 bool_t writeable);
+int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame);
 
 #endif /* _XEN_P2M_H */
 
-- 
1.7.10.4

Re: [PATCH v4 2/3] xen: introduce arch_grant_(un)map_page_identity

[Jan Beulich]

>>> On 28.07.14 at 12:16, <stefano.stabellini@eu.citrix.com> wrote:
> Changes in v4:
> - declare the x86 implementations as extern;

What you say here is what I asked for, but ...

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1905,6 +1905,19 @@ out:
>          rcu_unlock_domain(fdom);
>      return rc;
>  }
> +
> +
> +int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
> +                                 bool_t writeable)
> +{
> +    return -ENOSYS;
> +}
> +
> +int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
> +{
> +    return -ENOSYS;
> +}
> +

... what's this (not to speak of the double blank line you introduce)? I
specifically said declare but don't define, to catch problems at build
rather than run time.

Jan

Re: [PATCH v4 2/3] xen: introduce arch_grant_(un)map_page_identity

[Stefano Stabellini]

On Mon, 28 Jul 2014, Jan Beulich wrote:
> >>> On 28.07.14 at 12:16, <stefano.stabellini@eu.citrix.com> wrote:
> > Changes in v4:
> > - declare the x86 implementations as extern;
> 
> What you say here is what I asked for, but ...
> 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1905,6 +1905,19 @@ out:
> >          rcu_unlock_domain(fdom);
> >      return rc;
> >  }
> > +
> > +
> > +int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
> > +                                 bool_t writeable)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> > +int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> 
> ... what's this (not to speak of the double blank line you introduce)? I
> specifically said declare but don't define, to catch problems at build
> rather than run time.

I understand what you meant now. Clever. I'll do it next time.

[PATCH v4 3/3] xen/arm: introduce XENFEAT_grant_map_identity

[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.

Call arch_grant_map_page_identity and arch_grant_unmap_page_identity
from __gnttab_map_grant_ref and __gnttab_unmap_common to introduce the
second mapping if the domain is directly mapped. To do so we also need
to change gnttab_need_iommu_mapping to just be defined as
is_domain_direct_mapped on arm.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v4:
- add XENFEAT_grant_map_identity if is_domain_direct_mapped;
- remove gnttab_need_identity_mapping, check is_domain_direct_mapped
instead;
- define gnttab_need_iommu_mapping as is_domain_direct_mapped on arm.

Changes in v3:
- introduce gnttab_need_identity_mapping;
- check gnttab_need_identity_mapping in __gnttab_map_grant_ref and
__gnttab_unmap_common.

Changes in v2:
- rename XENFEAT_grant_map_11 to XENFEAT_grant_map_identity;
- remove superfluous ifdef CONFIG_ARM in xen/common/kernel.c;
- don't modify gnttab_need_iommu_mapping;
- call arch_grant_map_page_identity and arch_grant_unmap_page_identity
from grant_table functions.
---
 xen/common/grant_table.c          |   31 +++++++++++++++++++++++++------
 xen/common/kernel.c               |    2 ++
 xen/include/asm-arm/grant_table.h |    3 +--
 xen/include/public/features.h     |    3 +++
 4 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 464007e..68df49d 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -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 ( is_domain_direct_mapped(ld) )
+                    err = arch_grant_map_page_identity(ld, frame, 1);
+                else
+                    err = iommu_map_page(ld, frame, 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 ( is_domain_direct_mapped(ld) )
+                    err = arch_grant_map_page_identity(ld, frame, 0);
+                else
+                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
+            }
         }
         if ( err )
         {
@@ -941,9 +951,18 @@ __gnttab_unmap_common(
         int err = 0;
         mapcount(lgt, rd, op->frame, &wrc, &rdc);
         if ( (wrc + rdc) == 0 )
-            err = iommu_unmap_page(ld, op->frame);
-        else if ( wrc == 0 )
-            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
+        {
+            if ( is_domain_direct_mapped(ld) )
+                err = arch_grant_unmap_page_identity(ld, op->frame);
+            else
+                err = iommu_unmap_page(ld, op->frame);
+        } else if ( wrc == 0 )
+        {
+            if ( is_domain_direct_mapped(ld) )
+                err = arch_grant_map_page_identity(ld, op->frame, 0);
+            else
+                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
+        }
         if ( err )
         {
             rc = GNTST_general_error;
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 7e83353..fa40a36 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -325,6 +325,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                 break;
             }
 #endif
+            if ( is_domain_direct_mapped(d) )
+                fi.submap |= 1U << XENFEAT_grant_map_identity;
             break;
         default:
             return -EINVAL;
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index eac8a70..47147ce 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..ba753a0 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_identity              12
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
1.7.10.4

Re: [PATCH v4 3/3] xen/arm: introduce XENFEAT_grant_map_identity

[Jan Beulich]

>>> On 28.07.14 at 12:16, <stefano.stabellini@eu.citrix.com> wrote:
> @@ -941,9 +951,18 @@ __gnttab_unmap_common(
>          int err = 0;
>          mapcount(lgt, rd, op->frame, &wrc, &rdc);
>          if ( (wrc + rdc) == 0 )
> -            err = iommu_unmap_page(ld, op->frame);
> -        else if ( wrc == 0 )
> -            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> +        {
> +            if ( is_domain_direct_mapped(ld) )
> +                err = arch_grant_unmap_page_identity(ld, op->frame);
> +            else
> +                err = iommu_unmap_page(ld, op->frame);
> +        } else if ( wrc == 0 )

Coding style. With that taken care of:
Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v4 3/3] xen/arm: introduce XENFEAT_grant_map_identity

[Julien Grall]

Hi Stefano,

On 07/28/2014 11:16 AM, Stefano Stabellini wrote:
> 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

completion

[..]

>          if ( err )
>          {
> @@ -941,9 +951,18 @@ __gnttab_unmap_common(
>          int err = 0;
>          mapcount(lgt, rd, op->frame, &wrc, &rdc);
>          if ( (wrc + rdc) == 0 )
> -            err = iommu_unmap_page(ld, op->frame);
> -        else if ( wrc == 0 )
> -            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> +        {
> +            if ( is_domain_direct_mapped(ld) )
> +                err = arch_grant_unmap_page_identity(ld, op->frame);
> +            else
> +                err = iommu_unmap_page(ld, op->frame);
> +        } else if ( wrc == 0 )
> +        {
> +            if ( is_domain_direct_mapped(ld) )
> +                err = arch_grant_map_page_identity(ld, op->frame, 0);
> +            else
> +                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);

With this solution, iommu_map_page will never be called on ARM. It's
because gnttab_need_iommu_mapping is checking the domain is using direct
mapping.

So, I think you can drop the iommu_map_page callback of the SMMU in
patch #2.

Regards,

-- 
Julien Grall

Re: [PATCH v4 3/3] xen/arm: introduce XENFEAT_grant_map_identity

[Stefano Stabellini]

On Mon, 28 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/28/2014 11:16 AM, Stefano Stabellini wrote:
> > 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
> 
> completion
> 
> [..]
> 
> >          if ( err )
> >          {
> > @@ -941,9 +951,18 @@ __gnttab_unmap_common(
> >          int err = 0;
> >          mapcount(lgt, rd, op->frame, &wrc, &rdc);
> >          if ( (wrc + rdc) == 0 )
> > -            err = iommu_unmap_page(ld, op->frame);
> > -        else if ( wrc == 0 )
> > -            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> > +        {
> > +            if ( is_domain_direct_mapped(ld) )
> > +                err = arch_grant_unmap_page_identity(ld, op->frame);
> > +            else
> > +                err = iommu_unmap_page(ld, op->frame);
> > +        } else if ( wrc == 0 )
> > +        {
> > +            if ( is_domain_direct_mapped(ld) )
> > +                err = arch_grant_map_page_identity(ld, op->frame, 0);
> > +            else
> > +                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> 
> With this solution, iommu_map_page will never be called on ARM. It's
> because gnttab_need_iommu_mapping is checking the domain is using direct
> mapping.
> 
> So, I think you can drop the iommu_map_page callback of the SMMU in
> patch #2.

OK. Should I keep your acked-by on that patch?

Re: [PATCH v4 3/3] xen/arm: introduce XENFEAT_grant_map_identity

[Stefano Stabellini]

On Mon, 28 Jul 2014, Stefano Stabellini wrote:
> On Mon, 28 Jul 2014, Julien Grall wrote:
> > Hi Stefano,
> > 
> > On 07/28/2014 11:16 AM, Stefano Stabellini wrote:
> > > 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
> > 
> > completion
> > 
> > [..]
> > 
> > >          if ( err )
> > >          {
> > > @@ -941,9 +951,18 @@ __gnttab_unmap_common(
> > >          int err = 0;
> > >          mapcount(lgt, rd, op->frame, &wrc, &rdc);
> > >          if ( (wrc + rdc) == 0 )
> > > -            err = iommu_unmap_page(ld, op->frame);
> > > -        else if ( wrc == 0 )
> > > -            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> > > +        {
> > > +            if ( is_domain_direct_mapped(ld) )
> > > +                err = arch_grant_unmap_page_identity(ld, op->frame);
> > > +            else
> > > +                err = iommu_unmap_page(ld, op->frame);
> > > +        } else if ( wrc == 0 )
> > > +        {
> > > +            if ( is_domain_direct_mapped(ld) )
> > > +                err = arch_grant_map_page_identity(ld, op->frame, 0);
> > > +            else
> > > +                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
> > 
> > With this solution, iommu_map_page will never be called on ARM. It's
> > because gnttab_need_iommu_mapping is checking the domain is using direct
> > mapping.
> > 
> > So, I think you can drop the iommu_map_page callback of the SMMU in
> > patch #2.
> 
> OK. Should I keep your acked-by on that patch?

Actually it has to be done on this patch, otherwise the changes are not
bisectable.

Re: [PATCH v4 3/3] xen/arm: introduce XENFEAT_grant_map_identity

[Julien Grall]

On 07/28/2014 04:21 PM, Stefano Stabellini wrote:
> On Mon, 28 Jul 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 07/28/2014 11:16 AM, Stefano Stabellini wrote:
>>> 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
>>
>> completion
>>
>> [..]
>>
>>>          if ( err )
>>>          {
>>> @@ -941,9 +951,18 @@ __gnttab_unmap_common(
>>>          int err = 0;
>>>          mapcount(lgt, rd, op->frame, &wrc, &rdc);
>>>          if ( (wrc + rdc) == 0 )
>>> -            err = iommu_unmap_page(ld, op->frame);
>>> -        else if ( wrc == 0 )
>>> -            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
>>> +        {
>>> +            if ( is_domain_direct_mapped(ld) )
>>> +                err = arch_grant_unmap_page_identity(ld, op->frame);
>>> +            else
>>> +                err = iommu_unmap_page(ld, op->frame);
>>> +        } else if ( wrc == 0 )
>>> +        {
>>> +            if ( is_domain_direct_mapped(ld) )
>>> +                err = arch_grant_map_page_identity(ld, op->frame, 0);
>>> +            else
>>> +                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
>>
>> With this solution, iommu_map_page will never be called on ARM. It's
>> because gnttab_need_iommu_mapping is checking the domain is using direct
>> mapping.
>>
>> So, I think you can drop the iommu_map_page callback of the SMMU in
>> patch #2.
> 
> OK. Should I keep your acked-by on that patch?

Yes, I'm fine with one of 2 solutions (i.e either keep as it is or
removing the {,un}map_page callback).

Regards,

-- 
Julien Grall