Xen devel - Dec 2013 - [PATCH 0/8] xen/arm: Handle correctly foreign mapping

Hello,

This patch series aims to fix "Failed to unmap" message in dom0 when a
guest
is creating. Actually it will result to leak dom0 memory.

The series is based on pvh dom0 v5 patch series
(http://www.gossamer-threads.com/lists/xen/devel/309493?page=last) from Mukesh.

    - Patch #1: this patch add support to remove correctly foreign page
    on ARM, if it''s possible I would like to merge with patch #6 of
Mukesh.
    - Patch #2: prepare work for the other patch
    - Patch #3-6: add support for p2m type
    - Patch #7: set p2m type for foreign page
    - Patch #8: it''s not really part of this serie. It adds support for
    read-only grant-mapping.

In the ideal world, this patch series and the pvh dom0 series should be mixed
to avoid to introduce dummy functions that will be removed later.

Sincerely yours,

Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>

Julien Grall (8):
  DO NOT APPLY: xen/arm: Correctly support foreign page removing on ARM
  xen/arm: move mfn_to_p2m_entry in arch/arm/p2m.c
  xen/arm: Implement p2m_type_t as an enum
  xen/arm: Store p2m type in each page of the guest
  xen/arm: p2m: Add p2m_get_entry
  xen/arm: Retrieve p2m type in get_page_from_gfn
  xen/arm: Set foreign page type to p2m_map_foreign
  xen/arm: grant-table: Support read-only mapping

 xen/arch/arm/mm.c          |   26 ++++++++-------
 xen/arch/arm/p2m.c         |   78 ++++++++++++++++++++++++++++++++++++--------
 xen/common/memory.c        |   12 ++++---
 xen/include/asm-arm/p2m.h  |   43 +++++++++++++++++-------
 xen/include/asm-arm/page.h |   22 -------------
 5 files changed, 117 insertions(+), 64 deletions(-)

-- 
1.7.10.4

This patch is here to fix "phv dom0: Add and remove foreign pages", I
hope it
will be merge to this patch in a later version.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/common/memory.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index ae11828..dfda092 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -708,19 +708,21 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
         {
             if ( page )
                 mfn = page_to_mfn(page);
-#ifdef CONFIG_X86
             else
             {
-                p2m_type_t tp;
                 struct domain *foreign_dom;
+#ifdef CONFIG_X86
+                p2m_type_t tp;
 
                 mfn = mfn_x(get_gfn_query(d, xrfp.gpfn, &tp));
-                foreign_dom = page_get_owner(mfn_to_page(mfn));
                 ASSERT(is_pvh_domain(d));
-                ASSERT(d != foreign_dom);
                 ASSERT(p2m_is_foreign(tp));
-            }
+#else
+                mfn = gmfn_to_mfn(d, xrfp.gpfn);
 #endif
+                foreign_dom = page_get_owner(mfn_to_page(mfn));
+                ASSERT(d != foreign_dom);
+            }
             guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
             if (page)
                 put_page(page);
-- 
1.7.10.4

The function mfn_to_p2m_entry will be extended in a following patch to handle
p2m_type_t. It will break compilation because p2m_type_t is not defined
(interdependence between includes).
It''s easier to move the function in arch/arm/p2m.c and it''s
not harmful as the
function is only used in this file.
---
 xen/arch/arm/p2m.c         |   22 ++++++++++++++++++++++
 xen/include/asm-arm/page.h |   22 ----------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1d5c841..8f8b47e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -124,6 +124,28 @@ int p2m_pod_decrease_reservation(struct domain *d,
     return -ENOSYS;
 }
 
+static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
+{
+    paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
+    lpae_t e = (lpae_t) {
+        .p2m.xn = 0,
+        .p2m.af = 1,
+        .p2m.sh = LPAE_SH_OUTER,
+        .p2m.read = 1,
+        .p2m.write = 1,
+        .p2m.mattr = mattr,
+        .p2m.table = 1,
+        .p2m.valid = 1,
+    };
+
+    ASSERT(!(pa & ~PAGE_MASK));
+    ASSERT(!(pa & ~PADDR_MASK));
+
+    e.bits |= pa;
+
+    return e;
+}
+
 /* Allocate a new page table page and hook it in via the given entry */
 static int p2m_create_table(struct domain *d,
                             lpae_t *entry)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d468418..0625464 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -213,28 +213,6 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
     return e;
 }
 
-static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
-{
-    paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
-    lpae_t e = (lpae_t) {
-        .p2m.xn = 0,
-        .p2m.af = 1,
-        .p2m.sh = LPAE_SH_OUTER,
-        .p2m.write = 1,
-        .p2m.read = 1,
-        .p2m.mattr = mattr,
-        .p2m.table = 1,
-        .p2m.valid = 1,
-    };
-
-    ASSERT(!(pa & ~PAGE_MASK));
-    ASSERT(!(pa & ~PADDR_MASK));
-
-    e.bits |= pa;
-
-    return e;
-}
-
 #if defined(CONFIG_ARM_32)
 # include <asm/arm32/page.h>
 #elif defined(CONFIG_ARM_64)
-- 
1.7.10.4

Until now, Xen doesn''t know the type of the page (ram, foreign page,
mmio,...).
Introduce p2m_type_t with basic types:
    - p2m_invalid: Nothing is mapped here
    - p2m_ram_rw: Normal read/write guest RAM
    - p2m_ram_ro: Read-only guest RAM
    - p2m_mmio_direct: Read/write mapping of device memory
    - p2m_map_foreign: RAM page from foreign guest

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/asm-arm/p2m.h |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f079f00..b24f94a 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -20,6 +20,16 @@ struct p2m_domain {
     uint8_t vmid;
 };
 
+typedef enum {
+    p2m_invalid = 0,        /* Nothing mapped here */
+    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
+    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
+    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
+    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
+} p2m_type_t;
+
+#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
+
 /* Initialise vmid allocator */
 void p2m_vmid_allocator_init(void);
 
@@ -72,7 +82,6 @@ p2m_pod_decrease_reservation(struct domain *d,
                              unsigned int order);
 
 /* Look up a GFN and take a reference count on the backing page. */
-typedef int p2m_type_t;
 typedef unsigned int p2m_query_t;
 #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
 #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
@@ -110,8 +119,6 @@ static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
-#define p2m_is_foreign(_t) (0)
-
 #endif /* _XEN_P2M_H */
 
 /*
-- 
1.7.10.4

Use the field ''avail'' to store the type of the page. This
information will be
retrieved in a future patch to change the behaviour when the page is removed.

Also introduce guest_physmap_add_entry to map and set a specific p2m type for
a page.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c        |   49 +++++++++++++++++++++++++++++++--------------
 xen/include/asm-arm/p2m.h |   18 +++++++++++++----
 2 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8f8b47e..5449a35 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
     return -ENOSYS;
 }
 
-static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
+static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
+                               p2m_type_t t)
 {
     paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
     lpae_t e = (lpae_t) {
@@ -132,12 +133,25 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned
int mattr)
         .p2m.af = 1,
         .p2m.sh = LPAE_SH_OUTER,
         .p2m.read = 1,
-        .p2m.write = 1,
         .p2m.mattr = mattr,
         .p2m.table = 1,
         .p2m.valid = 1,
+        .p2m.avail = t, /* Use avail to store p2m type */
     };
 
+    switch (t)
+    {
+    case p2m_ram_rw:
+    case p2m_mmio_direct:
+    case p2m_map_foreign:
+        e.p2m.write = 1;
+        break;
+    case p2m_invalid:
+    case p2m_ram_ro:
+    default:
+        e.p2m.write = 0;
+    }
+
     ASSERT(!(pa & ~PAGE_MASK));
     ASSERT(!(pa & ~PADDR_MASK));
 
@@ -167,7 +181,7 @@ static int p2m_create_table(struct domain *d,
     clear_page(p);
     unmap_domain_page(p);
 
-    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM);
+    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
 
     write_pte(entry, pte);
 
@@ -185,7 +199,8 @@ static int create_p2m_entries(struct domain *d,
                      paddr_t start_gpaddr,
                      paddr_t end_gpaddr,
                      paddr_t maddr,
-                     int mattr)
+                     int mattr,
+                     p2m_type_t t)
 {
     int rc, flush;
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -260,14 +275,15 @@ static int create_p2m_entries(struct domain *d,
                         goto out;
                     }
 
-                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr);
+                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
 
                     write_pte(&third[third_table_offset(addr)], pte);
                 }
                 break;
             case INSERT:
                 {
-                    lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT,
mattr);
+                    lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT,
+                                                  mattr, t);
                     write_pte(&third[third_table_offset(addr)], pte);
                     maddr += PAGE_SIZE;
                 }
@@ -302,7 +318,8 @@ int p2m_populate_ram(struct domain *d,
                      paddr_t start,
                      paddr_t end)
 {
-    return create_p2m_entries(d, ALLOCATE, start, end, 0, MATTR_MEM);
+    return create_p2m_entries(d, ALLOCATE, start, end,
+                              0, MATTR_MEM, p2m_ram_rw);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -310,18 +327,20 @@ int map_mmio_regions(struct domain *d,
                      paddr_t end_gaddr,
                      paddr_t maddr)
 {
-    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr, maddr,
MATTR_DEV);
+    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr,
+                              maddr, MATTR_DEV, p2m_mmio_direct);
 }
 
-int guest_physmap_add_page(struct domain *d,
-                           unsigned long gpfn,
-                           unsigned long mfn,
-                           unsigned int page_order)
+int guest_physmap_add_entry(struct domain *d,
+                            unsigned long gpfn,
+                            unsigned long mfn,
+                            unsigned long page_order,
+                            p2m_type_t t)
 {
     return create_p2m_entries(d, INSERT,
                               pfn_to_paddr(gpfn),
-                              pfn_to_paddr(gpfn + (1<<page_order)),
-                              pfn_to_paddr(mfn), MATTR_MEM);
+                              pfn_to_paddr(gpfn + (1 << page_order)),
+                              pfn_to_paddr(mfn), MATTR_MEM, t);
 }
 
 void guest_physmap_remove_page(struct domain *d,
@@ -331,7 +350,7 @@ void guest_physmap_remove_page(struct domain *d,
     create_p2m_entries(d, REMOVE,
                        pfn_to_paddr(gpfn),
                        pfn_to_paddr(gpfn + (1<<page_order)),
-                       pfn_to_paddr(mfn), MATTR_MEM);
+                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
 }
 
 int p2m_alloc_table(struct domain *d)
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b24f94a..1cf7d36 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -59,11 +59,21 @@ int p2m_populate_ram(struct domain *d, paddr_t start,
paddr_t end);
 int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
                      paddr_t end_gaddr, paddr_t maddr);
 
