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

[PATCH v3 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 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 (0) on x86
      xen: introduce arch_grant_(un)map_page_identity
      xen/arm: introduce XENFEAT_grant_map_identity

 xen/arch/arm/p2m.c                 |   22 ++++++++++++++++++++++
 xen/common/grant_table.c           |   35 +++++++++++++++++++++++++++--------
 xen/common/kernel.c                |    3 +++
 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/grant_table.h  |    2 ++
 xen/include/asm-x86/p2m.h          |   13 +++++++++++++
 xen/include/public/features.h      |    3 +++
 10 files changed, 80 insertions(+), 19 deletions(-)

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

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/include/asm-x86/domain.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index abf55fb..5f9354b 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) (0)
 
 #define VCPU_TRAP_NMI          1
 #define VCPU_TRAP_MCE          2
-- 
1.7.10.4

[PATCH v3 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>

---
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                 |   22 ++++++++++++++++++++++
 xen/drivers/passthrough/arm/smmu.c |   13 ++-----------
 xen/include/asm-arm/p2m.h          |    4 ++++
 xen/include/asm-x86/p2m.h          |   13 +++++++++++++
 4 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9960e17..6024b03 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -555,6 +555,28 @@ 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;
+
+    /* 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)
+{
+    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/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..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 v3 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.

Introduce gnttab_need_identity_mapping, to check if a domain needs the
identity mapping. On x86 it just returns always 0.

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

---

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

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 464007e..67cbed9 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) || gnttab_need_identity_mapping(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_identity_mapping(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 ( gnttab_need_identity_mapping(ld) )
+                    err = arch_grant_map_page_identity(ld, frame, 0);
+                else
+                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
+            }
         }
         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) || gnttab_need_identity_mapping(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_identity_mapping(ld) )
+                err = arch_grant_unmap_page_identity(ld, op->frame);
+            else
+                err = iommu_unmap_page(ld, op->frame);
+        } else if ( wrc == 0 )
+        {
+            if ( gnttab_need_identity_mapping(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..dacbe38 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -15,6 +15,7 @@
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <asm/current.h>
+#include <asm/grant_table.h>
 #include <public/nmi.h>
 #include <public/version.h>
 
@@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                 break;
             }
 #endif
+            if ( gnttab_need_identity_mapping(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..6f7ccd9 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -36,6 +36,9 @@ static inline int replace_grant_supported(void)
 #define gnttab_need_iommu_mapping(d)                    \
     (is_domain_direct_mapped(d) && need_iommu(d))
 
+#define gnttab_need_identity_mapping(d)                    \
+    (is_domain_direct_mapped(d) && !need_iommu(d))
+
 #endif /* __ASM_GRANT_TABLE_H__ */
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 8c9bbcf..56861a7 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -68,6 +68,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st)
 #define gnttab_need_iommu_mapping(d)                \
     (!paging_mode_translate(d) && need_iommu(d))
 
+#define gnttab_need_identity_mapping(d)     (0)
+
 static inline int replace_grant_supported(void)
 {
     return 1;
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 v3 1/3] xen/x86: introduce is_domain_direct_mapped(d) as (0) on x86

[Julien Grall]

Hi Stefano,

On 07/24/2014 02:31 PM, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/include/asm-x86/domain.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index abf55fb..5f9354b 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) (0)

When I've implemented this defined in common/memory.c, Jan told me to
use [1]:

#define is_domain_is_direct_mapped(d) ((void)(d), 0)

I suspect you want the same things here.

Also, could you drop the define in common/memory.c. I don't think it's
useful now.

>  #define VCPU_TRAP_NMI          1
>  #define VCPU_TRAP_MCE          2
> 

Regards,

[1] https://patches.linaro.org/22523/

-- 
Julien Grall

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

[Jan Beulich]

>>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> --- 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) || gnttab_need_identity_mapping(ld) )

As before I think this change is pointless.

Jan

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

[Jan Beulich]

>>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> --- 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;
> +}

As said before I'd prefer these to just be extern declarations, allowing
eventual issues to be found at build time rather than them being
deferred to runtime.

Jan

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

[Julien Grall]

Hi Stefano,

On 07/24/2014 02:31 PM, 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.
> 
> 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>
> 
> ---
> 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                 |   22 ++++++++++++++++++++++
>  xen/drivers/passthrough/arm/smmu.c |   13 ++-----------
>  xen/include/asm-arm/p2m.h          |    4 ++++
>  xen/include/asm-x86/p2m.h          |   13 +++++++++++++
>  4 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 9960e17..6024b03 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -555,6 +555,28 @@ 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;

NIT: I would add an ASSERT(!is_domain_direct_mapped(d)) for sanity check.

