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

[PATCH v2 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_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.



Stefano Stabellini (2):
      xen: introduce arch_iommu_grant_(un)map_page
      xen/arm: introduce XENFEAT_grant_map_identity

 xen/arch/arm/p2m.c            |   19 +++++++++++++++++++
 xen/common/grant_table.c      |   35 +++++++++++++++++++++++++++--------
 xen/common/kernel.c           |    2 ++
 xen/include/asm-arm/p2m.h     |    4 ++++
 xen/include/asm-x86/p2m.h     |   13 +++++++++++++
 xen/include/public/features.h |    3 +++
 6 files changed, 68 insertions(+), 8 deletions(-)

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

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
 xen/include/asm-arm/p2m.h |    4 ++++
 xen/include/asm-x86/p2m.h |   13 +++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9960e17..c38af59 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -555,6 +555,25 @@ 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;
+
+    if ( writeable )
+        t = p2m_ram_rw;
+    else
+        t = p2m_ram_ro;
+
+    return guest_physmap_add_entry(d, frame, frame, 0, t);
+}
+
+int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
+{
+    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/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..faead11 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -717,6 +717,19 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
 
     return flags;
 }
+ 
+static inline int arch_grant_map_page_identity(struct domain *d,
+                                               unsigned long frame,
+                                               bool_t writeable)
+{
+    return -ENOSYS;
+}
+
+static inline int arch_grant_unmap_page_identity(struct domain *d,
+                                                 unsigned long frame)
+{
+    return -ENOSYS;
+}
 
 #endif /* _XEN_P2M_H */
 
-- 
1.7.10.4

[PATCH v2 2/2] 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.

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

---

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      |   35 +++++++++++++++++++++++++++--------
 xen/common/kernel.c           |    2 ++
 xen/include/public/features.h |    3 +++
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 464007e..06cc88f 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_identity(ld, frame, true);
+            }
         }
         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_identity(ld, frame, false);
+            }
         }
         if ( err )
         {
@@ -935,15 +945,24 @@ __gnttab_unmap_common(
             act->pin -= GNTPIN_hstw_inc;
     }
 
-    if ( gnttab_need_iommu_mapping(ld) )
+    if ( gnttab_need_iommu_mapping(ld) || is_domain_direct_mapped(ld) )
     {
         unsigned int wrc, rdc;
         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 ( gnttab_need_iommu_mapping(ld) )
+                err = iommu_unmap_page(ld, op->frame);
+            else
+                err = arch_grant_unmap_page_identity(ld, op->frame);
+        } else if ( wrc == 0 )
+        {
+            if ( gnttab_need_iommu_mapping(ld) )
+                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
+            else
+                err = arch_grant_map_page_identity(ld, op->frame, false);
+        }
         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/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 v2 0/2] map grant refs at pfn = mfn

[Jan Beulich]

>>> On 23.07.14 at 19:18, <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.

Since you didn't respond to my earlier respective question: Is this
really the route you want to go? Bake the 1:1-ness into the kernel?
Iiuc this implies no backends using grants in other than Dom0... Or
else you still need to have the M->P tracking code in place, and I
would wonder whether bypassing it is really such a significant win.

Jan

Re: [PATCH v2 2/2] xen/arm: introduce XENFEAT_grant_map_identity

[Jan Beulich]

>>> On 23.07.14 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> Changes in v2:
> - rename XENFEAT_grant_map_11 to XENFEAT_grant_map_identity;
> - remove superfluous ifdef CONFIG_ARM in xen/common/kernel.c;

Looks like you forgot to add an is_domain_direct_mapped() stub for
x86, breaking the build there?

> @@ -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) )

This is bogus: On x86 the right part is always false, while on
ARM the right part is already part of gnttab_need_iommu_mapping().
IOW no need to change anything here afaict.

> @@ -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_identity(ld, frame, true);

Irrespective of the above this is inefficient too: If you used
!is_domain_direct_mapped() as the conditional here, the compiler
would be able to eliminate the arch_grant_map_page_identity()
path on x86, making it unnecessary to have the stub as an inline
function (a simple declaration without definition would then
suffice as long as we don't ever build with -Od, which we already
depend upon elsewhere).

Jan

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

[Tim Deegan]

At 18:18 +0100 on 23 Jul (1406135907), Stefano Stabellini wrote:
> 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.

Hrmn.  If your guest-kernel code relies on this new flag, you're 
effectively requiring that driver domains and middlebox VMs be
built 1:1 as well.  Unless they can deal with having a heavily
fragmented memory map that's going to be a problem.