+int guest_physmap_add_entry(struct domain *d,
+                            unsigned long gfn,
+                            unsigned long mfn,
+                            unsigned long page_order,
+                            p2m_type_t t);
+
 /* Untyped version for RAM only, for compatibility */
-int guest_physmap_add_page(struct domain *d,
-                           unsigned long gfn,
-                           unsigned long mfn,
-                           unsigned int page_order);
+static inline int guest_physmap_add_page(struct domain *d,
+                                         unsigned long gfn,
+                                         unsigned long mfn,
+                                         unsigned int page_order)
+{
+    return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
+}
+
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order);
-- 
1.7.10.4

Reuse the code of p2m_lookup and add an argument to retrieve the p2m type.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c        |   11 ++++++++++-
 xen/include/asm-arm/p2m.h |    7 ++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5449a35..1ae2521 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -71,11 +71,17 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t
addr)
  * There are no processor functions to do a stage 2 only lookup therefore we
  * do a a software walk.
  */
-paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
+paddr_t p2m_get_entry(struct domain *d, paddr_t paddr, p2m_type_t *t)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
     paddr_t maddr = INVALID_PADDR;
+    p2m_type_t _t;
+
+    /* Allow t to be NULL */
+    t = t ?: &_t;
+
+    *t = p2m_invalid;
 
     spin_lock(&p2m->lock);
 
@@ -99,7 +105,10 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
 
 done:
     if ( pte.p2m.valid )
+    {
         maddr = (pte.bits & PADDR_MASK & PAGE_MASK) | (paddr &
~PAGE_MASK);
+        *t = pte.p2m.avail;
+    }
 
     if (third) unmap_domain_page(third);
     if (second) unmap_domain_page(second);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 1cf7d36..3de69c4 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -49,7 +49,12 @@ int p2m_alloc_table(struct domain *d);
 void p2m_load_VTTBR(struct domain *d);
 
 /* Look up the MFN corresponding to a domain''s PFN. */
-paddr_t p2m_lookup(struct domain *d, paddr_t gpfn);
+paddr_t p2m_get_entry(struct domain *d, paddr_t gpfn, p2m_type_t *t);
+
+static inline paddr_t p2m_lookup(struct domain *d, paddr_t gpfn)
+{
+    return p2m_get_entry(d, gpfn, NULL);
+}
 
 /* Setup p2m RAM mapping for domain d from start-end. */
 int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-- 
1.7.10.4

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/asm-arm/p2m.h |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3de69c4..54d1dab 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
     struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
     struct page_info *page;
-    unsigned long mfn = gmfn_to_mfn(d, gfn);
-
-    ASSERT(t == NULL);
+    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
+    unsigned long mfn = maddr >> PAGE_SHIFT;
 
     if (!mfn_valid(mfn))
         return NULL;
-- 
1.7.10.4

Xen needs to know that the current page belongs to another domain. Also take
the refcount to this page.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/mm.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 960c872..bf383a7 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -977,6 +977,7 @@ static int xenmem_add_to_physmap_one(
 {
     unsigned long mfn = 0;
     int rc;
+    p2m_type_t t = p2m_ram_rw;
 
     switch ( space )
     {
@@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
         break;
     case XENMAPSPACE_gmfn_foreign:
     {
-        paddr_t maddr;
         struct domain *od;
+        struct page_info *page;
         od = rcu_lock_domain_by_any_id(foreign_domid);
         if ( od == NULL )
             return -ESRCH;
@@ -1032,15 +1033,18 @@ static int xenmem_add_to_physmap_one(
             return rc;
         }
 
-        maddr = p2m_lookup(od, pfn_to_paddr(idx));
-        if ( maddr == INVALID_PADDR )
+        /* Take refcount to the foreign domain page.
+         * Refcount will be release in XENMEM_remove_from_physmap */
+        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
+        if ( !page )
         {
             dump_p2m_lookup(od, pfn_to_paddr(idx));
             rcu_unlock_domain(od);
             return -EINVAL;
         }
 
-        mfn = maddr >> PAGE_SHIFT;
+        mfn = page_to_mfn(page);
+        t = p2m_map_foreign;
 
         rcu_unlock_domain(od);
         break;
@@ -1051,7 +1055,7 @@ static int xenmem_add_to_physmap_one(
     }
 
     /* Map at new location. */
-    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
+    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
 
     return rc;
 }
-- 
1.7.10.4

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/mm.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index bf383a7..8e190ec 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1277,19 +1277,17 @@ int create_grant_host_mapping(unsigned long addr,
unsigned long frame,
                               unsigned int flags, unsigned int cache_flags)
 {
     int rc;
+    p2m_type_t t = p2m_ram_rw;
 
     if ( cache_flags  || (flags & ~GNTMAP_readonly) != GNTMAP_host_map )
         return GNTST_general_error;
 
-    /* XXX: read only mappings */
     if ( flags & GNTMAP_readonly )
-    {
-        gdprintk(XENLOG_WARNING, "read only mappings not implemented
yet\n");
-        return GNTST_general_error;
-    }
+        t = p2m_ram_ro;
+
+    rc = guest_physmap_add_entry(current->domain, addr >> PAGE_SHIFT,
+                                 frame, 0, t);
 
-    rc = guest_physmap_add_page(current->domain,
-                                 addr >> PAGE_SHIFT, frame, 0);
     if ( rc )
         return GNTST_general_error;
     else
-- 
1.7.10.4

(Use the right address for Tim)

On 12/05/2013 03:42 PM, Julien Grall wrote:
> Hello,
>
> This patch series aims to fix "Failed to unmap" message in dom0
when a guest
> is creating. Actually it will result to leak dom0 memory.
>
> The series is based on pvh dom0 v5 patch series
> (http://www.gossamer-threads.com/lists/xen/devel/309493?page=last) from
Mukesh.
>
>      - Patch #1: this patch add support to remove correctly foreign page
>      on ARM, if it''s possible I would like to merge with patch #6
of Mukesh.
>      - Patch #2: prepare work for the other patch
>      - Patch #3-6: add support for p2m type
>      - Patch #7: set p2m type for foreign page
>      - Patch #8: it''s not really part of this serie. It adds
support for
>      read-only grant-mapping.
>
> In the ideal world, this patch series and the pvh dom0 series should be
mixed
> to avoid to introduce dummy functions that will be removed later.
>
> Sincerely yours,
>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
>
> Julien Grall (8):
>    DO NOT APPLY: xen/arm: Correctly support foreign page removing on ARM
>    xen/arm: move mfn_to_p2m_entry in arch/arm/p2m.c
>    xen/arm: Implement p2m_type_t as an enum
>    xen/arm: Store p2m type in each page of the guest
>    xen/arm: p2m: Add p2m_get_entry
>    xen/arm: Retrieve p2m type in get_page_from_gfn
>    xen/arm: Set foreign page type to p2m_map_foreign
>    xen/arm: grant-table: Support read-only mapping
>
>   xen/arch/arm/mm.c          |   26 ++++++++-------
>   xen/arch/arm/p2m.c         |   78
++++++++++++++++++++++++++++++++++++--------
>   xen/common/memory.c        |   12 ++++---
>   xen/include/asm-arm/p2m.h  |   43 +++++++++++++++++-------
>   xen/include/asm-arm/page.h |   22 -------------
>   5 files changed, 117 insertions(+), 64 deletions(-)
>
-- 
Julien Grall

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> The function mfn_to_p2m_entry will be extended in a following patch to
handle
> p2m_type_t. It will break compilation because p2m_type_t is not defined
> (interdependence between includes).
> It''s easier to move the function in arch/arm/p2m.c and
it''s not harmful as the
> function is only used in this file.
You forgot your S-o-b.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

On 12/05/2013 03:47 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> The function mfn_to_p2m_entry will be extended in a following patch to
handle
>> p2m_type_t. It will break compilation because p2m_type_t is not defined
>> (interdependence between includes).
>> It''s easier to move the function in arch/arm/p2m.c and
it''s not harmful as the
>> function is only used in this file.
>
> You forgot your S-o-b.
Oh, right:

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

> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>
-- 
Julien Grall

On 05.12.13 16:42, Julien Grall wrote:
> Until now, Xen doesn''t know the type of the page (ram, foreign
page, mmio,...).
> Introduce p2m_type_t with basic types:
>     - p2m_invalid: Nothing is mapped here
>     - p2m_ram_rw: Normal read/write guest RAM
>     - p2m_ram_ro: Read-only guest RAM
>     - p2m_mmio_direct: Read/write mapping of device memory
>     - p2m_map_foreign: RAM page from foreign guest
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f079f00..b24f94a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -20,6 +20,16 @@ struct p2m_domain {
>      uint8_t vmid;
>  };
>  
> +typedef enum {
> +    p2m_invalid = 0,        /* Nothing mapped here */
> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> +} p2m_type_t;
> +
> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> +
Is it possible to merge p2m_type_t with x86 and move into common code?

Christoph

>  /* Initialise vmid allocator */
>  void p2m_vmid_allocator_init(void);
>  
> @@ -72,7 +82,6 @@ p2m_pod_decrease_reservation(struct domain *d,
>                               unsigned int order);
>  
>  /* Look up a GFN and take a reference count on the backing page. */
> -typedef int p2m_type_t;
>  typedef unsigned int p2m_query_t;
>  #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries
*/
>  #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
> @@ -110,8 +119,6 @@ static inline int get_page_and_type(struct page_info
*page,
>      return rc;
>  }
>  
> -#define p2m_is_foreign(_t) (0)
> -
>  #endif /* _XEN_P2M_H */
>  
>  /*
>

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Until now, Xen doesn''t know the type of the page (ram, foreign
page, mmio,...).
> Introduce p2m_type_t with basic types:
>     - p2m_invalid: Nothing is mapped here
Do we really need this? Is it not equivalent to not setting the present
bit? I see x86 has the same type though -- Tim can you explain why.

Since the avail bits in the p2m pte are in pretty short supply I think
we can avoid unnecessary types.
>     - p2m_ram_rw: Normal read/write guest RAM
>     - p2m_ram_ro: Read-only guest RAM
>     - p2m_mmio_direct: Read/write mapping of device memory
>     - p2m_map_foreign: RAM page from foreign guest
Is there no need for an entry for a grant mapping (and a ro
counterpart)?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f079f00..b24f94a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -20,6 +20,16 @@ struct p2m_domain {
>      uint8_t vmid;
>  };
>  
> +typedef enum {
> +    p2m_invalid = 0,        /* Nothing mapped here */
> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> +} p2m_type_t;
> +
> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> +
>  /* Initialise vmid allocator */
>  void p2m_vmid_allocator_init(void);
>  
> @@ -72,7 +82,6 @@ p2m_pod_decrease_reservation(struct domain *d,
>                               unsigned int order);
>  
>  /* Look up a GFN and take a reference count on the backing page. */
> -typedef int p2m_type_t;
>  typedef unsigned int p2m_query_t;
>  #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries
*/
>  #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
> @@ -110,8 +119,6 @@ static inline int get_page_and_type(struct page_info
*page,
>      return rc;
>  }
>  
> -#define p2m_is_foreign(_t) (0)
> -
>  #endif /* _XEN_P2M_H */
>  
>  /*

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Use the field ''avail'' to store the type of the page. This
information will be
> retrieved in a future patch to change the behaviour when the page is
removed.
> 
> Also introduce guest_physmap_add_entry to map and set a specific p2m type
for
> a page.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/p2m.c        |   49
+++++++++++++++++++++++++++++++--------------
>  xen/include/asm-arm/p2m.h |   18 +++++++++++++----
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8f8b47e..5449a35 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
>      return -ENOSYS;
>  }
>  
> -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
> +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> +                               p2m_type_t t)
>  {
>      paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>      lpae_t e = (lpae_t) {
> @@ -132,12 +133,25 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn,
unsigned int mattr)
>          .p2m.af = 1,
>          .p2m.sh = LPAE_SH_OUTER,
>          .p2m.read = 1,
> -        .p2m.write = 1,
>          .p2m.mattr = mattr,
>          .p2m.table = 1,
>          .p2m.valid = 1,
> +        .p2m.avail = t, /* Use avail to store p2m type */
Can we change the name in the struct instead and have a comment "using
avail bits for type" there instead please. We only have 5 types so could
only steal 3 but we may as well take all 4.

A BUILD_BUG_ON to check that the last p2m entry is < 2^4 would be good
too.

>      };
>  
> +    switch (t)
> +    {
> +    case p2m_ram_rw:
> +    case p2m_mmio_direct:
> +    case p2m_map_foreign:
> +        e.p2m.write = 1;
> +        break;
> +    case p2m_invalid:
> +    case p2m_ram_ro:
> +    default:
> +        e.p2m.write = 0;
> +    }
> +
>      ASSERT(!(pa & ~PAGE_MASK));
>      ASSERT(!(pa & ~PADDR_MASK));
>  
> @@ -167,7 +181,7 @@ static int p2m_create_table(struct domain *d,
>      clear_page(p);
>      unmap_domain_page(p);
>  
> -    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM);
> +    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
>  
>      write_pte(entry, pte);
>  
> @@ -185,7 +199,8 @@ static int create_p2m_entries(struct domain *d,
>                       paddr_t start_gpaddr,
>                       paddr_t end_gpaddr,
>                       paddr_t maddr,
> -                     int mattr)
> +                     int mattr,
> +                     p2m_type_t t)
>  {
>      int rc, flush;
>      struct p2m_domain *p2m = &d->arch.p2m;
> @@ -260,14 +275,15 @@ static int create_p2m_entries(struct domain *d,
>                          goto out;
>                      }
>  
> -                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr);
> +                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
>  
>                      write_pte(&third[third_table_offset(addr)], pte);
>                  }
>                  break;
>              case INSERT:
>                  {
> -                    lpae_t pte = mfn_to_p2m_entry(maddr >>
PAGE_SHIFT, mattr);
> +                    lpae_t pte = mfn_to_p2m_entry(maddr >>
PAGE_SHIFT,
> +                                                  mattr, t);
>                      write_pte(&third[third_table_offset(addr)], pte);
>                      maddr += PAGE_SIZE;
>                  }
> @@ -302,7 +318,8 @@ int p2m_populate_ram(struct domain *d,
>                       paddr_t start,
>                       paddr_t end)
>  {
> -    return create_p2m_entries(d, ALLOCATE, start, end, 0, MATTR_MEM);
> +    return create_p2m_entries(d, ALLOCATE, start, end,
> +                              0, MATTR_MEM, p2m_ram_rw);
>  }
>  
>  int map_mmio_regions(struct domain *d,
> @@ -310,18 +327,20 @@ int map_mmio_regions(struct domain *d,
>                       paddr_t end_gaddr,
>                       paddr_t maddr)
>  {
> -    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr, maddr,
MATTR_DEV);
> +    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr,
> +                              maddr, MATTR_DEV, p2m_mmio_direct);
>  }
>  
> -int guest_physmap_add_page(struct domain *d,
> -                           unsigned long gpfn,
> -                           unsigned long mfn,
> -                           unsigned int page_order)
> +int guest_physmap_add_entry(struct domain *d,
> +                            unsigned long gpfn,
> +                            unsigned long mfn,
> +                            unsigned long page_order,
> +                            p2m_type_t t)
>  {
>      return create_p2m_entries(d, INSERT,
>                                pfn_to_paddr(gpfn),
> -                              pfn_to_paddr(gpfn + (1<<page_order)),
> -                              pfn_to_paddr(mfn), MATTR_MEM);
> +                              pfn_to_paddr(gpfn + (1 <<
page_order)),
> +                              pfn_to_paddr(mfn), MATTR_MEM, t);
>  }
>  
>  void guest_physmap_remove_page(struct domain *d,
> @@ -331,7 +350,7 @@ void guest_physmap_remove_page(struct domain *d,
>      create_p2m_entries(d, REMOVE,
>                         pfn_to_paddr(gpfn),
>                         pfn_to_paddr(gpfn + (1<<page_order)),
> -                       pfn_to_paddr(mfn), MATTR_MEM);
> +                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
>  }
>  
>  int p2m_alloc_table(struct domain *d)
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index b24f94a..1cf7d36 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -59,11 +59,21 @@ int p2m_populate_ram(struct domain *d, paddr_t start,
paddr_t end);
>  int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
>                       paddr_t end_gaddr, paddr_t maddr);
>  
> +int guest_physmap_add_entry(struct domain *d,
> +                            unsigned long gfn,
> +                            unsigned long mfn,
> +                            unsigned long page_order,
> +                            p2m_type_t t);
> +
>  /* Untyped version for RAM only, for compatibility */
> -int guest_physmap_add_page(struct domain *d,
> -                           unsigned long gfn,
> -                           unsigned long mfn,
> -                           unsigned int page_order);
> +static inline int guest_physmap_add_page(struct domain *d,
> +                                         unsigned long gfn,
> +                                         unsigned long mfn,
> +                                         unsigned int page_order)
> +{
> +    return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
> +}
> +
>  void guest_physmap_remove_page(struct domain *d,
>                                 unsigned long gpfn,
>                                 unsigned long mfn, unsigned int
page_order);