> +
> +    /* 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)
> +{

Same here.

> +    guest_physmap_remove_page(d, frame, frame, 0);
> +    return 0;
> +}
> +

In any case:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi Stefano,

On 07/24/2014 02:31 PM, Stefano Stabellini wrote:
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 7e83353..dacbe38 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -15,6 +15,7 @@
>  #include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <asm/current.h>
> +#include <asm/grant_table.h>
>  #include <public/nmi.h>
>  #include <public/version.h>
>  
> @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>                  break;
>              }
>  #endif
> +            if ( gnttab_need_identity_mapping(d) )

Actually even platform the IOMMU support needs to have this flags on.
With this solution you break platform where not every DMA-capable device
are behind an SMMU.

> +                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..6f7ccd9 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void)
>  #define gnttab_need_iommu_mapping(d)                    \
>      (is_domain_direct_mapped(d) && need_iommu(d))
>  
> +#define gnttab_need_identity_mapping(d)                    \
> +    (is_domain_direct_mapped(d) && !need_iommu(d))
> +

Why didn't you drop the need_iommu(d) in is_domain_direct_mapped?

Hence the name is confusing. You may think that we don't need identity
mapping when IOMMU is used while it's actually the case, but we use a
different function.

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Jan Beulich wrote:
> >>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> > --- 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) || gnttab_need_identity_mapping(ld) )
> 
> As before I think this change is pointless.

I don't understand how you propose to solve the problem.
In fact the two are not the same.

On ARM:

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

#ifdef HAS_PASSTHROUGH
#define need_iommu(d)    ((d)->need_iommu)
#else
#define need_iommu(d)    (0)
#endif

Also need_iommu is only set if an iommu is available.

On the other hand:

#define gnttab_need_identity_mapping(d)                    \
    (is_domain_direct_mapped(d) && !need_iommu(d))

So gnttab_need_identity_mapping would cover the case of a direct_mapped
dom0 with an iommu available.

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/24/2014 02:31 PM, Stefano Stabellini wrote:
> > diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> > index 7e83353..dacbe38 100644
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -15,6 +15,7 @@
> >  #include <xen/guest_access.h>
> >  #include <xen/hypercall.h>
> >  #include <asm/current.h>
> > +#include <asm/grant_table.h>
> >  #include <public/nmi.h>
> >  #include <public/version.h>
> >  
> > @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >                  break;
> >              }
> >  #endif
> > +            if ( gnttab_need_identity_mapping(d) )
> 
> Actually even platform the IOMMU support needs to have this flags on.
> With this solution you break platform where not every DMA-capable device
> are behind an SMMU.

I see.


> > +                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..6f7ccd9 100644
> > --- a/xen/include/asm-arm/grant_table.h
> > +++ b/xen/include/asm-arm/grant_table.h
> > @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void)
> >  #define gnttab_need_iommu_mapping(d)                    \
> >      (is_domain_direct_mapped(d) && need_iommu(d))
> >  
> > +#define gnttab_need_identity_mapping(d)                    \
> > +    (is_domain_direct_mapped(d) && !need_iommu(d))
> > +
> 
> Why didn't you drop the need_iommu(d) in is_domain_direct_mapped?

I guess you mean why didn't you drop the need_iommu(d) in
gnttab_need_iommu_mapping?

Because how can you need an iommu mapping if an iommu is not present?


> Hence the name is confusing. You may think that we don't need identity
> mapping when IOMMU is used while it's actually the case, but we use a
> different function.

Yes, you are right. The whole thing is a bit confusing, I am trying to
come up with a naming scheme that is as clear as possible.
What if we simply define:

#define gnttab_need_identity_mapping(d) (is_domain_direct_mapped(d))

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

[Jan Beulich]

>>> On 24.07.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 24 Jul 2014, Jan Beulich wrote:
>> >>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- 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) || gnttab_need_identity_mapping(ld) 
> )
>> 
>> As before I think this change is pointless.
> 
> I don't understand how you propose to solve the problem.

I think ARM's gnttab_need_iommu_mapping() needs to just be
is_domain_direct_mapped() then.

Jan

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

[Julien Grall]

On 07/24/2014 03:43 PM, Jan Beulich wrote:
>>>> On 24.07.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
>> On Thu, 24 Jul 2014, Jan Beulich wrote:
>>>>>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
>>>> --- 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) || gnttab_need_identity_mapping(ld) 
>> )
>>>
>>> As before I think this change is pointless.
>>
>> I don't understand how you propose to solve the problem.
> 
> I think ARM's gnttab_need_iommu_mapping() needs to just be
> is_domain_direct_mapped() then.

So ARM people will think we need an iommu to add the identity mapping.
Why don't we rename gnttab_need_iommu_mapping into
gnttab_need_identity_mapping?

It would less confusing for both x86 and ARM.

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 07/24/2014 03:14 PM, Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 07/24/2014 02:31 PM, Stefano Stabellini wrote:
>>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>>> index 7e83353..dacbe38 100644
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -15,6 +15,7 @@
>>>  #include <xen/guest_access.h>
>>>  #include <xen/hypercall.h>
>>>  #include <asm/current.h>
>>> +#include <asm/grant_table.h>
>>>  #include <public/nmi.h>
>>>  #include <public/version.h>
>>>  
>>> @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>                  break;
>>>              }
>>>  #endif
>>> +            if ( gnttab_need_identity_mapping(d) )
>>
>> Actually even platform the IOMMU support needs to have this flags on.
>> With this solution you break platform where not every DMA-capable device
>> are behind an SMMU.
> 
> I see.
> 
> 
>>> +                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..6f7ccd9 100644
>>> --- a/xen/include/asm-arm/grant_table.h
>>> +++ b/xen/include/asm-arm/grant_table.h
>>> @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void)
>>>  #define gnttab_need_iommu_mapping(d)                    \
>>>      (is_domain_direct_mapped(d) && need_iommu(d))
>>>  
>>> +#define gnttab_need_identity_mapping(d)                    \
>>> +    (is_domain_direct_mapped(d) && !need_iommu(d))
>>> +
>>
>> Why didn't you drop the need_iommu(d) in is_domain_direct_mapped?
> 
> I guess you mean why didn't you drop the need_iommu(d) in
> gnttab_need_iommu_mapping?
> 
> Because how can you need an iommu mapping if an iommu is not present?

I would invert the name of the 2 macros. And drop !need_iommu(d) in the
second.

The code would look like:

if (gnttab_need_identity_mapping(d))
{
   if (gnttab_need_iommu_mapping(d))
     iommu_map_page
   else
     arch_
}

On x86, the gnttab_need_iommu_mapping would be equal to 1.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> On 07/24/2014 03:43 PM, Jan Beulich wrote:
> >>>> On 24.07.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> >> On Thu, 24 Jul 2014, Jan Beulich wrote:
> >>>>>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> >>>> --- 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) || gnttab_need_identity_mapping(ld) 
> >> )
> >>>
> >>> As before I think this change is pointless.
> >>
> >> I don't understand how you propose to solve the problem.
> > 
> > I think ARM's gnttab_need_iommu_mapping() needs to just be
> > is_domain_direct_mapped() then.

That's not correct either. It would work, but it is conceptually wrong.
>From the ARM perspective we have 2 distinct cases to cover:
- we have an smmu and we need to create a mapping for it
- we don't have an smmu and we need an identity mapping


> So ARM people will think we need an iommu to add the identity mapping.
> Why don't we rename gnttab_need_iommu_mapping into
> gnttab_need_identity_mapping?
> 
> It would less confusing for both x86 and ARM.

I don't think that would be any better.
We have 2 distinct cases to cover, we need to treat them as such.

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

[Julien Grall]

On 07/24/2014 04:02 PM, Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Julien Grall wrote:
>> On 07/24/2014 03:43 PM, Jan Beulich wrote:
>>>>>> On 24.07.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
>>>> On Thu, 24 Jul 2014, Jan Beulich wrote:
>>>>>>>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
>>>>>> --- 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) || gnttab_need_identity_mapping(ld) 
>>>> )
>>>>>
>>>>> As before I think this change is pointless.
>>>>
>>>> I don't understand how you propose to solve the problem.
>>>
>>> I think ARM's gnttab_need_iommu_mapping() needs to just be
>>> is_domain_direct_mapped() then.
> 
> That's not correct either. It would work, but it is conceptually wrong.
> From the ARM perspective we have 2 distinct cases to cover:
> - we have an smmu and we need to create a mapping for it
> - we don't have an smmu and we need an identity mapping
> 
> 
>> So ARM people will think we need an iommu to add the identity mapping.
>> Why don't we rename gnttab_need_iommu_mapping into
>> gnttab_need_identity_mapping?
>>
>> It would less confusing for both x86 and ARM.
> 
> I don't think that would be any better.
> We have 2 distinct cases to cover, we need to treat them as such.

The cases are not so distinct. We need the 1:1 grant mapping for the
IOMMU because of the swiotlb and the 1:1 memory mapping.

Without it we can easily bypass this code.

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/24/2014 02:31 PM, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/include/asm-x86/domain.h |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> > index abf55fb..5f9354b 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) (0)
> 
> When I've implemented this defined in common/memory.c, Jan told me to
> use [1]:
> 
> #define is_domain_is_direct_mapped(d) ((void)(d), 0)
> 
> I suspect you want the same things here.

OK


> Also, could you drop the define in common/memory.c. I don't think it's
> useful now.

I'll do

> 
> >  #define VCPU_TRAP_NMI          1
> >  #define VCPU_TRAP_MCE          2
> > 
> 
> Regards,
> 
> [1] https://patches.linaro.org/22523/
> 
> -- 
> Julien Grall
> 

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Jan Beulich wrote:
> >>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> > --- 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;
> > +}
> 
> As said before I'd prefer these to just be extern declarations, allowing
> eventual issues to be found at build time rather than them being
> deferred to runtime.

OK

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/24/2014 02:31 PM, 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.
> > 
> > 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>
> > 
> > ---
> > 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                 |   22 ++++++++++++++++++++++
> >  xen/drivers/passthrough/arm/smmu.c |   13 ++-----------
> >  xen/include/asm-arm/p2m.h          |    4 ++++
> >  xen/include/asm-x86/p2m.h          |   13 +++++++++++++
> >  4 files changed, 41 insertions(+), 11 deletions(-)
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 9960e17..6024b03 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -555,6 +555,28 @@ 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;
> 
> NIT: I would add an ASSERT(!is_domain_direct_mapped(d)) for sanity check.
>

you mean 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)
> > +{
> 
> Same here.
> 
> > +    guest_physmap_remove_page(d, frame, frame, 0);
> > +    return 0;
> > +}
> > +
> 
> In any case:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

thanks

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

[Julien Grall]

On 07/24/2014 05:57 PM, Stefano Stabellini wrote:
> On Thu, 24 Jul 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 07/24/2014 02:31 PM, 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.
>>>
>>> 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>
>>>
>>> ---
>>> 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                 |   22 ++++++++++++++++++++++
>>>  xen/drivers/passthrough/arm/smmu.c |   13 ++-----------
>>>  xen/include/asm-arm/p2m.h          |    4 ++++
>>>  xen/include/asm-x86/p2m.h          |   13 +++++++++++++
>>>  4 files changed, 41 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 9960e17..6024b03 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -555,6 +555,28 @@ 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;
>>
>> NIT: I would add an ASSERT(!is_domain_direct_mapped(d)) for sanity check.
>>
> 
> you mean ASSERT(is_domain_direct_mapped(d))

Yes. Sorry.

>>> +
>>> +    /* 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)
>>> +{
>>
>> Same here.
>>
>>> +    guest_physmap_remove_page(d, frame, frame, 0);
>>> +    return 0;
>>> +}
>>> +
>>
>> In any case:
>>
>> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> thanks
> 


-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 24 Jul 2014, Julien Grall wrote:
> On 07/24/2014 04:02 PM, Stefano Stabellini wrote:
> > On Thu, 24 Jul 2014, Julien Grall wrote:
> >> On 07/24/2014 03:43 PM, Jan Beulich wrote:
> >>>>>> On 24.07.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> >>>> On Thu, 24 Jul 2014, Jan Beulich wrote:
> >>>>>>>> On 24.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> >>>>>> --- 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) || gnttab_need_identity_mapping(ld) 
> >>>> )
> >>>>>
> >>>>> As before I think this change is pointless.
> >>>>
> >>>> I don't understand how you propose to solve the problem.
> >>>
> >>> I think ARM's gnttab_need_iommu_mapping() needs to just be
> >>> is_domain_direct_mapped() then.
> > 
> > That's not correct either. It would work, but it is conceptually wrong.
> > From the ARM perspective we have 2 distinct cases to cover:
> > - we have an smmu and we need to create a mapping for it
> > - we don't have an smmu and we need an identity mapping
> > 
> > 
> >> So ARM people will think we need an iommu to add the identity mapping.
> >> Why don't we rename gnttab_need_iommu_mapping into
> >> gnttab_need_identity_mapping?
> >>
> >> It would less confusing for both x86 and ARM.
> > 
> > I don't think that would be any better.
> > We have 2 distinct cases to cover, we need to treat them as such.
> 
> The cases are not so distinct. We need the 1:1 grant mapping for the
> IOMMU because of the swiotlb and the 1:1 memory mapping.
> 
> Without it we can easily bypass this code.

Only until we have just one smmu driver with shared pagetables.
Otherwise the two cases are different.

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 24/07/14 14:30, 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.

Is there a version of these patches for Xen 4.4 that I can test? The 
restriction on duplicate pages is causing trouble for networking on 
Mirage 
(http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).

> 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 (0) on x86
>        xen: introduce arch_grant_(un)map_page_identity
>        xen/arm: introduce XENFEAT_grant_map_identity
>
>   xen/arch/arm/p2m.c                 |   22 ++++++++++++++++++++++
>   xen/common/grant_table.c           |   35 +++++++++++++++++++++++++++--------
>   xen/common/kernel.c                |    3 +++
>   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/grant_table.h  |    2 ++
>   xen/include/asm-x86/p2m.h          |   13 +++++++++++++
>   xen/include/public/features.h      |    3 +++
>   10 files changed, 80 insertions(+), 19 deletions(-)
>

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 24/07/14 14:30, 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.

Is there a version of these patches for Xen 4.4 that I can test? The 
restriction on duplicate pages is causing trouble for networking on 
Mirage too 
(http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).

> 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 (0) on x86
>        xen: introduce arch_grant_(un)map_page_identity
>        xen/arm: introduce XENFEAT_grant_map_identity
>
>   xen/arch/arm/p2m.c                 |   22 ++++++++++++++++++++++
>   xen/common/grant_table.c           |   35 +++++++++++++++++++++++++++--------
>   xen/common/kernel.c                |    3 +++
>   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/grant_table.h  |    2 ++
>   xen/include/asm-x86/p2m.h          |   13 +++++++++++++
>   xen/include/public/features.h      |    3 +++
>   10 files changed, 80 insertions(+), 19 deletions(-)
>


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Fri, 1 Aug 2014, Thomas Leonard wrote:
> On 24/07/14 14:30, 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.
> 
> Is there a version of these patches for Xen 4.4 that I can test? The
> restriction on duplicate pages is causing trouble for networking on Mirage too
> (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).

The backport is non-trivial because
552710b388630dfa461932940a998e120c42277d is missing from 4.4,
nonetheless it wasn't too hard to port:

  git://xenbits.xen.org/people/sstabellini/xen-unstable.git grant_map_identity_4.4



> > 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 (0) on x86
> >        xen: introduce arch_grant_(un)map_page_identity
> >        xen/arm: introduce XENFEAT_grant_map_identity
> > 
> >   xen/arch/arm/p2m.c                 |   22 ++++++++++++++++++++++
> >   xen/common/grant_table.c           |   35
> > +++++++++++++++++++++++++++--------
> >   xen/common/kernel.c                |    3 +++
> >   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/grant_table.h  |    2 ++
> >   xen/include/asm-x86/p2m.h          |   13 +++++++++++++
> >   xen/include/public/features.h      |    3 +++
> >   10 files changed, 80 insertions(+), 19 deletions(-)
> > 
> 
> 
> -- 
> Dr Thomas Leonard        http://0install.net/
> GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
> GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA
> 

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 1 August 2014 16:13, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> On 24/07/14 14:30, 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.
>>
>> Is there a version of these patches for Xen 4.4 that I can test? The
>> restriction on duplicate pages is causing trouble for networking on Mirage too
>> (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>
> The backport is non-trivial because
> 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
> nonetheless it wasn't too hard to port:
>
>   git://xenbits.xen.org/people/sstabellini/xen-unstable.git grant_map_identity_4.4

Thanks. I merged it with the stable-4.4 branch (as that has some
useful fixes too), but it crashed for me when I started my Mirage
guest:

Xen BUG at grant_table.c:729
(XEN) CPU1: Unexpected Trap: Undefined Instruction
(XEN) ----[ Xen-4.4.1-rc1  arm32  debug=y  Not tainted ]----
(XEN) CPU:    1
(XEN) PC:     00245f64 __bug+0x2c/0x48
(XEN) CPSR:   200b015a MODE:Hypervisor
(XEN)      R0: 0026c6dc R1: 00000003 R2: 3fd23d80 R3: 200b015a
(XEN)      R4: 000002d9 R5: 00262a34 R6: 40027000 R7: 0009dcae
(XEN)      R8: 4ffe3e30 R9: 00000000 R10:00000000 R11:4002fddc R12:00000004
(XEN) HYP: SP: 4002fdd4 LR: 00245f64
(XEN)
(XEN)   VTCR_EL2: 80002558
(XEN)  VTTBR_EL2: 00010000bec06000
(XEN)
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038283f
(XEN)  TTBR0_EL2: 00000000ae010000
(XEN)
(XEN)    ESR_EL2: 00000000
(XEN)  HPFAR_EL2: 000000000001c810
(XEN)      HDFAR: e0800f00
(XEN)      HIFAR: 7d69b44d
(XEN)
(XEN) Xen stack trace from sp=4002fdd4:
(XEN)    00000000 4002fedc 0021043c 40027000 4002fe68 00000007 4002fe04 00000000
(XEN)    00000000 00000001 00000000 002ee500 00000000 002ee298 00000000 00090001
(XEN)    40020008 002ef614 00000000 002ee500 00000001 d9939880 002ee298 4002501c
(XEN)    00000000 00000001 002ee500 4ffe4000 d9939880 40027000 40026e30 00000000
(XEN)    00000000 0000ef13 0000f1db 40005bf8 02bb95c0 0009dcae 0000ee6b 00002800
(XEN)    00000000 6dfba000 00000000 00000002 00000001 00000001 ffffffff 00000000
(XEN)    00000000 c0c6f118 00000000 00000001 00000000 00000000 00000000 00007ff0
(XEN)    00000000 00000000 4002ff58 c020e6b4 00000000 00000001 00000ea1 c0b71028
(XEN)    00000000 4002ff54 00255300 d99391c0 0025272c 4002ff0c 00230e88 00000001
(XEN)    002b9ff0 002b6000 002ef614 0026cb80 002b9ff0 4002ff3c 40039954 00000000
(XEN)    ffffffff 002ef614 00000001 c0c6f0f8 c0b731c4 00000001 c0c6f120 dbd98740
(XEN)    d99391c0 db58a898 00000000 00000001 d9939880 c0b71028 00000000 4002ff58
(XEN)    00257d10 00000000 d9939880 00000001 00000001 d99391c0 db58a898 00000000
(XEN)    00000001 d9939880 c0b71028 00000000 d9939b80 00000014 ffffffff b6f249f8
(XEN)    c020e6b4 600b0013 f7ffffff be88ea9c c0c35c00 c0212f40 db681e48 c0499794
(XEN)    c0c35c0c c0213160 c0c35c18 c0212fa0 00000000 00000000 00000000 00000000
(XEN)    00000000 00000000 bfffffff 400b0030 800b0193 20010093 60010193 00000000
(XEN)    ffffffff ffffffff ffffffff
(XEN) Xen call trace:
(XEN)    [<00245f64>] __bug+0x2c/0x48 (PC)
(XEN)    [<00245f64>] __bug+0x2c/0x48 (LR)
(XEN)    [<0021043c>] do_grant_table_op+0x94c/0x2e8c
(XEN)    [<00255300>] do_trap_hypervisor+0xb4c/0xea8
(XEN)    [<00257d10>] return_from_trap+0/0x4
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 1:
(XEN) CPU1: Unexpected Trap: Undefined Instruction
(XEN)
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...

My merged version is here (there were no conflicts):

https://github.com/talex5/xen/tree/stable-4.4


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Julien Grall]

On 01/08/14 17:16, Thomas Leonard wrote:
> On 1 August 2014 16:13, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
>> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>>> On 24/07/14 14:30, 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.
>>>
>>> Is there a version of these patches for Xen 4.4 that I can test? The
>>> restriction on duplicate pages is causing trouble for networking on Mirage too
>>> (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>>
>> The backport is non-trivial because
>> 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
>> nonetheless it wasn't too hard to port:
>>
>>    git://xenbits.xen.org/people/sstabellini/xen-unstable.git grant_map_identity_4.4
>
> Thanks. I merged it with the stable-4.4 branch (as that has some
> useful fixes too), but it crashed for me when I started my Mirage
> guest:

You have to drop the BUG_ON line 729.

Regards,

-- 
Julien Grall

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Fri, 1 Aug 2014, Julien Grall wrote:
> On 01/08/14 17:16, Thomas Leonard wrote:
> > On 1 August 2014 16:13, Stefano Stabellini
> > <stefano.stabellini@eu.citrix.com> wrote:
> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> > > > On 24/07/14 14:30, 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.
> > > > 
> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> > > > restriction on duplicate pages is causing trouble for networking on
> > > > Mirage too
> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> > > 
> > > The backport is non-trivial because
> > > 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
> > > nonetheless it wasn't too hard to port:
> > > 
> > >    git://xenbits.xen.org/people/sstabellini/xen-unstable.git
> > > grant_map_identity_4.4
> > 
> > Thanks. I merged it with the stable-4.4 branch (as that has some
> > useful fixes too), but it crashed for me when I started my Mirage
> > guest:
> 
> You have to drop the BUG_ON line 729.

Yes, you are right

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 1 August 2014 17:25, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 1 Aug 2014, Julien Grall wrote:
>> On 01/08/14 17:16, Thomas Leonard wrote:
>> > On 1 August 2014 16:13, Stefano Stabellini
>> > <stefano.stabellini@eu.citrix.com> wrote:
>> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> > > > On 24/07/14 14:30, 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.
>> > > >
>> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> > > > restriction on duplicate pages is causing trouble for networking on
>> > > > Mirage too
>> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> > >
>> > > The backport is non-trivial because
>> > > 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
>> > > nonetheless it wasn't too hard to port:
>> > >
>> > >    git://xenbits.xen.org/people/sstabellini/xen-unstable.git
>> > > grant_map_identity_4.4
>> >
>> > Thanks. I merged it with the stable-4.4 branch (as that has some
>> > useful fixes too), but it crashed for me when I started my Mirage
>> > guest:
>>
>> You have to drop the BUG_ON line 729.
>
> Yes, you are right

This one too?

(XEN) Xen BUG at grant_table.c:946
(XEN) CPU1: Unexpected Trap: Undefined Instruction


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Fri, 1 Aug 2014, Thomas Leonard wrote:
> On 1 August 2014 17:25, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 1 Aug 2014, Julien Grall wrote:
> >> On 01/08/14 17:16, Thomas Leonard wrote:
> >> > On 1 August 2014 16:13, Stefano Stabellini
> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> > > > On 24/07/14 14:30, 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.
> >> > > >
> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> >> > > > restriction on duplicate pages is causing trouble for networking on
> >> > > > Mirage too
> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> >> > >
> >> > > The backport is non-trivial because
> >> > > 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
> >> > > nonetheless it wasn't too hard to port:
> >> > >
> >> > >    git://xenbits.xen.org/people/sstabellini/xen-unstable.git
> >> > > grant_map_identity_4.4
> >> >
> >> > Thanks. I merged it with the stable-4.4 branch (as that has some
> >> > useful fixes too), but it crashed for me when I started my Mirage
> >> > guest:
> >>
> >> You have to drop the BUG_ON line 729.
> >
> > Yes, you are right
> 
> This one too?
> 
> (XEN) Xen BUG at grant_table.c:946
> (XEN) CPU1: Unexpected Trap: Undefined Instruction

Yep, sorry.

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 1 August 2014 18:01, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> On 1 August 2014 17:25, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> > > > On 24/07/14 14:30, 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.
>> >> > > >
>> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >> > > > Mirage too
>> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> >> > >
>> >> > > The backport is non-trivial because
>> >> > > 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
>> >> > > nonetheless it wasn't too hard to port:
>> >> > >
>> >> > >    git://xenbits.xen.org/people/sstabellini/xen-unstable.git
>> >> > > grant_map_identity_4.4
>> >> >
>> >> > Thanks. I merged it with the stable-4.4 branch (as that has some
>> >> > useful fixes too), but it crashed for me when I started my Mirage
>> >> > guest:
>> >>
>> >> You have to drop the BUG_ON line 729.
>> >
>> > Yes, you are right
>>
>> This one too?
>>
>> (XEN) Xen BUG at grant_table.c:946
>> (XEN) CPU1: Unexpected Trap: Undefined Instruction
>
> Yep, sorry.

It seems that removing this makes it not compile:

arm-linux-gnueabihf-ld    -EL  -T xen.lds -N prelink.o \
            /xen-arm-builder/xen/xen/common/symbols-dummy.o -o
/xen-arm-builder/xen/xen/.xen-syms.0
prelink.o: In function `__gnttab_unmap_common':
/xen-arm-builder/xen/xen/common/grant_table.c:952: undefined reference
to `iommu_unmap_page'
/xen-arm-builder/xen/xen/common/grant_table.c:959: undefined reference
to `iommu_map_page'
prelink.o: In function `__gnttab_map_grant_ref':
/xen-arm-builder/xen/xen/common/grant_table.c:739: undefined reference
to `iommu_map_page'
/xen-arm-builder/xen/xen/common/grant_table.c:750: undefined reference
to `iommu_map_page'
arm-linux-gnueabihf-ld: /xen-arm-builder/xen/xen/.xen-syms.0: hidden
symbol `iommu_unmap_page' isn't defined
arm-linux-gnueabihf-ld: final link failed: Bad value

Maybe the BUG_ON made this code unreachable before?


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Fri, 1 Aug 2014, Thomas Leonard wrote:
> On 1 August 2014 18:01, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> On 1 August 2014 17:25, Stefano Stabellini
> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
> >> >> > On 1 August 2014 16:13, Stefano Stabellini
> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> > > > On 24/07/14 14:30, 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.
> >> >> > > >
> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> >> >> > > > restriction on duplicate pages is causing trouble for networking on
> >> >> > > > Mirage too
> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> >> >> > >
> >> >> > > The backport is non-trivial because
> >> >> > > 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
> >> >> > > nonetheless it wasn't too hard to port:
> >> >> > >
> >> >> > >    git://xenbits.xen.org/people/sstabellini/xen-unstable.git
> >> >> > > grant_map_identity_4.4
> >> >> >
> >> >> > Thanks. I merged it with the stable-4.4 branch (as that has some
> >> >> > useful fixes too), but it crashed for me when I started my Mirage
> >> >> > guest:
> >> >>
> >> >> You have to drop the BUG_ON line 729.
> >> >
> >> > Yes, you are right
> >>
> >> This one too?
> >>
> >> (XEN) Xen BUG at grant_table.c:946
> >> (XEN) CPU1: Unexpected Trap: Undefined Instruction
> >
> > Yep, sorry.
> 
> It seems that removing this makes it not compile:
> 
> arm-linux-gnueabihf-ld    -EL  -T xen.lds -N prelink.o \
>             /xen-arm-builder/xen/xen/common/symbols-dummy.o -o
> /xen-arm-builder/xen/xen/.xen-syms.0
> prelink.o: In function `__gnttab_unmap_common':
> /xen-arm-builder/xen/xen/common/grant_table.c:952: undefined reference
> to `iommu_unmap_page'
> /xen-arm-builder/xen/xen/common/grant_table.c:959: undefined reference
> to `iommu_map_page'
> prelink.o: In function `__gnttab_map_grant_ref':
> /xen-arm-builder/xen/xen/common/grant_table.c:739: undefined reference
> to `iommu_map_page'
> /xen-arm-builder/xen/xen/common/grant_table.c:750: undefined reference
> to `iommu_map_page'
> arm-linux-gnueabihf-ld: /xen-arm-builder/xen/xen/.xen-syms.0: hidden
> symbol `iommu_unmap_page' isn't defined
> arm-linux-gnueabihf-ld: final link failed: Bad value
> 
> Maybe the BUG_ON made this code unreachable before?

Checkout the branch grant_map_identity_4.4_2

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 1 August 2014 18:16, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> On 1 August 2014 18:01, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> On 1 August 2014 17:25, Stefano Stabellini
>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> > > > On 24/07/14 14:30, 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.
>> >> >> > > >
>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >> >> > > > Mirage too
>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> >> >> > >
>> >> >> > > The backport is non-trivial because
>> >> >> > > 552710b388630dfa461932940a998e120c42277d is missing from 4.4,
>> >> >> > > nonetheless it wasn't too hard to port:
>> >> >> > >
>> >> >> > >    git://xenbits.xen.org/people/sstabellini/xen-unstable.git
>> >> >> > > grant_map_identity_4.4
>> >> >> >
>> >> >> > Thanks. I merged it with the stable-4.4 branch (as that has some
>> >> >> > useful fixes too), but it crashed for me when I started my Mirage
>> >> >> > guest:
>> >> >>
>> >> >> You have to drop the BUG_ON line 729.
>> >> >
>> >> > Yes, you are right
>> >>
>> >> This one too?
>> >>
>> >> (XEN) Xen BUG at grant_table.c:946
>> >> (XEN) CPU1: Unexpected Trap: Undefined Instruction
>> >
>> > Yep, sorry.
>>
>> It seems that removing this makes it not compile:
>>
>> arm-linux-gnueabihf-ld    -EL  -T xen.lds -N prelink.o \
>>             /xen-arm-builder/xen/xen/common/symbols-dummy.o -o
>> /xen-arm-builder/xen/xen/.xen-syms.0
>> prelink.o: In function `__gnttab_unmap_common':
>> /xen-arm-builder/xen/xen/common/grant_table.c:952: undefined reference
>> to `iommu_unmap_page'
>> /xen-arm-builder/xen/xen/common/grant_table.c:959: undefined reference
>> to `iommu_map_page'
>> prelink.o: In function `__gnttab_map_grant_ref':
>> /xen-arm-builder/xen/xen/common/grant_table.c:739: undefined reference
>> to `iommu_map_page'
>> /xen-arm-builder/xen/xen/common/grant_table.c:750: undefined reference
>> to `iommu_map_page'
>> arm-linux-gnueabihf-ld: /xen-arm-builder/xen/xen/.xen-syms.0: hidden
>> symbol `iommu_unmap_page' isn't defined
>> arm-linux-gnueabihf-ld: final link failed: Bad value
>>
>> Maybe the BUG_ON made this code unreachable before?
>
> Checkout the branch grant_map_identity_4.4_2

Thanks - it's working now!


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
> On 1 August 2014 18:16, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
>> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>>> On 1 August 2014 18:01, Stefano Stabellini
>>> <stefano.stabellini@eu.citrix.com> wrote:
>>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>>> >> On 1 August 2014 17:25, Stefano Stabellini
>>> >> <stefano.stabellini@eu.citrix.com> wrote:
>>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>>> >> >> > > > On 24/07/14 14:30, 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.
>>> >> >> > > >
>>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>>> >> >> > > > Mirage too
>>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
[...]
>> Checkout the branch grant_map_identity_4.4_2
>
> Thanks - it's working now!

Update: if my VM uses the network and then exits quickly, dom0 gets
stuck. It keeps logging:

[  360.652715] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.652796] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.652835] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.652874] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.652910] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.652947] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.652983] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d
[  360.653026] vif vif-1-0 (unregistered net_device): Page still
granted! Index: 9d

The domain can't be destroyed:

$ sudo xl list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   512     2     r-----     185.7
(null)                                      10     0     1     --ps-d       0.1

top shows the "xenwatch" process using 100% CPU and I have to reboot.

A work-around is to use:

  on_poweroff = 'preserve'


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Wed, 6 Aug 2014, Thomas Leonard wrote:
> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
> > On 1 August 2014 18:16, Stefano Stabellini
> > <stefano.stabellini@eu.citrix.com> wrote:
> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >>> On 1 August 2014 18:01, Stefano Stabellini
> >>> <stefano.stabellini@eu.citrix.com> wrote:
> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >>> >> On 1 August 2014 17:25, Stefano Stabellini
> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >>> >> >> > > > On 24/07/14 14:30, 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.
> >>> >> >> > > >
> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
> >>> >> >> > > > Mirage too
> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> [...]
> >> Checkout the branch grant_map_identity_4.4_2
> >
> > Thanks - it's working now!
> 
> Update: if my VM uses the network and then exits quickly, dom0 gets
> stuck.

What do you mean by exit quickly? xl destroy domain?


> It keeps logging:
> 
> [  360.652715] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.652796] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.652835] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.652874] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.652910] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.652947] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.652983] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> [  360.653026] vif vif-1-0 (unregistered net_device): Page still
> granted! Index: 9d
> 
> The domain can't be destroyed:
> 
> $ sudo xl list
> Name                                        ID   Mem VCPUs      State   Time(s)
> Domain-0                                     0   512     2     r-----     185.7
> (null)                                      10     0     1     --ps-d       0.1
> 
> top shows the "xenwatch" process using 100% CPU and I have to reboot.
> 
> A work-around is to use:
> 
>   on_poweroff = 'preserve'

Does the "Page still granted!" message go away after a while, or just
keeps printing it?


> 
> -- 
> Dr Thomas Leonard        http://0install.net/
> GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
> GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA
> 

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 14:35, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
>> > On 1 August 2014 18:16, Stefano Stabellini
>> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >>> On 1 August 2014 18:01, Stefano Stabellini
>> >>> <stefano.stabellini@eu.citrix.com> wrote:
>> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >>> >> On 1 August 2014 17:25, Stefano Stabellini
>> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >>> >> >> > > > On 24/07/14 14:30, 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.
>> >>> >> >> > > >
>> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >>> >> >> > > > Mirage too
>> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> [...]
>> >> Checkout the branch grant_map_identity_4.4_2
>> >
>> > Thanks - it's working now!
>>
>> Update: if my VM uses the network and then exits quickly, dom0 gets
>> stuck.
>
> What do you mean by exit quickly? xl destroy domain?

The guest starts, sends a few packets and then exits (possibly before
the packets have been transmitted). This happens before "xl create"
manages to attach to the console.

>> It keeps logging:
>>
>> [  360.652715] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.652796] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.652835] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.652874] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.652910] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.652947] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.652983] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>> [  360.653026] vif vif-1-0 (unregistered net_device): Page still
>> granted! Index: 9d
>>
>> The domain can't be destroyed:
>>
>> $ sudo xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   512     2     r-----     185.7
>> (null)                                      10     0     1     --ps-d       0.1
>>
>> top shows the "xenwatch" process using 100% CPU and I have to reboot.
>>
>> A work-around is to use:
>>
>>   on_poweroff = 'preserve'
>
> Does the "Page still granted!" message go away after a while, or just
> keeps printing it?

It just keeps printing it.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Wed, 6 Aug 2014, Thomas Leonard wrote:
> On 6 August 2014 14:35, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
> >> > On 1 August 2014 18:16, Stefano Stabellini
> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >>> On 1 August 2014 18:01, Stefano Stabellini
> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >>> >> >> > > > On 24/07/14 14:30, 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.
> >> >>> >> >> > > >
> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
> >> >>> >> >> > > > Mirage too
> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> >> [...]
> >> >> Checkout the branch grant_map_identity_4.4_2
> >> >
> >> > Thanks - it's working now!
> >>
> >> Update: if my VM uses the network and then exits quickly, dom0 gets
> >> stuck.
> >
> > What do you mean by exit quickly? xl destroy domain?
> 
> The guest starts, sends a few packets and then exits (possibly before
> the packets have been transmitted). This happens before "xl create"
> manages to attach to the console.

I haven't seen anything like this and I cannot reproduce the issue on
unstable.
What if you delay the network initialization within the guest to a bit
later?


> >> It keeps logging:
> >>
> >> [  360.652715] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.652796] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.652835] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.652874] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.652910] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.652947] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.652983] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >> [  360.653026] vif vif-1-0 (unregistered net_device): Page still
> >> granted! Index: 9d
> >>
> >> The domain can't be destroyed:
> >>
> >> $ sudo xl list
> >> Name                                        ID   Mem VCPUs      State   Time(s)
> >> Domain-0                                     0   512     2     r-----     185.7
> >> (null)                                      10     0     1     --ps-d       0.1
> >>
> >> top shows the "xenwatch" process using 100% CPU and I have to reboot.
> >>
> >> A work-around is to use:
> >>
> >>   on_poweroff = 'preserve'
> >
> > Does the "Page still granted!" message go away after a while, or just
> > keeps printing it?
> 
> It just keeps printing it.
> 
> 
> -- 
> Dr Thomas Leonard        http://0install.net/
> GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
> GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA
> 

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 14:46, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> On 6 August 2014 14:35, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
>> >> > On 1 August 2014 18:16, Stefano Stabellini
>> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >>> On 1 August 2014 18:01, Stefano Stabellini
>> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
>> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >>> >> >> > > > On 24/07/14 14:30, 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.
>> >> >>> >> >> > > >
>> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >> >>> >> >> > > > Mirage too
>> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> >> [...]
>> >> >> Checkout the branch grant_map_identity_4.4_2
>> >> >
>> >> > Thanks - it's working now!
>> >>
>> >> Update: if my VM uses the network and then exits quickly, dom0 gets
>> >> stuck.
>> >
>> > What do you mean by exit quickly? xl destroy domain?
>>
>> The guest starts, sends a few packets and then exits (possibly before
>> the packets have been transmitted). This happens before "xl create"
>> manages to attach to the console.
>
> I haven't seen anything like this and I cannot reproduce the issue on
> unstable.

Here's a test case (tested on stable-4.4 with your patches:
https://github.com/talex5/xen/commits/stable-4.4):

$ wget http://test.roscidus.com/static/breaks.xen

$ cat > breaks.xl << EOF
name = 'breaks'
kernel = 'breaks.xen'
builder = 'linux'
memory = 256
on_crash = 'preserve'
vif = [ 'mac=c0:ff:ee:c0:ff:ee,bridge=br0' ]
EOF

$ sudo xl create -c breaks.xl
Parsing config from breaks.xl
xenconsole: Could not read tty from store: No such file or directory
libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console
child [0] exited with error status 2

$ sudo xl list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   512     2     r-----     335.1
(null)                                       8     0     1     --ps-d       0.1


> What if you delay the network initialization within the guest to a bit
> later?

Yes, it's fine with a delay before using the network.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Wei Liu]

On Wed, Aug 06, 2014 at 02:39:34PM +0100, Thomas Leonard wrote:
[...]
> >> >> Checkout the branch grant_map_identity_4.4_2
> >> >
> >> > Thanks - it's working now!
> >>
> >> Update: if my VM uses the network and then exits quickly, dom0 gets
> >> stuck.
> >
> > What do you mean by exit quickly? xl destroy domain?
> 
> The guest starts, sends a few packets and then exits (possibly before
> the packets have been transmitted). This happens before "xl create"
> manages to attach to the console.
> 

Are you using mirage DomU? I guess it takes much shorter time to bootup
/ shutdown than a normal Linux guest?

The packets definite hit backend at that time, otherwise you would not
have seen that error message.

Using console attach to determine what state guest is in is inacurate as
it's asynchronised.

My preliminary analysis is as followed:
  1. DomU transmit a packet
  2. packet received and mapped in Dom0
  3. packet goes into Dom0 stack
  4. DomU exits, xenbus thread in Dom0 stops processing threads
  5. packet processed in Dom0 stack, netback puts grefs of
     that packets in dealloc_ring
  6. Dom0 waits for all grefs to be unmapped before freeing up
     resources
As the thread to unmap grefs is stopped in step 4, now Dom0 waits
forever in step 6. :-(

Normally a DomU won't exit this fast so the sequence is actually 123564.

Wei.

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Wed, 6 Aug 2014, Thomas Leonard wrote:
> On 6 August 2014 14:46, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
> >> On 6 August 2014 14:35, Stefano Stabellini
> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
> >> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
> >> >> > On 1 August 2014 18:16, Stefano Stabellini
> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> >>> On 1 August 2014 18:01, Stefano Stabellini
> >> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
> >> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
> >> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
> >> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
> >> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> >>> >> >> > > > On 24/07/14 14:30, 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.
> >> >> >>> >> >> > > >
> >> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> >> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
> >> >> >>> >> >> > > > Mirage too
> >> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> >> >> [...]
> >> >> >> Checkout the branch grant_map_identity_4.4_2
> >> >> >
> >> >> > Thanks - it's working now!
> >> >>
> >> >> Update: if my VM uses the network and then exits quickly, dom0 gets
> >> >> stuck.
> >> >
> >> > What do you mean by exit quickly? xl destroy domain?
> >>
> >> The guest starts, sends a few packets and then exits (possibly before
> >> the packets have been transmitted). This happens before "xl create"
> >> manages to attach to the console.
> >
> > I haven't seen anything like this and I cannot reproduce the issue on
> > unstable.
> 
> Here's a test case (tested on stable-4.4 with your patches:
> https://github.com/talex5/xen/commits/stable-4.4):
> 
> $ wget http://test.roscidus.com/static/breaks.xen
> 
> $ cat > breaks.xl << EOF
> name = 'breaks'
> kernel = 'breaks.xen'
> builder = 'linux'
> memory = 256
> on_crash = 'preserve'
> vif = [ 'mac=c0:ff:ee:c0:ff:ee,bridge=br0' ]
> EOF
> 
> $ sudo xl create -c breaks.xl
> Parsing config from breaks.xl
> xenconsole: Could not read tty from store: No such file or directory
> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console
> child [0] exited with error status 2

I cannot repro the issue even using "breaks.xen" as guest kernel.
What dom0 kernel are you using?


> $ sudo xl list
> Name                                        ID   Mem VCPUs      State   Time(s)
> Domain-0                                     0   512     2     r-----     335.1
> (null)                                       8     0     1     --ps-d       0.1
> 
> 
> > What if you delay the network initialization within the guest to a bit
> > later?
> 
> Yes, it's fine with a delay before using the network.

Very interesting.

> 
> -- 
> Dr Thomas Leonard        http://0install.net/
> GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
> GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA
> 

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 15:14, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> On 6 August 2014 14:46, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> On 6 August 2014 14:35, Stefano Stabellini
>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
>> >> >> > On 1 August 2014 18:16, Stefano Stabellini
>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >>> On 1 August 2014 18:01, Stefano Stabellini
>> >> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
>> >> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >>> >> >> > > > On 24/07/14 14:30, 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.
>> >> >> >>> >> >> > > >
>> >> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >> >> >>> >> >> > > > Mirage too
>> >> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> >> >> [...]
>> >> >> >> Checkout the branch grant_map_identity_4.4_2
>> >> >> >
>> >> >> > Thanks - it's working now!
>> >> >>
>> >> >> Update: if my VM uses the network and then exits quickly, dom0 gets
>> >> >> stuck.
>> >> >
>> >> > What do you mean by exit quickly? xl destroy domain?
>> >>
>> >> The guest starts, sends a few packets and then exits (possibly before
>> >> the packets have been transmitted). This happens before "xl create"
>> >> manages to attach to the console.
>> >
>> > I haven't seen anything like this and I cannot reproduce the issue on
>> > unstable.
>>
>> Here's a test case (tested on stable-4.4 with your patches:
>> https://github.com/talex5/xen/commits/stable-4.4):
>>
>> $ wget http://test.roscidus.com/static/breaks.xen
>>
>> $ cat > breaks.xl << EOF
>> name = 'breaks'
>> kernel = 'breaks.xen'
>> builder = 'linux'
>> memory = 256
>> on_crash = 'preserve'
>> vif = [ 'mac=c0:ff:ee:c0:ff:ee,bridge=br0' ]
>> EOF
>>
>> $ sudo xl create -c breaks.xl
>> Parsing config from breaks.xl
>> xenconsole: Could not read tty from store: No such file or directory
>> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console
>> child [0] exited with error status 2
>
> I cannot repro the issue even using "breaks.xen" as guest kernel.
> What dom0 kernel are you using?

It's Linux 3.16-rc7, plus your patches:

  https://github.com/talex5/linux/commits/master

I'm testing on a CubieTruck, using this SD card image (includes
patched Linux and Xen):

  http://blobs.openmirage.org/cubietruck-xen-iso.tar.bz2

And yes, it's a Mirage unikernel.

>> $ sudo xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   512     2     r-----     335.1
>> (null)                                       8     0     1     --ps-d       0.1
>>
>>
>> > What if you delay the network initialization within the guest to a bit
>> > later?
>>
>> Yes, it's fine with a delay before using the network.
>
> Very interesting.




-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 15:14, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> On 6 August 2014 14:46, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> On 6 August 2014 14:35, Stefano Stabellini
>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
>> >> >> > On 1 August 2014 18:16, Stefano Stabellini
>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >>> On 1 August 2014 18:01, Stefano Stabellini
>> >> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
>> >> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >>> >> >> > > > On 24/07/14 14:30, 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.
>> >> >> >>> >> >> > > >
>> >> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >> >> >>> >> >> > > > Mirage too
>> >> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> >> >> [...]
>> >> >> >> Checkout the branch grant_map_identity_4.4_2
>> >> >> >
>> >> >> > Thanks - it's working now!
>> >> >>
>> >> >> Update: if my VM uses the network and then exits quickly, dom0 gets
>> >> >> stuck.
>> >> >
>> >> > What do you mean by exit quickly? xl destroy domain?
>> >>
>> >> The guest starts, sends a few packets and then exits (possibly before
>> >> the packets have been transmitted). This happens before "xl create"
>> >> manages to attach to the console.
>> >
>> > I haven't seen anything like this and I cannot reproduce the issue on
>> > unstable.
>>
>> Here's a test case (tested on stable-4.4 with your patches:
>> https://github.com/talex5/xen/commits/stable-4.4):
>>
>> $ wget http://test.roscidus.com/static/breaks.xen
>>
>> $ cat > breaks.xl << EOF
>> name = 'breaks'
>> kernel = 'breaks.xen'
>> builder = 'linux'
>> memory = 256
>> on_crash = 'preserve'
>> vif = [ 'mac=c0:ff:ee:c0:ff:ee,bridge=br0' ]
>> EOF
>>
>> $ sudo xl create -c breaks.xl
>> Parsing config from breaks.xl
>> xenconsole: Could not read tty from store: No such file or directory
>> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console
>> child [0] exited with error status 2
>
> I cannot repro the issue even using "breaks.xen" as guest kernel.
> What dom0 kernel are you using?
>
>
>> $ sudo xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   512     2     r-----     335.1
>> (null)                                       8     0     1     --ps-d       0.1
>>
>>
>> > What if you delay the network initialization within the guest to a bit
>> > later?
>>
>> Yes, it's fine with a delay before using the network.
>
> Very interesting.

Note: the delay is before my code sends a packet, but Mirage might be
doing some DHCP stuff before that.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Stefano Stabellini]

On Wed, 6 Aug 2014, Thomas Leonard wrote:
> On 6 August 2014 15:14, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
> >> On 6 August 2014 14:46, Stefano Stabellini
> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
> >> >> On 6 August 2014 14:35, Stefano Stabellini
> >> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
> >> >> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
> >> >> >> > On 1 August 2014 18:16, Stefano Stabellini
> >> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> >> >>> On 1 August 2014 18:01, Stefano Stabellini
> >> >> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
> >> >> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
> >> >> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
> >> >> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
> >> >> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
> >> >> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
> >> >> >> >>> >> >> > > > On 24/07/14 14:30, 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.
> >> >> >> >>> >> >> > > >
> >> >> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
> >> >> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
> >> >> >> >>> >> >> > > > Mirage too
> >> >> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
> >> >> >> [...]
> >> >> >> >> Checkout the branch grant_map_identity_4.4_2
> >> >> >> >
> >> >> >> > Thanks - it's working now!
> >> >> >>
> >> >> >> Update: if my VM uses the network and then exits quickly, dom0 gets
> >> >> >> stuck.
> >> >> >
> >> >> > What do you mean by exit quickly? xl destroy domain?
> >> >>
> >> >> The guest starts, sends a few packets and then exits (possibly before
> >> >> the packets have been transmitted). This happens before "xl create"
> >> >> manages to attach to the console.
> >> >
> >> > I haven't seen anything like this and I cannot reproduce the issue on
> >> > unstable.
> >>
> >> Here's a test case (tested on stable-4.4 with your patches:
> >> https://github.com/talex5/xen/commits/stable-4.4):
> >>
> >> $ wget http://test.roscidus.com/static/breaks.xen
> >>
> >> $ cat > breaks.xl << EOF
> >> name = 'breaks'
> >> kernel = 'breaks.xen'
> >> builder = 'linux'
> >> memory = 256
> >> on_crash = 'preserve'
> >> vif = [ 'mac=c0:ff:ee:c0:ff:ee,bridge=br0' ]
> >> EOF
> >>
> >> $ sudo xl create -c breaks.xl
> >> Parsing config from breaks.xl
> >> xenconsole: Could not read tty from store: No such file or directory
> >> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console
> >> child [0] exited with error status 2
> >
> > I cannot repro the issue even using "breaks.xen" as guest kernel.
> > What dom0 kernel are you using?
> 
> It's Linux 3.16-rc7, plus your patches:
> 
>   https://github.com/talex5/linux/commits/master

I realize that it's a bit old, but would you be able to test with a 3.14
dom0? I am asking because 3.14 doesn't need my patches to work properly,
so that would rule them out. You could also remove the corresponding Xen
series.



> I'm testing on a CubieTruck, using this SD card image (includes
> patched Linux and Xen):
> 
>   http://blobs.openmirage.org/cubietruck-xen-iso.tar.bz2
> 
> And yes, it's a Mirage unikernel.
> 
> >> $ sudo xl list
> >> Name                                        ID   Mem VCPUs      State   Time(s)
> >> Domain-0                                     0   512     2     r-----     335.1
> >> (null)                                       8     0     1     --ps-d       0.1
> >>
> >>
> >> > What if you delay the network initialization within the guest to a bit
> >> > later?
> >>
> >> Yes, it's fine with a delay before using the network.
> >
> > Very interesting.
> 
> 
> 
> 
> -- 
> Dr Thomas Leonard        http://0install.net/
> GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
> GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA
> 

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 15:27, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> On 6 August 2014 15:14, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> On 6 August 2014 14:46, Stefano Stabellini
>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> >> On 6 August 2014 14:35, Stefano Stabellini
>> >> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> > On Wed, 6 Aug 2014, Thomas Leonard wrote:
>> >> >> >> On 1 August 2014 19:12, Thomas Leonard <talex5@gmail.com> wrote:
>> >> >> >> > On 1 August 2014 18:16, Stefano Stabellini
>> >> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >> >> On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >> >>> On 1 August 2014 18:01, Stefano Stabellini
>> >> >> >> >>> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >> >>> > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >> >>> >> On 1 August 2014 17:25, Stefano Stabellini
>> >> >> >> >>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >> >>> >> > On Fri, 1 Aug 2014, Julien Grall wrote:
>> >> >> >> >>> >> >> On 01/08/14 17:16, Thomas Leonard wrote:
>> >> >> >> >>> >> >> > On 1 August 2014 16:13, Stefano Stabellini
>> >> >> >> >>> >> >> > <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> >> >>> >> >> > > On Fri, 1 Aug 2014, Thomas Leonard wrote:
>> >> >> >> >>> >> >> > > > On 24/07/14 14:30, 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.
>> >> >> >> >>> >> >> > > >
>> >> >> >> >>> >> >> > > > Is there a version of these patches for Xen 4.4 that I can test? The
>> >> >> >> >>> >> >> > > > restriction on duplicate pages is causing trouble for networking on
>> >> >> >> >>> >> >> > > > Mirage too
>> >> >> >> >>> >> >> > > > (http://roscidus.com/blog/blog/2014/07/28/my-first-unikernel/#tcp-retransmissions).
>> >> >> >> [...]
>> >> >> >> >> Checkout the branch grant_map_identity_4.4_2
>> >> >> >> >
>> >> >> >> > Thanks - it's working now!
>> >> >> >>
>> >> >> >> Update: if my VM uses the network and then exits quickly, dom0 gets
>> >> >> >> stuck.
>> >> >> >
>> >> >> > What do you mean by exit quickly? xl destroy domain?
>> >> >>
>> >> >> The guest starts, sends a few packets and then exits (possibly before
>> >> >> the packets have been transmitted). This happens before "xl create"
>> >> >> manages to attach to the console.
>> >> >
>> >> > I haven't seen anything like this and I cannot reproduce the issue on
>> >> > unstable.
>> >>
>> >> Here's a test case (tested on stable-4.4 with your patches:
>> >> https://github.com/talex5/xen/commits/stable-4.4):
>> >>
>> >> $ wget http://test.roscidus.com/static/breaks.xen
>> >>
>> >> $ cat > breaks.xl << EOF
>> >> name = 'breaks'
>> >> kernel = 'breaks.xen'
>> >> builder = 'linux'
>> >> memory = 256
>> >> on_crash = 'preserve'
>> >> vif = [ 'mac=c0:ff:ee:c0:ff:ee,bridge=br0' ]
>> >> EOF
>> >>
>> >> $ sudo xl create -c breaks.xl
>> >> Parsing config from breaks.xl
>> >> xenconsole: Could not read tty from store: No such file or directory
>> >> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console
>> >> child [0] exited with error status 2
>> >
>> > I cannot repro the issue even using "breaks.xen" as guest kernel.
>> > What dom0 kernel are you using?
>>
>> It's Linux 3.16-rc7, plus your patches:
>>
>>   https://github.com/talex5/linux/commits/master
>
> I realize that it's a bit old, but would you be able to test with a 3.14
> dom0? I am asking because 3.14 doesn't need my patches to work properly,
> so that would rule them out. You could also remove the corresponding Xen
> series.

It's not easy, because 3.14 doesn't have the mmc driver.

>> I'm testing on a CubieTruck, using this SD card image (includes
>> patched Linux and Xen):
>>
>>   http://blobs.openmirage.org/cubietruck-xen-iso.tar.bz2
>>
>> And yes, it's a Mirage unikernel.
>>
>> >> $ sudo xl list
>> >> Name                                        ID   Mem VCPUs      State   Time(s)
>> >> Domain-0                                     0   512     2     r-----     335.1
>> >> (null)                                       8     0     1     --ps-d       0.1
>> >>
>> >>
>> >> > What if you delay the network initialization within the guest to a bit
>> >> > later?
>> >>
>> >> Yes, it's fine with a delay before using the network.
>> >
>> > Very interesting.
>>
>>
>>
>>
>> --
>> Dr Thomas Leonard        http://0install.net/
>> GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
>> GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA
>>



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Wei Liu]

Thomas, can you give this patch a quick try? I only compile test it
because I don't have handy testing environment at the moment.

Apply it to Dom0 kernel.

---8<---
>From b32aa53ba8b79f0006780549953510930513a3ac Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 6 Aug 2014 16:50:06 +0100
Subject: [PATCH] xen-netback: don't stop kthreads until all in-flight packets
 are processed

Reference count the number of packets in host stack, so that we don't
stop the deallocation thread too early. If not, we can end up with
xenvif_free permanently waiting for deallocation thread to unmap grefs.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/common.h    |    5 +++++
 drivers/net/xen-netback/interface.c |   16 ++++++++++++++++
 drivers/net/xen-netback/netback.c   |    2 ++
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 2532ce8..e2b5734 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -164,6 +164,8 @@ struct xenvif_queue { /* Per-queue data for xenvif */
 	u16 dealloc_ring[MAX_PENDING_REQS];
 	struct task_struct *dealloc_task;
 	wait_queue_head_t dealloc_wq;