Also it prevents dom0 mapping anything else into its p2m.  Unless dom0
knows where all the RAM in the system is (which is really none of its
business) it can't be sure that a GFN is safe to use for the _first_
p2m mapping, or any other p2m tricks it might like to play.

Tim.

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Tim Deegan wrote:
> At 18:18 +0100 on 23 Jul (1406135907), Stefano Stabellini wrote:
> > 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.
> 
> Hrmn.  If your guest-kernel code relies on this new flag, you're 
> effectively requiring that driver domains and middlebox VMs be
> built 1:1 as well.  Unless they can deal with having a heavily
> fragmented memory map that's going to be a problem.
> 
> Also it prevents dom0 mapping anything else into its p2m.  Unless dom0
> knows where all the RAM in the system is (which is really none of its
> business) it can't be sure that a GFN is safe to use for the _first_
> p2m mapping, or any other p2m tricks it might like to play.

Wait, this flag is only necessary when no SMMU is available. Or when at
least one DMA-capable device is not protected by an SMMU.
In this configuration, we cannot do driver domains and we are forced to
have the 1:1 in Dom0 and track p2m info for foreign grants in dom0.

On the other hand if all the DMA-capable devices are protected by an
SMMU, then we don't need the 1:1, we don't need this flags, we can have
driver domains, we don't have to track the p2m for foreign grants in
dom0.

Julien has a patch series to tell Dom0 whether it can rely on the SMMU
and therefore avoid p2m tracking and swiotlb-xen. If this "xen_dma_safe"
flag is present, dom0 would avoid the swiotlb-xen altogether, and
XENFEAT_grant_map_identity would be useless.

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

[Tim Deegan]

At 11:28 +0100 on 24 Jul (1406197711), Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Tim Deegan wrote:
> > At 18:18 +0100 on 23 Jul (1406135907), Stefano Stabellini wrote:
> > > 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.
> > 
> > Hrmn.  If your guest-kernel code relies on this new flag, you're 
> > effectively requiring that driver domains and middlebox VMs be
> > built 1:1 as well.  Unless they can deal with having a heavily
> > fragmented memory map that's going to be a problem.
> > 
> > Also it prevents dom0 mapping anything else into its p2m.  Unless dom0
> > knows where all the RAM in the system is (which is really none of its
> > business) it can't be sure that a GFN is safe to use for the _first_
> > p2m mapping, or any other p2m tricks it might like to play.
> 
> Wait, this flag is only necessary when no SMMU is available. Or when at
> least one DMA-capable device is not protected by an SMMU.
> In this configuration, we cannot do driver domains and we are forced to
> have the 1:1 in Dom0 and track p2m info for foreign grants in dom0.
> 
> On the other hand if all the DMA-capable devices are protected by an
> SMMU, then we don't need the 1:1, we don't need this flags, we can have
> driver domains, we don't have to track the p2m for foreign grants in
> dom0.
> 
> Julien has a patch series to tell Dom0 whether it can rely on the SMMU
> and therefore avoid p2m tracking and swiotlb-xen. If this "xen_dma_safe"
> flag is present, dom0 would avoid the swiotlb-xen altogether, and
> XENFEAT_grant_map_identity would be useless.

OK, makes sense. 

Cheers,

Tim

Re: [PATCH v2 2/2] xen/arm: introduce XENFEAT_grant_map_identity

[Stefano Stabellini]

On Thu, 24 Jul 2014, Jan Beulich wrote:
> >>> On 23.07.14 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> > Changes in v2:
> > - rename XENFEAT_grant_map_11 to XENFEAT_grant_map_identity;
> > - remove superfluous ifdef CONFIG_ARM in xen/common/kernel.c;
> 
> Looks like you forgot to add an is_domain_direct_mapped() stub for
> x86, breaking the build there?

Sorry, I assumed there was already one. I'll build test on x86 too next
time.


> > @@ -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) )
> 
> This is bogus: On x86 the right part is always false, while on
> ARM the right part is already part of gnttab_need_iommu_mapping().
> IOW no need to change anything here afaict.

On ARM gnttab_need_iommu_mapping expands to

#define gnttab_need_iommu_mapping(d)                    \
    (is_domain_direct_mapped(d) && need_iommu(d))

however we would need to do the second mapping even if need_iommu(d)
returns false. Changing gnttab_need_iommu_mapping(d) on ARM the way I
did in the previous version of the patch series seems wrong to me now,
because it is not an iommu mapping that gnttab needs, but a second
regular p2m mapping.