On Thu, 2013-12-05 at 16:51 +0100, Egger, Christoph
wrote:
> On 05.12.13 16:42, Julien Grall wrote:
> > Until now, Xen doesn''t know the type of the page (ram,
foreign page, mmio,...).
> > Introduce p2m_type_t with basic types:
> >     - p2m_invalid: Nothing is mapped here
> >     - p2m_ram_rw: Normal read/write guest RAM
> >     - p2m_ram_ro: Read-only guest RAM
> >     - p2m_mmio_direct: Read/write mapping of device memory
> >     - p2m_map_foreign: RAM page from foreign guest
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > ---
> >  xen/include/asm-arm/p2m.h |   13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index f079f00..b24f94a 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -20,6 +20,16 @@ struct p2m_domain {
> >      uint8_t vmid;
> >  };
> >  
> > +typedef enum {
> > +    p2m_invalid = 0,        /* Nothing mapped here */
> > +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> > +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped
*/
> > +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO
area */
> > +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> > +} p2m_type_t;
> > +
> > +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> > +
> 
> Is it possible to merge p2m_type_t with x86 and move into common code?
Not really, the p2m handling is very different on the two arches, even
if they look superficially similar at this level.

x86 has more types than can fit in the available pte space on ARM for a
start.

I''d like to keep them separate please.

On 12/05/2013 03:52 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Until now, Xen doesn''t know the type of the page (ram, foreign
page, mmio,...).
>> Introduce p2m_type_t with basic types:
>>      - p2m_invalid: Nothing is mapped here
>
> Do we really need this? Is it not equivalent to not setting the present
> bit? I see x86 has the same type though -- Tim can you explain why.
We need a default value when Xen retrieves the p2m type. I don''t think 
we can assume that p2m_ram_rw (or any other type) is used by default.
> Since the avail bits in the p2m pte are in pretty short supply I think
> we can avoid unnecessary types.
I plan to use directly the decimal value. So we can store up to 16 values.
>>      - p2m_ram_rw: Normal read/write guest RAM
>>      - p2m_ram_ro: Read-only guest RAM
>>      - p2m_mmio_direct: Read/write mapping of device memory
>>      - p2m_map_foreign: RAM page from foreign guest
>
> Is there no need for an entry for a grant mapping (and a ro
> counterpart)?
Hmmm .. actually grant table is mapped as RAM (so read/write and 
execute). Do we want to allow code execution from grant-mapping page?
If not, then we will need to introduce specific p2m type from grant-mapping.

-- 
Julien Grall

On Thu, 2013-12-05 at 15:55 +0000, Ian Campbell wrote:
[....]

oops, hit send before I was done ;-)
> >      };
> >  
> > +    switch (t)
> > +    {
> > +    case p2m_ram_rw:
> > +    case p2m_mmio_direct:
> > +    case p2m_map_foreign:
> > +        e.p2m.write = 1;
> > +        break;
> > +    case p2m_invalid:
> > +    case p2m_ram_ro:
> > +    default:
> > +        e.p2m.write = 0;
> > +    }
> > +
> >      ASSERT(!(pa & ~PAGE_MASK));
> >      ASSERT(!(pa & ~PADDR_MASK));
> >  
> > @@ -167,7 +181,7 @@ static int p2m_create_table(struct domain *d,
> >      clear_page(p);
> >      unmap_domain_page(p);
> >  
> > -    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM);
> > +    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM,
p2m_invalid);
Is invalid really the right type here?

Are you using invalid for non-leaf pages? I think the table bit suffices
to identify them, doesn''t it?

Really the type field is only valid for the leafs ptes (including
superpage leafs).