+	atomic_t inflight_packets; /* number of packets in host network stack */
+	wait_queue_head_t inflight_wq;
 
 	/* Use kthread for guest RX */
 	struct task_struct *task;
@@ -297,6 +299,9 @@ static inline pending_ring_idx_t nr_pending_reqs(struct xenvif_queue *queue)
 /* Callback from stack when TX packet can be released */
 void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success);
 
+void xenvif_inc_inflight_packets(struct xenvif_queue *queue);
+void xenvif_dec_inflight_packets(struct xenvif_queue *queue);
+
 extern bool separate_tx_rx_irq;
 
 extern unsigned int rx_drain_timeout_msecs;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 91dad80..84aa863 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -43,6 +43,17 @@
 #define XENVIF_QUEUE_LENGTH 32
 #define XENVIF_NAPI_WEIGHT  64
 
+void xenvif_inc_inflight_packets(struct xenvif_queue *queue)
+{
+	atomic_inc(&queue->inflight_packets);
+}
+
+void xenvif_dec_inflight_packets(struct xenvif_queue *queue)
+{
+	if (atomic_dec_and_test(&queue->inflight_packets))
+		wake_up(&queue->inflight_wq);
+}
+
 static inline void xenvif_stop_queue(struct xenvif_queue *queue)
 {
 	struct net_device *dev = queue->vif->dev;
@@ -548,6 +559,8 @@ int xenvif_connect(struct xenvif_queue *queue, unsigned long tx_ring_ref,
 
 	init_waitqueue_head(&queue->wq);
 	init_waitqueue_head(&queue->dealloc_wq);
+	init_waitqueue_head(&queue->inflight_wq);
+	atomic_set(&queue->inflight_packets, 0);
 
 	if (tx_evtchn == rx_evtchn) {
 		/* feature-split-event-channels == 0 */
@@ -662,6 +675,9 @@ void xenvif_disconnect(struct xenvif *vif)
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
 		queue = &vif->queues[queue_index];
 
+		wait_event(queue->inflight_wq,
+			   atomic_read(&queue->inflight_packets) == 0);
+
 		if (queue->task) {
 			del_timer_sync(&queue->wake_queue);
 			kthread_stop(queue->task);
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index c65b636..59c90619 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1645,6 +1645,7 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 			queue->stats.tx_zerocopy_sent++;
 		}
 
+		xenvif_inc_inflight_packets(queue);
 		netif_receive_skb(skb);
 	}
 
@@ -1681,6 +1682,7 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success)
 		queue->stats.tx_zerocopy_success++;
 	else
 		queue->stats.tx_zerocopy_fail++;
+	xenvif_dec_inflight_packets(queue);
 }
 
 static inline void xenvif_tx_dealloc_action(struct xenvif_queue *queue)