This is way I decided to add the "|| is_domain_direct_mapped(ld)".



> > @@ -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_identity(ld, frame, true);
> 
> Irrespective of the above this is inefficient too: If you used
> !is_domain_direct_mapped() as the conditional here, the compiler
> would be able to eliminate the arch_grant_map_page_identity()
> path on x86, making it unnecessary to have the stub as an inline
> function (a simple declaration without definition would then
> suffice as long as we don't ever build with -Od, which we already
> depend upon elsewhere).
 
It would be nicer the way you suggested, but on ARM a direct_mapped
domain could still need the iommu mapping. Conceptually we need to check
on gnttab_need_iommu_mapping.

Alternatively we could introduce a new macro:

gnttab_need_identity_mapping that is always (0) on x86.

That is probably the best option.

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

[Julien Grall]

Hi Stefano,

The Title of the commit message is now wrong.

On 23/07/14 18:19, Stefano Stabellini wrote:
> Introduce two arch specific functions to create a new p2m mapping of
> granted pages at pfn == mfn.
> The x86 implementation just returns ENOSYS.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
>   xen/include/asm-arm/p2m.h |    4 ++++
>   xen/include/asm-x86/p2m.h |   13 +++++++++++++
>   3 files changed, 36 insertions(+)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 9960e17..c38af59 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -555,6 +555,25 @@ 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;
> +
> +    if ( writeable )
> +        t = p2m_ram_rw;
> +    else
> +        t = p2m_ram_ro;

This is not the right p2m type to use here. p2m_ram_{rw,ro} allow 
foreign mapping. So another guest could access to the grant.

I would use p2m_iommu_map_{rw,ro}.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2014-07-24 at 11:28 +0100, Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Tim Deegan wrote:
> > At 18:18 +0100 on 23 Jul (1406135907), Stefano Stabellini wrote:
> > > 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.
> > 
> > Hrmn.  If your guest-kernel code relies on this new flag, you're 
> > effectively requiring that driver domains and middlebox VMs be
> > built 1:1 as well.  Unless they can deal with having a heavily
> > fragmented memory map that's going to be a problem.
> > 
> > Also it prevents dom0 mapping anything else into its p2m.  Unless dom0
> > knows where all the RAM in the system is (which is really none of its
> > business) it can't be sure that a GFN is safe to use for the _first_
> > p2m mapping, or any other p2m tricks it might like to play.
> 
> Wait, this flag is only necessary when no SMMU is available. Or when at
> least one DMA-capable device is not protected by an SMMU.
> In this configuration, we cannot do driver domains and we are forced to
> have the 1:1 in Dom0 and track p2m info for foreign grants in dom0.
> 
> On the other hand if all the DMA-capable devices are protected by an
> SMMU, then we don't need the 1:1, we don't need this flags, we can have
> driver domains, we don't have to track the p2m for foreign grants in
> dom0.

TO be pedantic I think you can have driver domain for devices which are
behind an SMMU even if other devices in the system aren't behind an
smmu.

> 
> Julien has a patch series to tell Dom0 whether it can rely on the SMMU
> and therefore avoid p2m tracking and swiotlb-xen. If this "xen_dma_safe"
> flag is present, dom0 would avoid the swiotlb-xen altogether, and
> XENFEAT_grant_map_identity would be useless.

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> The Title of the commit message is now wrong.

Thanks, I didn't notice.


> On 23/07/14 18:19, Stefano Stabellini wrote:
> > Introduce two arch specific functions to create a new p2m mapping of
> > granted pages at pfn == mfn.
> > The x86 implementation just returns ENOSYS.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >   xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
> >   xen/include/asm-arm/p2m.h |    4 ++++
> >   xen/include/asm-x86/p2m.h |   13 +++++++++++++
> >   3 files changed, 36 insertions(+)
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 9960e17..c38af59 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -555,6 +555,25 @@ 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;
> > +
> > +    if ( writeable )
> > +        t = p2m_ram_rw;
> > +    else
> > +        t = p2m_ram_ro;
> 
> This is not the right p2m type to use here. p2m_ram_{rw,ro} allow foreign
> mapping. So another guest could access to the grant.
>
> I would use p2m_iommu_map_{rw,ro}.


I see. I'll make the change and add a comment to explain why we are
using p2m_iommu types for non-iommu related mappings.

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

[Julien Grall]