If it is useful to have "pseudo-types" which don''t directly
correspond
to the 4 available bits perhaps make them >= 16, although then you would
need a function to take a pte and extract its type taking that into
account.
> >  
> > -int guest_physmap_add_page(struct domain *d,
> > -                           unsigned long gpfn,
> > -                           unsigned long mfn,
> > -                           unsigned int page_order)
> > +int guest_physmap_add_entry(struct domain *d,
> > +                            unsigned long gpfn,
> > +                            unsigned long mfn,
> > +                            unsigned long page_order,
> > +                            p2m_type_t t)
> >  {
> >      return create_p2m_entries(d, INSERT,
> >                                pfn_to_paddr(gpfn),
> > -                              pfn_to_paddr(gpfn +
(1<<page_order)),
> > -                              pfn_to_paddr(mfn), MATTR_MEM);
> > +                              pfn_to_paddr(gpfn + (1 <<
page_order)),
> > +                              pfn_to_paddr(mfn), MATTR_MEM, t);
> >  }
> >  
> >  void guest_physmap_remove_page(struct domain *d,
> > @@ -331,7 +350,7 @@ void guest_physmap_remove_page(struct domain *d,
> >      create_p2m_entries(d, REMOVE,
> >                         pfn_to_paddr(gpfn),
> >                         pfn_to_paddr(gpfn + (1<<page_order)),
> > -                       pfn_to_paddr(mfn), MATTR_MEM);
> > +                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
This looks odd to me, although I understand it is calling a common
helper function which requires a type elsewhere. Perhpas p2m_invalid
here really needs a placeholder. p2m_none == INT_MAX perhaps?

Ian.

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Reuse the code of p2m_lookup and add an argument to retrieve the p2m type.
I''d be inclined to just add the parameter and change the existing
callers to pass NULL if they don''t care about the type.

The x86 equivalent of this seems to be get_gfn_type which also takes the
p2m lock, which makes sense because you don''t want the entry changing
under your feet while you do whatever you do with it.

However, I have a feeling this is related to things like CoW and PoD
(Tim?) which we don''t have yet, so perhaps we can get away without this
for now? That would be best since it seems like a lot to chew for 4.4.

Ian.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/p2m.c        |   11 ++++++++++-
>  xen/include/asm-arm/p2m.h |    7 ++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 5449a35..1ae2521 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -71,11 +71,17 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m,
paddr_t addr)
>   * There are no processor functions to do a stage 2 only lookup therefore
we
>   * do a a software walk.
>   */
> -paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
> +paddr_t p2m_get_entry(struct domain *d, paddr_t paddr, p2m_type_t *t)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
>      lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
>      paddr_t maddr = INVALID_PADDR;
> +    p2m_type_t _t;
> +
> +    /* Allow t to be NULL */
> +    t = t ?: &_t;
> +
> +    *t = p2m_invalid;
>  
>      spin_lock(&p2m->lock);
>  
> @@ -99,7 +105,10 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr)
>  
>  done:
>      if ( pte.p2m.valid )
> +    {
>          maddr = (pte.bits & PADDR_MASK & PAGE_MASK) | (paddr &
~PAGE_MASK);
> +        *t = pte.p2m.avail;
> +    }
>  
>      if (third) unmap_domain_page(third);
>      if (second) unmap_domain_page(second);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 1cf7d36..3de69c4 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -49,7 +49,12 @@ int p2m_alloc_table(struct domain *d);
>  void p2m_load_VTTBR(struct domain *d);
>  
>  /* Look up the MFN corresponding to a domain''s PFN. */
> -paddr_t p2m_lookup(struct domain *d, paddr_t gpfn);
> +paddr_t p2m_get_entry(struct domain *d, paddr_t gpfn, p2m_type_t *t);
> +
> +static inline paddr_t p2m_lookup(struct domain *d, paddr_t gpfn)
> +{
> +    return p2m_get_entry(d, gpfn, NULL);
> +}
>  
>  /* Setup p2m RAM mapping for domain d from start-end. */
>  int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);

On 12/05/2013 03:55 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Use the field ''avail'' to store the type of the page.
This information will be
>> retrieved in a future patch to change the behaviour when the page is
removed.
>>
>> Also introduce guest_physmap_add_entry to map and set a specific p2m
type for
>> a page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/p2m.c        |   49
+++++++++++++++++++++++++++++++--------------
>>   xen/include/asm-arm/p2m.h |   18 +++++++++++++----
>>   2 files changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 8f8b47e..5449a35 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
>>       return -ENOSYS;
>>   }
>>
>> -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>> +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> +                               p2m_type_t t)
>>   {
>>       paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>>       lpae_t e = (lpae_t) {
>> @@ -132,12 +133,25 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn,
unsigned int mattr)
>>           .p2m.af = 1,
>>           .p2m.sh = LPAE_SH_OUTER,
>>           .p2m.read = 1,
>> -        .p2m.write = 1,
>>           .p2m.mattr = mattr,
>>           .p2m.table = 1,
>>           .p2m.valid = 1,
>> +        .p2m.avail = t, /* Use avail to store p2m type */
>
> Can we change the name in the struct instead and have a comment "using
> avail bits for type" there instead please. We only have 5 types so
could
> only steal 3 but we may as well take all 4.
>
> A BUILD_BUG_ON to check that the last p2m entry is < 2^4 would be good
> too.
I will do both modification for the next version.

-- 
Julien Grall

At 15:52 +0000 on 05 Dec (1386255150), Ian Campbell
wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> > Until now, Xen doesn''t know the type of the page (ram,
foreign page, mmio,...).
> > Introduce p2m_type_t with basic types:
> >     - p2m_invalid: Nothing is mapped here
> 
> Do we really need this? Is it not equivalent to not setting the present
> bit? I see x86 has the same type though -- Tim can you explain why.
There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  

Tim.

B11;rgb:0000/0000/0000At 16:07 +0000 on 05 Dec (1386256032), Ian Campbell
wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> > Reuse the code of p2m_lookup and add an argument to retrieve the p2m
type.
> 
> I''d be inclined to just add the parameter and change the existing
> callers to pass NULL if they don''t care about the type.
> 
> The x86 equivalent of this seems to be get_gfn_type which also takes the
> p2m lock, which makes sense because you don''t want the entry
changing
> under your feet while you do whatever you do with it.
> 
> However, I have a feeling this is related to things like CoW and PoD
> (Tim?)
Yes, though in theory you could have the same sort of trouble with
balloon drivers, device models moving framebuffers, &c. 

Tim.

On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote:
> 
> On 12/05/2013 03:52 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> >> Until now, Xen doesn''t know the type of the page (ram,
foreign page, mmio,...).
> >> Introduce p2m_type_t with basic types:
> >>      - p2m_invalid: Nothing is mapped here
> >
> > Do we really need this? Is it not equivalent to not setting the
present
> > bit? I see x86 has the same type though -- Tim can you explain why.
> 
> We need a default value when Xen retrieves the p2m type. I don''t
think
> we can assume that p2m_ram_rw (or any other type) is used by default.
> 
> > Since the avail bits in the p2m pte are in pretty short supply I think
> > we can avoid unnecessary types.
> 
> I plan to use directly the decimal value. So we can store up to 16 values.
16 is short supply in my book ;-)

Having got a bit further through the series I see how p2m_invalid is
being used now. It is a useful pseudo-type but it doesn''t need to be
represented in the avail bits I don''t think. How about:

typedef enum {
    p2m_ram_rw,         /* Normal read/write guest RAM */
    p2m_ram_ro,         /* Read-only; writes are silently dropped */
    p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area /
    p2m_map_foreign,    /* Ram pages from foreign domain */

    p2m_max_real_type = 16,    /* Types after this are pseudo-types. */

    p2m_invalid,        /* Nothing mapped here */

} p2m_type_t;

BUILD_BUG_ON(p2m_max_real_type >= 2^4);

Now you can return it etc but it never needs to get put in an actual
pte?

Maybe this is one for the future when we get a bit short on bits.
> >>      - p2m_ram_rw: Normal read/write guest RAM
> >>      - p2m_ram_ro: Read-only guest RAM
> >>      - p2m_mmio_direct: Read/write mapping of device memory
> >>      - p2m_map_foreign: RAM page from foreign guest
> >
> > Is there no need for an entry for a grant mapping (and a ro
> > counterpart)?
> 
> Hmmm .. actually grant table is mapped as RAM (so read/write and 
> execute). Do we want to allow code execution from grant-mapping page?
> If not, then we will need to introduce specific p2m type from
grant-mapping.
If a guest is stupid enough to execute code from a page owned by another
guest then it gets what it deserves ;-)

My question wasn''t about that though -- just whether it is useful for
Xen to track whether the particular RAM mapping is normal or a grant
mapping.

x86 has some special handling, but I don''t know if that is for
correctness or just a historical legacy of something else.

Ian.

On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> At 15:52 +0000 on 05 Dec (1386255150), Ian Campbell wrote:
> > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> > > Until now, Xen doesn''t know the type of the page (ram,
foreign page, mmio,...).
> > > Introduce p2m_type_t with basic types:
> > >     - p2m_invalid: Nothing is mapped here
> > 
> > Do we really need this? Is it not equivalent to not setting the
present
> > bit? I see x86 has the same type though -- Tim can you explain why.
> 
> There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  
This means not Present doesn''t imply invalid, I see. Even if we
don''t
currently have such types I can see how this makes sense. In theory when
we get short of bits we could consider the P bit one of the type bits,
which would give us 16 present and 16 not-present types. Overkill for
now I expect.

Is it ever the case that an actual p2m entry contains the p2m_invalid
bit pattern or is it more of a pseudo-type which is used internally and
as an API token in the hypervisor?

Ian.

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/include/asm-arm/p2m.h |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3de69c4..54d1dab 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      struct page_info *page;
> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
> -
> -    ASSERT(t == NULL);
> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
> +    unsigned long mfn = maddr >> PAGE_SHIFT;
Do we need to eg. convert p2m_invalid into returning NULL instead of
whatever maddr contains in that case?

In the case of p2m_mmio_direct we need to be careful because there is no
struct page. Ah.. here it is in the context:
>      if (!mfn_valid(mfn))
>          return NULL;
Although I wonder if we should convert mmio_direct into NULL even if the
MMIO region happens to have a struct page? (e.g. due to holes in the
frametable)

Ian.