-- 
1.7.10.4

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 16:59, Wei Liu <wei.liu2@citrix.com> wrote:
> Thomas, can you give this patch a quick try? I only compile test it
> because I don't have handy testing environment at the moment.
>
> Apply it to Dom0 kernel.
>
> ---8<---
> From b32aa53ba8b79f0006780549953510930513a3ac Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Wed, 6 Aug 2014 16:50:06 +0100
> Subject: [PATCH] xen-netback: don't stop kthreads until all in-flight packets
>  are processed
>
> Reference count the number of packets in host stack, so that we don't
> stop the deallocation thread too early. If not, we can end up with
> xenvif_free permanently waiting for deallocation thread to unmap grefs.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

That stops the errors being printed, but I still have a domain I can't destroy:

$ xl list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   512     2     r-----      35.6
(null)                                       1     0     1     --ps-d       0.1

I do see this after a bit:

[  240.062339] INFO: task xenwatch:18 blocked for more than 120 seconds.
[  240.062404]       Not tainted 3.16.0-rc7-00004-g9d89db9 #1
[  240.062426] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.

The load average is 1.

If I try to create the domain again, I get:

$ xl create -c network.xl
Parsing config from network.xl
libxl: error: libxl_device.c:934:device_backend_callback: unable to
add device with path /local/domain/0/backend/vif/4/0
libxl: error: libxl_create.c:1226:domcreate_attach_vtpms: unable to
add nic devices
libxl: error: libxl_device.c:934:device_backend_callback: unable to
remove device with path /local/domain/0/backend/vif/4/0
libxl: error: libxl.c:1457:devices_destroy_cb: libxl__devices_destroy
failed for 4


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Wei Liu]