On 24/07/14 12:07, Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> The Title of the commit message is now wrong.
>
> Thanks, I didn't notice.
>
>
>> On 23/07/14 18:19, Stefano Stabellini wrote:
>>> Introduce two arch specific functions to create a new p2m mapping of
>>> granted pages at pfn == mfn.
>>> The x86 implementation just returns ENOSYS.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> ---
>>>    xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
>>>    xen/include/asm-arm/p2m.h |    4 ++++
>>>    xen/include/asm-x86/p2m.h |   13 +++++++++++++
>>>    3 files changed, 36 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 9960e17..c38af59 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -555,6 +555,25 @@ 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;
>>> +
>>> +    if ( writeable )
>>> +        t = p2m_ram_rw;
>>> +    else
>>> +        t = p2m_ram_ro;
>>
>> This is not the right p2m type to use here. p2m_ram_{rw,ro} allow foreign
>> mapping. So another guest could access to the grant.
>>
>> I would use p2m_iommu_map_{rw,ro}.
>
>
> I see. I'll make the change and add a comment to explain why we are
> using p2m_iommu types for non-iommu related mappings.

The code to add the 1:1 mapping for the SMMU is very ugly. I was 
wondering if we could drop the iommu_map_page callback and
directly use arch_grant_map_page_identity in all the case.

It would avoid the if/else in the grant code.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> On 24/07/14 12:07, Stefano Stabellini wrote:
> > On Thu, 24 Jul 2014, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > The Title of the commit message is now wrong.
> > 
> > Thanks, I didn't notice.
> > 
> > 
> > > On 23/07/14 18:19, Stefano Stabellini wrote:
> > > > Introduce two arch specific functions to create a new p2m mapping of
> > > > granted pages at pfn == mfn.
> > > > The x86 implementation just returns ENOSYS.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > ---
> > > >    xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
> > > >    xen/include/asm-arm/p2m.h |    4 ++++
> > > >    xen/include/asm-x86/p2m.h |   13 +++++++++++++
> > > >    3 files changed, 36 insertions(+)
> > > > 
> > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > index 9960e17..c38af59 100644
> > > > --- a/xen/arch/arm/p2m.c
> > > > +++ b/xen/arch/arm/p2m.c
> > > > @@ -555,6 +555,25 @@ 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;
> > > > +
> > > > +    if ( writeable )
> > > > +        t = p2m_ram_rw;
> > > > +    else
> > > > +        t = p2m_ram_ro;
> > > 
> > > This is not the right p2m type to use here. p2m_ram_{rw,ro} allow foreign
> > > mapping. So another guest could access to the grant.
> > > 
> > > I would use p2m_iommu_map_{rw,ro}.
> > 
> > 
> > I see. I'll make the change and add a comment to explain why we are
> > using p2m_iommu types for non-iommu related mappings.
> 
> The code to add the 1:1 mapping for the SMMU is very ugly. I was wondering if
> we could drop the iommu_map_page callback and
> directly use arch_grant_map_page_identity in all the case.
> 
> It would avoid the if/else in the grant code.

Thing is arch_grant_map_page_identity has nothing to do with IOMMUs. It
gets confusing, especially on x86 where people don't know why we need
this. The if/else is ugly but clearer.
See also the previous email exchange with Jan.

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

[Julien Grall]

On 24/07/14 12:16, Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Julien Grall wrote:
>> On 24/07/14 12:07, Stefano Stabellini wrote:
>>> On Thu, 24 Jul 2014, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> The Title of the commit message is now wrong.
>>>
>>> Thanks, I didn't notice.
>>>
>>>
>>>> On 23/07/14 18:19, Stefano Stabellini wrote:
>>>>> Introduce two arch specific functions to create a new p2m mapping of
>>>>> granted pages at pfn == mfn.
>>>>> The x86 implementation just returns ENOSYS.
>>>>>
>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>> ---
>>>>>     xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
>>>>>     xen/include/asm-arm/p2m.h |    4 ++++
>>>>>     xen/include/asm-x86/p2m.h |   13 +++++++++++++
>>>>>     3 files changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 9960e17..c38af59 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -555,6 +555,25 @@ 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;
>>>>> +
>>>>> +    if ( writeable )
>>>>> +        t = p2m_ram_rw;
>>>>> +    else
>>>>> +        t = p2m_ram_ro;
>>>>
>>>> This is not the right p2m type to use here. p2m_ram_{rw,ro} allow foreign
>>>> mapping. So another guest could access to the grant.
>>>>
>>>> I would use p2m_iommu_map_{rw,ro}.
>>>
>>>
>>> I see. I'll make the change and add a comment to explain why we are
>>> using p2m_iommu types for non-iommu related mappings.
>>
>> The code to add the 1:1 mapping for the SMMU is very ugly. I was wondering if
>> we could drop the iommu_map_page callback and
>> directly use arch_grant_map_page_identity in all the case.
>>
>> It would avoid the if/else in the grant code.
>
> Thing is arch_grant_map_page_identity has nothing to do with IOMMUs. It
> gets confusing, especially on x86 where people don't know why we need
> this. The if/else is ugly but clearer.
> See also the previous email exchange with Jan.