On 12/05/2013 04:14 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote:
>>
>> On 12/05/2013 03:52 PM, Ian Campbell wrote:
>>> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>>>> Until now, Xen doesn''t know the type of the page (ram,
foreign page, mmio,...).
>>>> Introduce p2m_type_t with basic types:
>>>>       - p2m_invalid: Nothing is mapped here
>>>
>>> Do we really need this? Is it not equivalent to not setting the
present
>>> bit? I see x86 has the same type though -- Tim can you explain why.
>>
>> We need a default value when Xen retrieves the p2m type. I
don''t think
>> we can assume that p2m_ram_rw (or any other type) is used by default.
>>
>>> Since the avail bits in the p2m pte are in pretty short supply I
think
>>> we can avoid unnecessary types.
>>
>> I plan to use directly the decimal value. So we can store up to 16
values.
>
> 16 is short supply in my book ;-)
>
> Having got a bit further through the series I see how p2m_invalid is
> being used now. It is a useful pseudo-type but it doesn''t need to
be
> represented in the avail bits I don''t think. How about:
>
> typedef enum {
>      p2m_ram_rw,         /* Normal read/write guest RAM */
>      p2m_ram_ro,         /* Read-only; writes are silently dropped */
>      p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area /
>      p2m_map_foreign,    /* Ram pages from foreign domain */
>
>      p2m_max_real_type = 16,    /* Types after this are pseudo-types. */
>
>      p2m_invalid,        /* Nothing mapped here */
>
> } p2m_type_t;
>
> BUILD_BUG_ON(p2m_max_real_type >= 2^4);
>
> Now you can return it etc but it never needs to get put in an actual
> pte?

This solution was easier to avoid extra code in the different function.
I will rework it for the next series.
>
> Maybe this is one for the future when we get a bit short on bits.
>
>>>>       - p2m_ram_rw: Normal read/write guest RAM
>>>>       - p2m_ram_ro: Read-only guest RAM
>>>>       - p2m_mmio_direct: Read/write mapping of device memory
>>>>       - p2m_map_foreign: RAM page from foreign guest
>>>
>>> Is there no need for an entry for a grant mapping (and a ro
>>> counterpart)?
>>
>> Hmmm .. actually grant table is mapped as RAM (so read/write and
>> execute). Do we want to allow code execution from grant-mapping page?
>> If not, then we will need to introduce specific p2m type from
grant-mapping.
>
> If a guest is stupid enough to execute code from a page owned by another
> guest then it gets what it deserves ;-)
Actually X86, disable execution on grant and foreign mapping.
> My question wasn''t about that though -- just whether it is useful
for
> Xen to track whether the particular RAM mapping is normal or a grant
> mapping.
For now, I don''t see a specific reason to track it.

-- 
Julien Grall

At 16:19 +0000 on 05 Dec (1386256758), Ian Campbell
wrote:
> On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> > At 15:52 +0000 on 05 Dec (1386255150), Ian Campbell wrote:
> > > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> > > > Until now, Xen doesn''t know the type of the page
(ram, foreign page, mmio,...).
> > > > Introduce p2m_type_t with basic types:
> > > >     - p2m_invalid: Nothing is mapped here
> > > 
> > > Do we really need this? Is it not equivalent to not setting the
present
> > > bit? I see x86 has the same type though -- Tim can you explain
why.
> > 
> > There are other not-present types on x86: PoD, paged-out,
emulated-MMIO.
> 
> This means not Present doesn''t imply invalid, I see. Even if we
don''t
> currently have such types I can see how this makes sense. In theory when
> we get short of bits we could consider the P bit one of the type bits,
> which would give us 16 present and 16 not-present types. Overkill for
> now I expect.
> 
> Is it ever the case that an actual p2m entry contains the p2m_invalid
> bit pattern
Yes, IIRC if an existing entry is removed, it''s replaced with an
explicit invalid entry.  It used to be the case that inclid was type 0
so by default all uninitialised entries were invalid too, but ram_rw
had to be type 0 (becasue of an inconvenient interacion with AMD
IOMMUs).  For added confusion on x86 we still have the legacy rule
that invalid entries are reported as mmio_dm, because the default path
is to ask qemu.