On Wed, Aug 06, 2014 at 06:50:04PM +0100, Thomas Leonard wrote:
> On 6 August 2014 16:59, Wei Liu <wei.liu2@citrix.com> wrote:
> > Thomas, can you give this patch a quick try? I only compile test it
> > because I don't have handy testing environment at the moment.
> >
> > Apply it to Dom0 kernel.
> >
> > ---8<---
> > From b32aa53ba8b79f0006780549953510930513a3ac Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Wed, 6 Aug 2014 16:50:06 +0100
> > Subject: [PATCH] xen-netback: don't stop kthreads until all in-flight packets
> >  are processed
> >
> > Reference count the number of packets in host stack, so that we don't
> > stop the deallocation thread too early. If not, we can end up with
> > xenvif_free permanently waiting for deallocation thread to unmap grefs.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> That stops the errors being printed, but I still have a domain I can't destroy:
> 
> $ xl list
> Name                                        ID   Mem VCPUs      State   Time(s)
> Domain-0                                     0   512     2     r-----      35.6
> (null)                                       1     0     1     --ps-d       0.1
> 
> I do see this after a bit:
> 
> [  240.062339] INFO: task xenwatch:18 blocked for more than 120 seconds.
> [  240.062404]       Not tainted 3.16.0-rc7-00004-g9d89db9 #1
> [  240.062426] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> 
> The load average is 1.
> 