Ok. So could we call this function from iommu_map_page on the SMMU? It 
would at least avoid duplicating the code?

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> On 24/07/14 12:16, Stefano Stabellini wrote:
> > On Thu, 24 Jul 2014, Julien Grall wrote:
> > > On 24/07/14 12:07, Stefano Stabellini wrote:
> > > > On Thu, 24 Jul 2014, Julien Grall wrote:
> > > > > Hi Stefano,
> > > > > 
> > > > > The Title of the commit message is now wrong.
> > > > 
> > > > Thanks, I didn't notice.
> > > > 
> > > > 
> > > > > On 23/07/14 18:19, Stefano Stabellini wrote:
> > > > > > Introduce two arch specific functions to create a new p2m mapping of
> > > > > > granted pages at pfn == mfn.
> > > > > > The x86 implementation just returns ENOSYS.
> > > > > > 
> > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > > > ---
> > > > > >     xen/arch/arm/p2m.c        |   19 +++++++++++++++++++
> > > > > >     xen/include/asm-arm/p2m.h |    4 ++++
> > > > > >     xen/include/asm-x86/p2m.h |   13 +++++++++++++
> > > > > >     3 files changed, 36 insertions(+)
> > > > > > 
> > > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > > > index 9960e17..c38af59 100644
> > > > > > --- a/xen/arch/arm/p2m.c
> > > > > > +++ b/xen/arch/arm/p2m.c
> > > > > > @@ -555,6 +555,25 @@ 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;
> > > > > > +
> > > > > > +    if ( writeable )
> > > > > > +        t = p2m_ram_rw;
> > > > > > +    else
> > > > > > +        t = p2m_ram_ro;
> > > > > 
> > > > > This is not the right p2m type to use here. p2m_ram_{rw,ro} allow
> > > > > foreign
> > > > > mapping. So another guest could access to the grant.
> > > > > 
> > > > > I would use p2m_iommu_map_{rw,ro}.
> > > > 
> > > > 
> > > > I see. I'll make the change and add a comment to explain why we are
> > > > using p2m_iommu types for non-iommu related mappings.
> > > 
> > > The code to add the 1:1 mapping for the SMMU is very ugly. I was wondering
> > > if
> > > we could drop the iommu_map_page callback and
> > > directly use arch_grant_map_page_identity in all the case.
> > > 
> > > It would avoid the if/else in the grant code.
> > 
> > Thing is arch_grant_map_page_identity has nothing to do with IOMMUs. It
> > gets confusing, especially on x86 where people don't know why we need
> > this. The if/else is ugly but clearer.
> > See also the previous email exchange with Jan.
> 
> Ok. So could we call this function from iommu_map_page on the SMMU? It would
> at least avoid duplicating the code?

Yes, that would be possible.

Re: [PATCH v2 2/2] xen/arm: introduce XENFEAT_grant_map_identity

[Jan Beulich]

>>> On 24.07.14 at 12:47, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 24 Jul 2014, Jan Beulich wrote:
>> >>> On 23.07.14 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
>> > @@ -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) )
>> 
>> This is bogus: On x86 the right part is always false, while on
>> ARM the right part is already part of gnttab_need_iommu_mapping().
>> IOW no need to change anything here afaict.
> 
> On ARM gnttab_need_iommu_mapping expands to
> 
> #define gnttab_need_iommu_mapping(d)                    \
>     (is_domain_direct_mapped(d) && need_iommu(d))
> 
> however we would need to do the second mapping even if need_iommu(d)
> returns false. Changing gnttab_need_iommu_mapping(d) on ARM the way I
> did in the previous version of the patch series seems wrong to me now,
> because it is not an iommu mapping that gnttab needs, but a second
> regular p2m mapping.
> 
> This is way I decided to add the "|| is_domain_direct_mapped(ld)".

I guessed that you did it for reasons along those lines. But unless
we're certain the compiler will always fold the redundant expressions
and unless you're certain the redundancy won't cause future
similar questions by readers of the code, I'd suggest to drop the
addition.

Jan