Tim.

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Xen needs to know that the current page belongs to another domain. Also
take
> the refcount to this page.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/mm.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 960c872..bf383a7 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -977,6 +977,7 @@ static int xenmem_add_to_physmap_one(
>  {
>      unsigned long mfn = 0;
>      int rc;
> +    p2m_type_t t = p2m_ram_rw;
Can we set this explicitly on a per-mapspace basis please, so the
compiler will complain about an uninitialised variable if we forget to
add the type for a new map space, instead of silently creating writable
ram mappings.
>  
>      switch ( space )
>      {
> @@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
>          break;
>      case XENMAPSPACE_gmfn_foreign:
>      {
> -        paddr_t maddr;
>          struct domain *od;
> +        struct page_info *page;
>          od = rcu_lock_domain_by_any_id(foreign_domid);
>          if ( od == NULL )
>              return -ESRCH;
> @@ -1032,15 +1033,18 @@ static int xenmem_add_to_physmap_one(
>              return rc;
>          }
>  
> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> -        if ( maddr == INVALID_PADDR )
> +        /* Take refcount to the foreign domain page.
"on the foreign" (and maybe "domain''s" or just
"foreign page"
> +         * Refcount will be release in XENMEM_remove_from_physmap */
"will be released".

The refcount will also be removed by p2m_teardown. That probably needs
changing to do an actual walk of the p2m tearing things down, which is a
pain.

Stefano''s now obsolete dma pinning series had a patch which added a
fairly generic walker in it which might be reusable.

The walk might need to support continuations though.

I wonder if this ref ought to be taken in create_p2m_entries for all
entries and not just foreign ones, and then released in the appropriate
places.
> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
> +        if ( !page )
>          {
>              dump_p2m_lookup(od, pfn_to_paddr(idx));
>              rcu_unlock_domain(od);
>              return -EINVAL;
>          }
>  
> -        mfn = maddr >> PAGE_SHIFT;
> +        mfn = page_to_mfn(page);
> +        t = p2m_map_foreign;
>  
>          rcu_unlock_domain(od);
>          break;
> @@ -1051,7 +1055,7 @@ static int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
> +    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
>  
>      return rc;
>  }

On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On Thu, 2013-12-05 at 16:28 +0000, Julien Grall wrote:
> 
> On 12/05/2013 04:14 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote:
> >>
> >> On 12/05/2013 03:52 PM, Ian Campbell wrote:
> >>> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> >>>> Until now, Xen doesn''t know the type of the page
(ram, foreign page, mmio,...).
> >>>> Introduce p2m_type_t with basic types:
> >>>>       - p2m_invalid: Nothing is mapped here
> >>>
> >>> Do we really need this? Is it not equivalent to not setting
the present
> >>> bit? I see x86 has the same type though -- Tim can you explain
why.
> >>
> >> We need a default value when Xen retrieves the p2m type. I
don''t think
> >> we can assume that p2m_ram_rw (or any other type) is used by
default.
> >>
> >>> Since the avail bits in the p2m pte are in pretty short supply
I think
> >>> we can avoid unnecessary types.
> >>
> >> I plan to use directly the decimal value. So we can store up to 16
values.
> >
> > 16 is short supply in my book ;-)
> >
> > Having got a bit further through the series I see how p2m_invalid is
> > being used now. It is a useful pseudo-type but it doesn''t
need to be
> > represented in the avail bits I don''t think. How about:
> >
> > typedef enum {
> >      p2m_ram_rw,         /* Normal read/write guest RAM */
> >      p2m_ram_ro,         /* Read-only; writes are silently dropped */
> >      p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area /
> >      p2m_map_foreign,    /* Ram pages from foreign domain */
> >
> >      p2m_max_real_type = 16,    /* Types after this are pseudo-types.
*/
> >
> >      p2m_invalid,        /* Nothing mapped here */
> >
> > } p2m_type_t;
> >
> > BUILD_BUG_ON(p2m_max_real_type >= 2^4);
> >
> > Now you can return it etc but it never needs to get put in an actual
> > pte?
> 
> 
> This solution was easier to avoid extra code in the different function.
> I will rework it for the next series.
"This" is what I suggested here or what you wrote already?
> > Maybe this is one for the future when we get a bit short on bits.
> >
> >>>>       - p2m_ram_rw: Normal read/write guest RAM
> >>>>       - p2m_ram_ro: Read-only guest RAM
> >>>>       - p2m_mmio_direct: Read/write mapping of device
memory
> >>>>       - p2m_map_foreign: RAM page from foreign guest
> >>>
> >>> Is there no need for an entry for a grant mapping (and a ro
> >>> counterpart)?
> >>
> >> Hmmm .. actually grant table is mapped as RAM (so read/write and
> >> execute). Do we want to allow code execution from grant-mapping
page?
> >> If not, then we will need to introduce specific p2m type from
grant-mapping.
> >
> > If a guest is stupid enough to execute code from a page owned by
another
> > guest then it gets what it deserves ;-)
> 
> Actually X86, disable execution on grant and foreign mapping.
I guess consistency is a good reason to do the same then.
> > My question wasn''t about that though -- just whether it is
useful for
> > Xen to track whether the particular RAM mapping is normal or a grant
> > mapping.
> 
> For now, I don''t see a specific reason to track it.
OK.

On Thu, 2013-12-05 at 17:31 +0100, Tim Deegan wrote:
> At 16:19 +0000 on 05 Dec (1386256758), Ian Campbell wrote:
> > On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> > > At 15:52 +0000 on 05 Dec (1386255150), Ian Campbell wrote:
> > > > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> > > > > Until now, Xen doesn''t know the type of the
page (ram, foreign page, mmio,...).
> > > > > Introduce p2m_type_t with basic types:
> > > > >     - p2m_invalid: Nothing is mapped here
> > > > 
> > > > Do we really need this? Is it not equivalent to not setting
the present
> > > > bit? I see x86 has the same type though -- Tim can you
explain why.
> > > 
> > > There are other not-present types on x86: PoD, paged-out,
emulated-MMIO.
> > 
> > This means not Present doesn''t imply invalid, I see. Even if
we don''t
> > currently have such types I can see how this makes sense. In theory
when
> > we get short of bits we could consider the P bit one of the type bits,
> > which would give us 16 present and 16 not-present types. Overkill for
> > now I expect.
> > 
> > Is it ever the case that an actual p2m entry contains the p2m_invalid
> > bit pattern
> 
> Yes, IIRC if an existing entry is removed, it''s replaced with an
> explicit invalid entry.  It used to be the case that inclid was type 0
> so by default all uninitialised entries were invalid too, but ram_rw
> had to be type 0 (becasue of an inconvenient interacion with AMD
> IOMMUs).  For added confusion on x86 we still have the legacy rule
> that invalid entries are reported as mmio_dm, because the default path
> is to ask qemu.
I think we avoid all those pitfalls on arm right now, so probably
invalid == P==0b0,AVAIL=0b0000 makes sense?

Ian.

On 12/05/2013 04:34 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Xen needs to know that the current page belongs to another domain. Also
take
>> the refcount to this page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/mm.c |   14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 960c872..bf383a7 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -977,6 +977,7 @@ static int xenmem_add_to_physmap_one(
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> +    p2m_type_t t = p2m_ram_rw;
>
> Can we set this explicitly on a per-mapspace basis please, so the
> compiler will complain about an uninitialised variable if we forget to
> add the type for a new map space, instead of silently creating writable
> ram mappings.
Will do.
>>
>>       switch ( space )
>>       {
>> @@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
>>           break;
>>       case XENMAPSPACE_gmfn_foreign:
>>       {
>> -        paddr_t maddr;
>>           struct domain *od;
>> +        struct page_info *page;
>>           od = rcu_lock_domain_by_any_id(foreign_domid);
>>           if ( od == NULL )
>>               return -ESRCH;
>> @@ -1032,15 +1033,18 @@ static int xenmem_add_to_physmap_one(
>>               return rc;
>>           }
>>
>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>> -        if ( maddr == INVALID_PADDR )
>> +        /* Take refcount to the foreign domain page.
>
> "on the foreign" (and maybe "domain''s" or just
"foreign page"
>
>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>
> "will be released".
>
> The refcount will also be removed by p2m_teardown. That probably needs
> changing to do an actual walk of the p2m tearing things down, which is a
> pain.
>
> Stefano''s now obsolete dma pinning series had a patch which added
a
> fairly generic walker in it which might be reusable.
Do you have a link to this patch?
> The walk might need to support continuations though.
>
> I wonder if this ref ought to be taken in create_p2m_entries for all
> entries and not just foreign ones, and then released in the appropriate
> places.
If I''m not mistaken, Xen already takes a ref when the page is allocated
for the domain. Why would we need to take another ref for these pages?

-- 
Julien Grall

On 12/05/2013 04:38 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:28 +0000, Julien Grall wrote:
>>
>> On 12/05/2013 04:14 PM, Ian Campbell wrote:
>>> On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote:
>>>>
>>>> On 12/05/2013 03:52 PM, Ian Campbell wrote:
>>>>> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>>>>>> Until now, Xen doesn''t know the type of the
page (ram, foreign page, mmio,...).
>>>>>> Introduce p2m_type_t with basic types:
>>>>>>        - p2m_invalid: Nothing is mapped here
>>>>>
>>>>> Do we really need this? Is it not equivalent to not setting
the present
>>>>> bit? I see x86 has the same type though -- Tim can you
explain why.
>>>>
>>>> We need a default value when Xen retrieves the p2m type. I
don''t think
>>>> we can assume that p2m_ram_rw (or any other type) is used by
default.
>>>>
>>>>> Since the avail bits in the p2m pte are in pretty short
supply I think
>>>>> we can avoid unnecessary types.
>>>>
>>>> I plan to use directly the decimal value. So we can store up to
16 values.
>>>
>>> 16 is short supply in my book ;-)
>>>
>>> Having got a bit further through the series I see how p2m_invalid
is
>>> being used now. It is a useful pseudo-type but it doesn''t
need to be
>>> represented in the avail bits I don''t think. How about:
>>>
>>> typedef enum {
>>>       p2m_ram_rw,         /* Normal read/write guest RAM */
>>>       p2m_ram_ro,         /* Read-only; writes are silently dropped
*/
>>>       p2m_mmio_direct,    /* Read/write mapping of genuine MMIO
area /
>>>       p2m_map_foreign,    /* Ram pages from foreign domain */
>>>
>>>       p2m_max_real_type = 16,    /* Types after this are
pseudo-types. */
>>>
>>>       p2m_invalid,        /* Nothing mapped here */
>>>
>>> } p2m_type_t;
>>>
>>> BUILD_BUG_ON(p2m_max_real_type >= 2^4);
>>>
>>> Now you can return it etc but it never needs to get put in an
actual
>>> pte?
>>
>>
>> This solution was easier to avoid extra code in the different function.
>> I will rework it for the next series.
>
> "This" is what I suggested here or what you wrote already?
The code I wrote.
>
>>> Maybe this is one for the future when we get a bit short on bits.
>>>
>>>>>>        - p2m_ram_rw: Normal read/write guest RAM
>>>>>>        - p2m_ram_ro: Read-only guest RAM
>>>>>>        - p2m_mmio_direct: Read/write mapping of device
memory
>>>>>>        - p2m_map_foreign: RAM page from foreign guest
>>>>>
>>>>> Is there no need for an entry for a grant mapping (and a ro
>>>>> counterpart)?
>>>>
>>>> Hmmm .. actually grant table is mapped as RAM (so read/write
and
>>>> execute). Do we want to allow code execution from grant-mapping
page?
>>>> If not, then we will need to introduce specific p2m type from
grant-mapping.
>>>
>>> If a guest is stupid enough to execute code from a page owned by
another
>>> guest then it gets what it deserves ;-)
>>
>> Actually X86, disable execution on grant and foreign mapping.
>
> I guess consistency is a good reason to do the same then.
Ok. So I will add p2m_grant_map_ro and p2m_grant_map_rw.

-- 
Julien Grall

On 12/05/2013 04:23 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/include/asm-arm/p2m.h |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 3de69c4..54d1dab 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
>>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t
q)
>>   {
>>       struct page_info *page;
>> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
>> -
>> -    ASSERT(t == NULL);
>> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
>
> Do we need to eg. convert p2m_invalid into returning NULL instead of
> whatever maddr contains in that case?
>
> In the case of p2m_mmio_direct we need to be careful because there is no
> struct page. Ah.. here it is in the context:
>>       if (!mfn_valid(mfn))
>>           return NULL;
>
> Although I wonder if we should convert mmio_direct into NULL even if the
> MMIO region happens to have a struct page? (e.g. due to holes in the
> frametable)
I will do the both in the next version.


-- 
Julien Grall

On Thu, 2013-12-05 at 16:41 +0000, Julien Grall wrote:
> >>
> >>       switch ( space )
> >>       {
> >> @@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
> >>           break;
> >>       case XENMAPSPACE_gmfn_foreign:
> >>       {
> >> -        paddr_t maddr;
> >>           struct domain *od;
> >> +        struct page_info *page;
> >>           od = rcu_lock_domain_by_any_id(foreign_domid);
> >>           if ( od == NULL )
> >>               return -ESRCH;
> >> @@ -1032,15 +1033,18 @@ static int xenmem_add_to_physmap_one(
> >>               return rc;
> >>           }
> >>
> >> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> >> -        if ( maddr == INVALID_PADDR )
> >> +        /* Take refcount to the foreign domain page.
> >
> > "on the foreign" (and maybe "domain''s" or
just "foreign page"
> >
> >> +         * Refcount will be release in XENMEM_remove_from_physmap
*/
> >
> > "will be released".
> >
> > The refcount will also be removed by p2m_teardown. That probably needs
> > changing to do an actual walk of the p2m tearing things down, which is
a
> > pain.
> >
> > Stefano''s now obsolete dma pinning series had a patch which
added a
> > fairly generic walker in it which might be reusable.
> 
> Do you have a link to this patch?
I was afraid you''d say that...
/scrobbles

"xen/arm: introduce a generic p2m walker and use it in p2m_lookup". v6
is msgid
<1380298560-29352-2-git-send-email-stefano.stabellini@eu.citrix.com>. I
don''t recall if that was the last one though.
> 
> > The walk might need to support continuations though.
> >
> > I wonder if this ref ought to be taken in create_p2m_entries for all
> > entries and not just foreign ones, and then released in the
appropriate
> > places.
> 
> If I''m not mistaken, Xen already takes a ref when the page is
allocated
> for the domain. Why would we need to take another ref for these pages?
There''s a nice symmetry to all p2m entries taking a ref count, and it
makes the teardown a bit simpler since you can just drop the ref every
time without worrying about the type.

In some sense (and I''m not sure it does make sense) you could consider
there to be one reference for owning the page and a second for mapping
it.

I don''t think a domain can own a page which isn''t mapped in
its p2m,
because decrease reservation frees the page, although the 1:1 ballooning
workaround has some aspect of that to it.

Ian.

On Thu, 2013-12-05 at 16:44 +0000, Julien Grall wrote:
> 
> On 12/05/2013 04:38 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 16:28 +0000, Julien Grall wrote:
> >>
> >> On 12/05/2013 04:14 PM, Ian Campbell wrote:
> >>> On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote:
> >>>>
> >>>> On 12/05/2013 03:52 PM, Ian Campbell wrote:
> >>>>> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> >>>>>> Until now, Xen doesn''t know the type of
the page (ram, foreign page, mmio,...).
> >>>>>> Introduce p2m_type_t with basic types:
> >>>>>>        - p2m_invalid: Nothing is mapped here
> >>>>>
> >>>>> Do we really need this? Is it not equivalent to not
setting the present
> >>>>> bit? I see x86 has the same type though -- Tim can you
explain why.
> >>>>
> >>>> We need a default value when Xen retrieves the p2m type. I
don''t think
> >>>> we can assume that p2m_ram_rw (or any other type) is used
by default.
> >>>>
> >>>>> Since the avail bits in the p2m pte are in pretty
short supply I think
> >>>>> we can avoid unnecessary types.
> >>>>
> >>>> I plan to use directly the decimal value. So we can store
up to 16 values.
> >>>
> >>> 16 is short supply in my book ;-)
> >>>
> >>> Having got a bit further through the series I see how
p2m_invalid is
> >>> being used now. It is a useful pseudo-type but it
doesn''t need to be
> >>> represented in the avail bits I don''t think. How
about:
> >>>
> >>> typedef enum {
> >>>       p2m_ram_rw,         /* Normal read/write guest RAM */
> >>>       p2m_ram_ro,         /* Read-only; writes are silently
dropped */
> >>>       p2m_mmio_direct,    /* Read/write mapping of genuine
MMIO area /
> >>>       p2m_map_foreign,    /* Ram pages from foreign domain */
> >>>
> >>>       p2m_max_real_type = 16,    /* Types after this are
pseudo-types. */
> >>>
> >>>       p2m_invalid,        /* Nothing mapped here */
> >>>
> >>> } p2m_type_t;
> >>>
> >>> BUILD_BUG_ON(p2m_max_real_type >= 2^4);
> >>>
> >>> Now you can return it etc but it never needs to get put in an
actual
> >>> pte?
> >>
> >>
> >> This solution was easier to avoid extra code in the different
function.
> >> I will rework it for the next series.
> >
> > "This" is what I suggested here or what you wrote already?
> 
> The code I wrote.
Maybe we should just keep this trick in our pocket for when we run out
of bits then?

Ian.

George: I forgot to add that for the release perspective, this series is 
a bug fix for Xen 4.4 on ARM.

Without this series, dom0 will run out of memory sooner or later.

On 12/05/2013 03:42 PM, Julien Grall wrote:
> Hello,
>
> This patch series aims to fix "Failed to unmap" message in dom0
when a guest
> is creating. Actually it will result to leak dom0 memory.
>
> The series is based on pvh dom0 v5 patch series
> (http://www.gossamer-threads.com/lists/xen/devel/309493?page=last) from
Mukesh.
>
>      - Patch #1: this patch add support to remove correctly foreign page
>      on ARM, if it''s possible I would like to merge with patch #6
of Mukesh.
>      - Patch #2: prepare work for the other patch
>      - Patch #3-6: add support for p2m type
>      - Patch #7: set p2m type for foreign page
>      - Patch #8: it''s not really part of this serie. It adds
support for
>      read-only grant-mapping.
>
> In the ideal world, this patch series and the pvh dom0 series should be
mixed
> to avoid to introduce dummy functions that will be removed later.
>
> Sincerely yours,
>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
>
> Julien Grall (8):
>    DO NOT APPLY: xen/arm: Correctly support foreign page removing on ARM
>    xen/arm: move mfn_to_p2m_entry in arch/arm/p2m.c
>    xen/arm: Implement p2m_type_t as an enum
>    xen/arm: Store p2m type in each page of the guest
>    xen/arm: p2m: Add p2m_get_entry
>    xen/arm: Retrieve p2m type in get_page_from_gfn
>    xen/arm: Set foreign page type to p2m_map_foreign
>    xen/arm: grant-table: Support read-only mapping
>
>   xen/arch/arm/mm.c          |   26 ++++++++-------
>   xen/arch/arm/p2m.c         |   78
++++++++++++++++++++++++++++++++++++--------
>   xen/common/memory.c        |   12 ++++---
>   xen/include/asm-arm/p2m.h  |   43 +++++++++++++++++-------
>   xen/include/asm-arm/page.h |   22 -------------
>   5 files changed, 117 insertions(+), 64 deletions(-)
>
-- 
Julien Grall

B11;rgb:0000/0000/0000At 16:39 +0000 on 05 Dec (1386257979), Ian Campbell
wrote:
> On Thu, 2013-12-05 at 17:31 +0100, Tim Deegan wrote:
> > At 16:19 +0000 on 05 Dec (1386256758), Ian Campbell wrote:
> > > On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> > > > At 15:52 +0000 on 05 Dec (1386255150), Ian Campbell wrote:
> > > > > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> > > > > > Until now, Xen doesn''t know the type of
the page (ram, foreign page, mmio,...).
> > > > > > Introduce p2m_type_t with basic types:
> > > > > >     - p2m_invalid: Nothing is mapped here
> > > > > 
> > > > > Do we really need this? Is it not equivalent to not
setting the present
> > > > > bit? I see x86 has the same type though -- Tim can you
explain why.
> > > > 
> > > > There are other not-present types on x86: PoD, paged-out,
emulated-MMIO.
> > > 
> > > This means not Present doesn''t imply invalid, I see.
Even if we don''t
> > > currently have such types I can see how this makes sense. In
theory when
> > > we get short of bits we could consider the P bit one of the type
bits,
> > > which would give us 16 present and 16 not-present types. Overkill
for
> > > now I expect.
> > > 
> > > Is it ever the case that an actual p2m entry contains the
p2m_invalid
> > > bit pattern
> > 
> > Yes, IIRC if an existing entry is removed, it''s replaced with
an
> > explicit invalid entry.  It used to be the case that inclid was type 0
> > so by default all uninitialised entries were invalid too, but ram_rw
> > had to be type 0 (becasue of an inconvenient interacion with AMD
> > IOMMUs).  For added confusion on x86 we still have the legacy rule
> > that invalid entries are reported as mmio_dm, because the default path
> > is to ask qemu.
> 
> I think we avoid all those pitfalls on arm right now, so probably
> invalid == P==0b0,AVAIL=0b0000 makes sense?
Sure.

Tim.

On 12/05/2013 04:54 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:41 +0000, Julien Grall wrote:
>
>>>>
>>>>        switch ( space )
>>>>        {
>>>> @@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
>>>>            break;
>>>>        case XENMAPSPACE_gmfn_foreign:
>>>>        {
>>>> -        paddr_t maddr;
>>>>            struct domain *od;
>>>> +        struct page_info *page;
>>>>            od = rcu_lock_domain_by_any_id(foreign_domid);
>>>>            if ( od == NULL )
>>>>                return -ESRCH;
>>>> @@ -1032,15 +1033,18 @@ static int xenmem_add_to_physmap_one(
>>>>                return rc;
>>>>            }
>>>>
>>>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>>>> -        if ( maddr == INVALID_PADDR )
>>>> +        /* Take refcount to the foreign domain page.
>>>
>>> "on the foreign" (and maybe "domain''s"
or just "foreign page"
>>>
>>>> +         * Refcount will be release in
XENMEM_remove_from_physmap */
>>>
>>> "will be released".
>>>
>>> The refcount will also be removed by p2m_teardown. That probably
needs
>>> changing to do an actual walk of the p2m tearing things down, which
is a
>>> pain.
>>>
>>> Stefano''s now obsolete dma pinning series had a patch
which added a
>>> fairly generic walker in it which might be reusable.
>>
>> Do you have a link to this patch?
>
> I was afraid you''d say that...
> /scrobbles
>
> "xen/arm: introduce a generic p2m walker and use it in
p2m_lookup". v6
> is msgid
> <1380298560-29352-2-git-send-email-stefano.stabellini@eu.citrix.com>.
I
> don''t recall if that was the last one though.
Actually the generic walker will not be usefull for p2m_teardown. It 
seems to be an extension of p2m_lookup, so will only search a specific gfn.
>>
>>> The walk might need to support continuations though.
>>>
>>> I wonder if this ref ought to be taken in create_p2m_entries for
all
>>> entries and not just foreign ones, and then released in the
appropriate
>>> places.
>>
>> If I''m not mistaken, Xen already takes a ref when the page is
allocated
>> for the domain. Why would we need to take another ref for these pages?
>
> There''s a nice symmetry to all p2m entries taking a ref count, and
it
> makes the teardown a bit simpler since you can just drop the ref every
> time without worrying about the type.
If I''m not mistaken, even x86 code doesn''t have 2 ref for each
page. Or
I didn''t see where the ref is taken/release ...

In any case, walk the whole p2m for looking for present page seems a bit 
tough and slow to release memory for a domain.

I''m wondering if we can have a list of foreign page, and browse it when
the domain is destroyed.

-- 
Julien Grall

On Thu, 2013-12-05 at 17:39 +0000, Julien Grall wrote:
> 
> On 12/05/2013 04:54 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 16:41 +0000, Julien Grall wrote:
> >
> >>>>
> >>>>        switch ( space )
> >>>>        {
> >>>> @@ -1019,8 +1020,8 @@ static int
xenmem_add_to_physmap_one(
> >>>>            break;
> >>>>        case XENMAPSPACE_gmfn_foreign:
> >>>>        {
> >>>> -        paddr_t maddr;
> >>>>            struct domain *od;
> >>>> +        struct page_info *page;
> >>>>            od = rcu_lock_domain_by_any_id(foreign_domid);
> >>>>            if ( od == NULL )
> >>>>                return -ESRCH;
> >>>> @@ -1032,15 +1033,18 @@ static int
xenmem_add_to_physmap_one(
> >>>>                return rc;
> >>>>            }
> >>>>
> >>>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> >>>> -        if ( maddr == INVALID_PADDR )
> >>>> +        /* Take refcount to the foreign domain page.
> >>>
> >>> "on the foreign" (and maybe
"domain''s" or just "foreign page"
> >>>
> >>>> +         * Refcount will be release in
XENMEM_remove_from_physmap */
> >>>
> >>> "will be released".
> >>>
> >>> The refcount will also be removed by p2m_teardown. That
probably needs
> >>> changing to do an actual walk of the p2m tearing things down,
which is a
> >>> pain.
> >>>
> >>> Stefano''s now obsolete dma pinning series had a patch
which added a
> >>> fairly generic walker in it which might be reusable.
> >>
> >> Do you have a link to this patch?
> >
> > I was afraid you''d say that...
> > /scrobbles
> >
> > "xen/arm: introduce a generic p2m walker and use it in
p2m_lookup". v6
> > is msgid
> >
<1380298560-29352-2-git-send-email-stefano.stabellini@eu.citrix.com>. I
> > don''t recall if that was the last one though.
> 
> Actually the generic walker will not be usefull for p2m_teardown. It 
> seems to be an extension of p2m_lookup, so will only search a specific gfn.
Ah, sorry, my memory must have been faulty.
> >>> The walk might need to support continuations though.
> >>>
> >>> I wonder if this ref ought to be taken in create_p2m_entries
for all
> >>> entries and not just foreign ones, and then released in the
appropriate
> >>> places.
> >>
> >> If I''m not mistaken, Xen already takes a ref when the
page is allocated
> >> for the domain. Why would we need to take another ref for these
pages?
> >
> > There''s a nice symmetry to all p2m entries taking a ref
count, and it
> > makes the teardown a bit simpler since you can just drop the ref every
> > time without worrying about the type.
> 
> If I''m not mistaken, even x86 code doesn''t have 2 ref for
each page. Or
> I didn''t see where the ref is taken/release ...
I seem to remember Tim saying this was some weird historical reason.
(maybe something to do with shadow page tables, or type counts rather
than ref counts?)
> In any case, walk the whole p2m for looking for present page seems a bit 
> tough and slow to release memory for a domain.
Yes, for a large domain it might take a while, which is why it would
need to use hypercall continuations. Luckily the page table itself
stores your progress if you clear the entries as you go, so that should
be reasonably easy to arrange, take a look at x86''s
domain_relinquish_resources and how it uses d->arch.relmem to track what
phase of the teardown it is at.
> I''m wondering if we can have a list of foreign page, and browse it
when
> the domain is destroyed.
The problem is that struct page_info only has one list head in it and it
is used for the domain''s list of pages.

Ian.

On 12/05/2013 04:56 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:44 +0000, Julien Grall wrote:
>>
>> On 12/05/2013 04:38 PM, Ian Campbell wrote:
>>> On Thu, 2013-12-05 at 16:28 +0000, Julien Grall wrote:
>>>>
>>>> On 12/05/2013 04:14 PM, Ian Campbell wrote:
>>>>> On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote:
>>>>>>
>>>>>> On 12/05/2013 03:52 PM, Ian Campbell wrote:
>>>>>>> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall
wrote:
>>>>>>>> Until now, Xen doesn''t know the type
of the page (ram, foreign page, mmio,...).
>>>>>>>> Introduce p2m_type_t with basic types:
>>>>>>>>         - p2m_invalid: Nothing is mapped here
>>>>>>>
>>>>>>> Do we really need this? Is it not equivalent to not
setting the present
>>>>>>> bit? I see x86 has the same type though -- Tim can
you explain why.
>>>>>>
>>>>>> We need a default value when Xen retrieves the p2m
type. I don''t think
>>>>>> we can assume that p2m_ram_rw (or any other type) is
used by default.
>>>>>>
>>>>>>> Since the avail bits in the p2m pte are in pretty
short supply I think
>>>>>>> we can avoid unnecessary types.
>>>>>>
>>>>>> I plan to use directly the decimal value. So we can
store up to 16 values.
>>>>>
>>>>> 16 is short supply in my book ;-)
>>>>>
>>>>> Having got a bit further through the series I see how
p2m_invalid is
>>>>> being used now. It is a useful pseudo-type but it
doesn''t need to be
>>>>> represented in the avail bits I don''t think. How
about:
>>>>>
>>>>> typedef enum {
>>>>>        p2m_ram_rw,         /* Normal read/write guest RAM
*/
>>>>>        p2m_ram_ro,         /* Read-only; writes are
silently dropped */
>>>>>        p2m_mmio_direct,    /* Read/write mapping of genuine
MMIO area /
>>>>>        p2m_map_foreign,    /* Ram pages from foreign domain
*/
>>>>>
>>>>>        p2m_max_real_type = 16,    /* Types after this are
pseudo-types. */
>>>>>
>>>>>        p2m_invalid,        /* Nothing mapped here */
>>>>>
>>>>> } p2m_type_t;
>>>>>
>>>>> BUILD_BUG_ON(p2m_max_real_type >= 2^4);
>>>>>
>>>>> Now you can return it etc but it never needs to get put in
an actual
>>>>> pte?
>>>>
>>>>
>>>> This solution was easier to avoid extra code in the different
function.
>>>> I will rework it for the next series.
>>>
>>> "This" is what I suggested here or what you wrote
already?
>>
>> The code I wrote.
>
> Maybe we should just keep this trick in our pocket for when we run out
> of bits then?
Sounds good to me. I will add a comment in the code with your solution.

-- 
Julien Grall

On 12/05/2013 04:34 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Xen needs to know that the current page belongs to another domain. Also
take
>> the refcount to this page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/mm.c |   14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 960c872..bf383a7 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -977,6 +977,7 @@ static int xenmem_add_to_physmap_one(
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> +    p2m_type_t t = p2m_ram_rw;
>
> Can we set this explicitly on a per-mapspace basis please, so the
> compiler will complain about an uninitialised variable if we forget to
> add the type for a new map space, instead of silently creating writable
> ram mappings.
Actually, GCC compiler won''t complain if t is not set in one of the
case.
If have tried this following dummy function on several version of GCC 
(the most recent is 4.8.2) and they don''t emit any warning.

================================================================int f (int a)
{
     int b;

     switch (a)
     {
     case 1:
         b = 1;
         break;
     case 2:
         /* b is unset */
         break;
     }

     return b;
}
===================================================================
I have also tried clang, and I effectively get an error. But we don''t 
yet support clang for Xen ARM port.

I have found a bug report opened in 2004 for this kind of bug, but never 
fixed... (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501).

-- 
Julien Grall

On 12/05/2013 04:23 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/include/asm-arm/p2m.h |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 3de69c4..54d1dab 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -105,9 +105,8 @@ static inline struct page_info *get_page_from_gfn(
>>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t
q)
>>   {
>>       struct page_info *page;
>> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
>> -
>> -    ASSERT(t == NULL);
>> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
>> +    unsigned long mfn = maddr >> PAGE_SHIFT;
>
> Do we need to eg. convert p2m_invalid into returning NULL instead of
> whatever maddr contains in that case?
In that case maddr will contain INVALID_PADDR, which will turn in 
INVALID_MFN with the shift. So we don''t need to check the p2m type.
>
> In the case of p2m_mmio_direct we need to be careful because there is no
> struct page. Ah.. here it is in the context:
>>       if (!mfn_valid(mfn))
>>           return NULL;
>
> Although I wonder if we should convert mmio_direct into NULL even if the
> MMIO region happens to have a struct page? (e.g. due to holes in the
> frametable)
Actually, I think it''s fine. get_page will fail because the page
won''t
belong to the domain. So the function will return NULL.

-- 
Julien Grall

On Mon, 2013-12-09 at 02:36 +0000, Julien Grall wrote:
> 
> On 12/05/2013 04:23 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote:
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>   xen/include/asm-arm/p2m.h |    5 ++---
> >>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >> index 3de69c4..54d1dab 100644
> >> --- a/xen/include/asm-arm/p2m.h
> >> +++ b/xen/include/asm-arm/p2m.h
> >> @@ -105,9 +105,8 @@ static inline struct page_info
*get_page_from_gfn(
> >>       struct domain *d, unsigned long gfn, p2m_type_t *t,
p2m_query_t q)
> >>   {
> >>       struct page_info *page;
> >> -    unsigned long mfn = gmfn_to_mfn(d, gfn);
> >> -
> >> -    ASSERT(t == NULL);
> >> +    paddr_t maddr = p2m_get_entry(d, pfn_to_paddr(gfn), t);
> >> +    unsigned long mfn = maddr >> PAGE_SHIFT;
> >
> > Do we need to eg. convert p2m_invalid into returning NULL instead of
> > whatever maddr contains in that case?
> 
> In that case maddr will contain INVALID_PADDR, which will turn in 
> INVALID_MFN with the shift. So we don''t need to check the p2m
type.
INVALID_PADDR is ~0ULL (==64-bits of ones) and INVALID_MFN is ~0UL,
which could be either 32- or 64-bits of ones depending on arm32 vs
arm64.

So on arm32 the shift and size truncation from paddr to mfn will give us
32-bits of ones (assuming that isn''t undefined behaviour somehow). But
on 64-bit the shift will give us zeroes at the top 12 bits, which is not
== INVALID_MFN
> 
> >
> > In the case of p2m_mmio_direct we need to be careful because there is
no
> > struct page. Ah.. here it is in the context:
> >>       if (!mfn_valid(mfn))
> >>           return NULL;
> >
> > Although I wonder if we should convert mmio_direct into NULL even if
the
> > MMIO region happens to have a struct page? (e.g. due to holes in the
> > frametable)
> 
> Actually, I think it''s fine. get_page will fail because the page
won''t
> belong to the domain. So the function will return NULL.
I''d rather be explicit that rely on such things, if possible, though.

If nothing else it makes the codes logic easier to understand.

Ian.

On 05.12.13 16:56, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:51 +0100, Egger, Christoph wrote:
>> On 05.12.13 16:42, Julien Grall wrote:
>>> Until now, Xen doesn''t know the type of the page (ram,
foreign page, mmio,...).
>>> Introduce p2m_type_t with basic types:
>>>     - p2m_invalid: Nothing is mapped here
>>>     - p2m_ram_rw: Normal read/write guest RAM
>>>     - p2m_ram_ro: Read-only guest RAM
>>>     - p2m_mmio_direct: Read/write mapping of device memory
>>>     - p2m_map_foreign: RAM page from foreign guest
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>> ---
>>>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>>> index f079f00..b24f94a 100644
>>> --- a/xen/include/asm-arm/p2m.h
>>> +++ b/xen/include/asm-arm/p2m.h
>>> @@ -20,6 +20,16 @@ struct p2m_domain {
>>>      uint8_t vmid;
>>>  };
>>>  
>>> +typedef enum {
>>> +    p2m_invalid = 0,        /* Nothing mapped here */
>>> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
>>> +    p2m_ram_ro = 2,         /* Read-only; writes are silently
dropped */
>>> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO
area */
>>> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
>>> +} p2m_type_t;
>>> +
>>> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
>>> +
>>
>> Is it possible to merge p2m_type_t with x86 and move into common code?
> 
> Not really, the p2m handling is very different on the two arches, even
> if they look superficially similar at this level.
That does not imply there is no common logic from the algorithm POV.
Do you see a way split the algorithm into architecture-specific and
architecture-independent parts?
> x86 has more types than can fit in the available pte space on ARM for a
> start.
>
> I''d like to keep them separate please.
Ok. Then leave the declaration in the architecture specific part
and make the accessors available for the common code.
In OO-languages this is called an ''interface''.

Christoph

On Mon, 2013-12-09 at 12:16 +0100, Egger, Christoph
wrote:
> On 05.12.13 16:56, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 16:51 +0100, Egger, Christoph wrote:
> >> On 05.12.13 16:42, Julien Grall wrote:
> >>> Until now, Xen doesn''t know the type of the page
(ram, foreign page, mmio,...).
> >>> Introduce p2m_type_t with basic types:
> >>>     - p2m_invalid: Nothing is mapped here
> >>>     - p2m_ram_rw: Normal read/write guest RAM
> >>>     - p2m_ram_ro: Read-only guest RAM
> >>>     - p2m_mmio_direct: Read/write mapping of device memory
> >>>     - p2m_map_foreign: RAM page from foreign guest
> >>>
> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>> ---
> >>>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
> >>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/include/asm-arm/p2m.h
b/xen/include/asm-arm/p2m.h
> >>> index f079f00..b24f94a 100644
> >>> --- a/xen/include/asm-arm/p2m.h
> >>> +++ b/xen/include/asm-arm/p2m.h
> >>> @@ -20,6 +20,16 @@ struct p2m_domain {
> >>>      uint8_t vmid;
> >>>  };
> >>>  
> >>> +typedef enum {
> >>> +    p2m_invalid = 0,        /* Nothing mapped here */
> >>> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> >>> +    p2m_ram_ro = 2,         /* Read-only; writes are silently
dropped */
> >>> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine
MMIO area */
> >>> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain
*/
> >>> +} p2m_type_t;
> >>> +
> >>> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> >>> +
> >>
> >> Is it possible to merge p2m_type_t with x86 and move into common
code?
> > 
> > Not really, the p2m handling is very different on the two arches, even
> > if they look superficially similar at this level.
> 
> That does not imply there is no common logic from the algorithm POV.
> Do you see a way split the algorithm into architecture-specific and
> architecture-independent parts?
The question was can we move p2m_type_t to common code. This is clearly
not possible because the PTEs in both cases have different number of
available bits, and the x86 case has particular requirements about which
type is represented by pattern 0 (due to some AMD IOMMU behaviour,
judging from the comment)
> > x86 has more types than can fit in the available pte space on ARM for
a
> > start.
> >
> > I''d like to keep them separate please.
> 
> Ok. Then leave the declaration in the architecture specific part
> and make the accessors available for the common code.
> In OO-languages this is called an ''interface''.
Thanks, I had never heard of an ''interface''. </sarcasm>.

What do you think we are defining here if it is not an interface?

Ian.