If you do "ps aux | grep vif" do you see some kthreads? If so, what is
the status of these threads? What's the status of "xenwatch" thread?  It
looks like that xenbus thread hangs waiting to be woken up, which means
there's yet another exit point.

This problem involes at least four execution entities (softirq +
kthreads) so to be honest it's not straightforward to figure out what
goes wrong. I will take another stab as soon as possible.

Wei.

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 6 August 2014 21:46, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Aug 06, 2014 at 06:50:04PM +0100, Thomas Leonard wrote:
>> On 6 August 2014 16:59, Wei Liu <wei.liu2@citrix.com> wrote:
>> > Thomas, can you give this patch a quick try? I only compile test it
>> > because I don't have handy testing environment at the moment.
>> >
>> > Apply it to Dom0 kernel.
>> >
>> > ---8<---
>> > From b32aa53ba8b79f0006780549953510930513a3ac Mon Sep 17 00:00:00 2001
>> > From: Wei Liu <wei.liu2@citrix.com>
>> > Date: Wed, 6 Aug 2014 16:50:06 +0100
>> > Subject: [PATCH] xen-netback: don't stop kthreads until all in-flight packets
>> >  are processed
>> >
>> > Reference count the number of packets in host stack, so that we don't
>> > stop the deallocation thread too early. If not, we can end up with
>> > xenvif_free permanently waiting for deallocation thread to unmap grefs.
>> >
>> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>
>> That stops the errors being printed, but I still have a domain I can't destroy:
>>
>> $ xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   512     2     r-----      35.6
>> (null)                                       1     0     1     --ps-d       0.1
>>
>> I do see this after a bit:
>>
>> [  240.062339] INFO: task xenwatch:18 blocked for more than 120 seconds.
>> [  240.062404]       Not tainted 3.16.0-rc7-00004-g9d89db9 #1
>> [  240.062426] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
>> disables this message.
>>
>> The load average is 1.
>>
>
> If you do "ps aux | grep vif" do you see some kthreads? If so, what is
> the status of these threads?

root@cubietruck:~# xl list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   512     2     r-----      22.0
(null)                                       1     0     1     --ps-d       0.1

root@cubietruck:~# ps aux | grep vif
root      1189  0.0  0.0      0     0 ?        S    07:50   0:00
[vif1.0-q0-guest]
root      1190  0.0  0.0      0     0 ?        S    07:50   0:00
[vif1.0-q0-deall]
root      1242  0.0  0.2   3388  1052 pts/0    S+   07:51   0:00 grep
--color=auto vif

> What's the status of "xenwatch" thread?

root@cubietruck:~# ps aux | grep xenwatch
root        18  0.0  0.0      0     0 ?        D    07:49   0:00 [xenwatch]

root@cubietruck:~# cat /proc/18/wchan
xenvif_disconnect

>  It looks like that xenbus thread hangs waiting to be woken up, which means
> there's yet another exit point.
>
> This problem involes at least four execution entities (softirq +
> kthreads) so to be honest it's not straightforward to figure out what
> goes wrong. I will take another stab as soon as possible.
>
> Wei.



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Wei Liu]

On Thu, Aug 07, 2014 at 08:59:30AM +0100, Thomas Leonard wrote:
[...]
> >
> > If you do "ps aux | grep vif" do you see some kthreads? If so, what is
> > the status of these threads?
> 
> root@cubietruck:~# xl list
> Name                                        ID   Mem VCPUs      State   Time(s)
> Domain-0                                     0   512     2     r-----      22.0
> (null)                                       1     0     1     --ps-d       0.1
> 
> root@cubietruck:~# ps aux | grep vif
> root      1189  0.0  0.0      0     0 ?        S    07:50   0:00
> [vif1.0-q0-guest]
> root      1190  0.0  0.0      0     0 ?        S    07:50   0:00
> [vif1.0-q0-deall]
> root      1242  0.0  0.2   3388  1052 pts/0    S+   07:51   0:00 grep
> --color=auto vif
> 
> > What's the status of "xenwatch" thread?
> 
> root@cubietruck:~# ps aux | grep xenwatch
> root        18  0.0  0.0      0     0 ?        D    07:49   0:00 [xenwatch]
> 
> root@cubietruck:~# cat /proc/18/wchan
> xenvif_disconnect
> 

OK, so the ref-count is messed up. My patch is buggy.

Another simplier fix came to mind this morning. Can you give it a go?
I've run preliminary test on x86, this patch at least doesn't cause
regression for me.

Wei.

---8<---
>From 9b7db1c85798a0f25ce845815d20ff331aaf15c1 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Thu, 7 Aug 2014 11:33:37 +0100
Subject: [PATCH] xen-netback: don't stop dealloc kthread until all packets
 are processed

Move xenvif_wait_unmap_timeout before stopping dealloc thread, so that
we guarantee all inflight packets are eventually unmapped.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/interface.c |   30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 5f11d1d..ce871db 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -655,6 +655,20 @@ void xenvif_disconnect(struct xenvif *vif)
 	struct xenvif_queue *queue = NULL;
 	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
+	/* Here we want to avoid timeout messages if an skb can be legitimately
+	 * stuck somewhere else. Realistically this could be an another vif's
+	 * internal or QDisc queue. That another vif also has this
+	 * rx_drain_timeout_msecs timeout, but the timer only ditches the
+	 * internal queue. After that, the QDisc queue can put in worst case
+	 * XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS skbs into that another vif's
+	 * internal queue, so we need several rounds of such timeouts until we
+	 * can be sure that no another vif should have skb's from us. We are
+	 * not sending more skb's, so newly stuck packets are not interesting
+	 * for us here.
+	 */
+	unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000) *
+		DIV_ROUND_UP(XENVIF_QUEUE_LENGTH, (XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS));
+
 
 	if (netif_carrier_ok(vif->dev))
 		xenvif_carrier_off(vif);
@@ -673,6 +687,8 @@ void xenvif_disconnect(struct xenvif *vif)
 			queue->task = NULL;
 		}
 
+		xenvif_wait_unmap_timeout(queue, worst_case_skb_lifetime);
+
 		if (queue->dealloc_task) {
 			kthread_stop(queue->dealloc_task);
 			queue->dealloc_task = NULL;
@@ -706,25 +722,11 @@ void xenvif_free(struct xenvif *vif)
 	struct xenvif_queue *queue = NULL;
 	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
-	/* Here we want to avoid timeout messages if an skb can be legitimately
-	 * stuck somewhere else. Realistically this could be an another vif's
-	 * internal or QDisc queue. That another vif also has this
-	 * rx_drain_timeout_msecs timeout, but the timer only ditches the
-	 * internal queue. After that, the QDisc queue can put in worst case
-	 * XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS skbs into that another vif's
-	 * internal queue, so we need several rounds of such timeouts until we
-	 * can be sure that no another vif should have skb's from us. We are
-	 * not sending more skb's, so newly stuck packets are not interesting
-	 * for us here.
-	 */
-	unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000) *
-		DIV_ROUND_UP(XENVIF_QUEUE_LENGTH, (XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS));
 
 	unregister_netdev(vif->dev);
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
 		queue = &vif->queues[queue_index];
-		xenvif_wait_unmap_timeout(queue, worst_case_skb_lifetime);
 		xenvif_deinit_queue(queue);
 	}
 
-- 
1.7.10.4

Re: [PATCH v3 0/3] map grant refs at pfn = mfn

[Thomas Leonard]

On 7 August 2014 11:40, Wei Liu <wei.liu2@citrix.com> wrote:
> On Thu, Aug 07, 2014 at 08:59:30AM +0100, Thomas Leonard wrote:
> [...]
>> >
>> > If you do "ps aux | grep vif" do you see some kthreads? If so, what is
>> > the status of these threads?
>>
>> root@cubietruck:~# xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   512     2     r-----      22.0
>> (null)                                       1     0     1     --ps-d       0.1
>>
>> root@cubietruck:~# ps aux | grep vif
>> root      1189  0.0  0.0      0     0 ?        S    07:50   0:00
>> [vif1.0-q0-guest]
>> root      1190  0.0  0.0      0     0 ?        S    07:50   0:00
>> [vif1.0-q0-deall]
>> root      1242  0.0  0.2   3388  1052 pts/0    S+   07:51   0:00 grep
>> --color=auto vif
>>
>> > What's the status of "xenwatch" thread?
>>
>> root@cubietruck:~# ps aux | grep xenwatch
>> root        18  0.0  0.0      0     0 ?        D    07:49   0:00 [xenwatch]
>>
>> root@cubietruck:~# cat /proc/18/wchan
>> xenvif_disconnect
>>
>
> OK, so the ref-count is messed up. My patch is buggy.
>
> Another simplier fix came to mind this morning. Can you give it a go?
> I've run preliminary test on x86, this patch at least doesn't cause
> regression for me.

Seems to be working. I did get this on the console:

[  156.933721] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.933787] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.933823] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.933869] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.933903] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.933936] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.933969] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.934002] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.934035] vif vif-1-0 vif1.0: Page still granted! Index: bd
[  156.934069] vif vif-1-0 vif1.0: Page still granted! Index: bd

But the domU was cleaned up and the second one didn't generate any
output. The third gave:

[  209.226154] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226206] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226239] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226270] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226302] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226333] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226364] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226395] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226427] vif vif-3-0 vif3.0: Page still granted! Index: b7
[  209.226458] vif vif-3-0 vif3.0: Page still granted! Index: b7

Apart from displaying these messages, it seems to be working fine.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA