[PATCH v3 0/4] xen/arm: grant_table

[PATCH v3 0/4] xen/arm: grant_table

[Stefano Stabellini]

Hi all,
this patch series implements the basic mechanisms needed by the grant
table to work properly.

With this patch series applied and the corresponding Linux side patch
series, I am able to boot a guest out of a PV disk and get the PV
network up and running.


Changes in v3:

- rebased on 9e0de31c672fd127e6fb0c88a26eb147037a9abc, drop patches
already applied from the series;
- replace printk with gdprintk in create_grant_host_mapping;
- print a warning once in gnttab_mark_dirty;
- replace get_page_type and put_page_type with an empty implementation;
- rephrase commit message of "create_p2m_entries should not call
free_domheap_page";
- remove HVM_PARAM_GRANT_START_PFN.


Changes in v2:

- create_grant_host_mapping returns error for read-only mappings;
- remove get_page_light reference;
- add a patch to fix the lr_queue initialization for the first 32 irqs;
- add a patch to implement get/put_page_type.


Stefano Stabellini (4):
      xen/arm: implement page reference and grant table functions needed by grant_table.c
      xen/arm: implement get/put_page_type
      xen/arm: create_p2m_entries should not call free_domheap_page
      xen/arm: grant table

 xen/arch/arm/dummy.S     |   13 ----
 xen/arch/arm/mm.c        |  157 +++++++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/p2m.c       |   82 ++++++++++++++++---------
 xen/arch/arm/traps.c     |    1 +
 xen/include/asm-arm/mm.h |    1 -
 5 files changed, 210 insertions(+), 44 deletions(-)

Cheers,

Stefano

[PATCH v3 1/4] xen/arm: implement page reference and grant table functions needed by grant_table.c

[Stefano Stabellini]

The implementation is strongly "inspired" by their x86 counterparts,
except that we assume paging_mode_external and paging_mode_translate.

TODO: read_only mappings and gnttab_mark_dirty.

Changes in v3:

- replace printk with gdprintk in create_grant_host_mapping;
- print a warning once in gnttab_mark_dirty.

Changes in v2:

- create_grant_host_mapping returns error for read-only mappings;
- remove get_page_light reference.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/dummy.S |    9 ----
 xen/arch/arm/mm.c    |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c   |   77 +++++++++++++++++++++++----------
 3 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
index cab9522..baced25 100644
--- a/xen/arch/arm/dummy.S
+++ b/xen/arch/arm/dummy.S
@@ -23,18 +23,10 @@ DUMMY(arch_vcpu_reset);
 NOP(update_vcpu_system_time);
 
 /* Page Reference & Type Maintenance */
-DUMMY(get_page);
 DUMMY(get_page_type);
-DUMMY(page_get_owner_and_reference);
-DUMMY(put_page);
 DUMMY(put_page_type);
 
 /* Grant Tables */
-DUMMY(create_grant_host_mapping);
-DUMMY(gnttab_clear_flag);
-DUMMY(gnttab_mark_dirty);
-DUMMY(is_iomem_page);
-DUMMY(replace_grant_host_mapping);
 DUMMY(steal_page);
 
 /* Page Offlining */
@@ -45,7 +37,6 @@ DUMMY(domain_get_maximum_gpfn);
 DUMMY(domain_relinquish_resources);
 DUMMY(domain_set_time_offset);
 DUMMY(dom_cow);
-DUMMY(gmfn_to_mfn);
 DUMMY(hypercall_create_continuation);
 DUMMY(send_timer_event);
 DUMMY(share_xen_page_with_privileged_guests);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index d369ee3..e963af9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -545,6 +545,121 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
 
     return 0;
 }
+
+struct domain *page_get_owner_and_reference(struct page_info *page)
+{
+    unsigned long x, y = page->count_info;
+
+    do {
+        x = y;
+        /*
+         * Count ==  0: Page is not allocated, so we cannot take a reference.
+         * Count == -1: Reference count would wrap, which is invalid.
+         */
+        if ( unlikely(((x + 1) & PGC_count_mask) <= 1) )
+            return NULL;
+    }
+    while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
+
+    return page_get_owner(page);
+}
+
+void put_page(struct page_info *page)
+{
+    unsigned long nx, x, y = page->count_info;
+
+    do {
+        ASSERT((y & PGC_count_mask) != 0);
+        x  = y;
+        nx = x - 1;
+    }
+    while ( unlikely((y = cmpxchg(&page->count_info, x, nx)) != x) );
+
+    if ( unlikely((nx & PGC_count_mask) == 0) )
+    {
+        free_domheap_page(page);
+    }
+}
+
+int get_page(struct page_info *page, struct domain *domain)
+{
+    struct domain *owner = page_get_owner_and_reference(page);
+
+    if ( likely(owner == domain) )
+        return 1;
+
+    if ( owner != NULL )
+        put_page(page);
+
+    return 0;
+}
+
+void gnttab_clear_flag(unsigned long nr, uint16_t *addr)
+{
+    /*
+     * Note that this cannot be clear_bit(), as the access must be
+     * confined to the specified 2 bytes.
+     */
+    uint16_t mask = ~(1 << nr), old;
+
+    do {
+        old = *addr;
+    } while (cmpxchg(addr, old, old & mask) != old);
+}
+
+void gnttab_mark_dirty(struct domain *d, unsigned long l)
+{
+    /* XXX: mark dirty */
+    static int warning;
+    if (!warning) {
+        gdprintk(XENLOG_WARNING, "gnttab_mark_dirty not implemented yet\n");
+        warning = 1;
+    }
+}
+
+int create_grant_host_mapping(unsigned long addr, unsigned long frame,
+                              unsigned int flags, unsigned int cache_flags)
+{
+    int rc;
+
+    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;
+    }
+
+    rc = guest_physmap_add_page(current->domain,
+                                 addr >> PAGE_SHIFT, frame, 0);
+    if ( rc )
+        return GNTST_general_error;
+    else
+        return GNTST_okay;
+}
+
+int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
+        unsigned long new_addr, unsigned int flags)
+{
+    unsigned long gfn = (unsigned long)(addr >> PAGE_SHIFT);
+    struct domain *d = current->domain;
+
+    if ( new_addr != 0 || (flags & GNTMAP_contains_pte) )
+        return GNTST_general_error;
+
+    guest_physmap_remove_page(d, gfn, mfn, 0);
+
+    return GNTST_okay;
+}
+
+int is_iomem_page(unsigned long mfn)
+{
+    if ( !mfn_valid(mfn) )
+        return 1;
+    return 0;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 073216b..7c23b7d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -120,8 +120,14 @@ static int p2m_create_table(struct domain *d,
     return 0;
 }
 
+enum p2m_operation {
+    INSERT,
+    ALLOCATE,
+    REMOVE
+};
+
 static int create_p2m_entries(struct domain *d,
-                     int alloc,
+                     enum p2m_operation op,
                      paddr_t start_gpaddr,
                      paddr_t end_gpaddr,
                      paddr_t maddr,
@@ -191,25 +197,39 @@ static int create_p2m_entries(struct domain *d,
         }
 
         /* Allocate a new RAM page and attach */
-        if (alloc)
-        {
-            struct page_info *page;
-            lpae_t pte;
-
-            rc = -ENOMEM;
-            page = alloc_domheap_page(d, 0);
-            if ( page == NULL ) {
-                printk("p2m_populate_ram: failed to allocate page\n");
-                goto out;
-            }
-
-            pte = mfn_to_p2m_entry(page_to_mfn(page), mattr);
-
-            write_pte(&third[third_table_offset(addr)], pte);
-        } else {
-            lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr);
-            write_pte(&third[third_table_offset(addr)], pte);
-            maddr += PAGE_SIZE;
+        switch (op) {
+            case ALLOCATE:
+                {
+                    struct page_info *page;
+                    lpae_t pte;
+
+                    rc = -ENOMEM;
+                    page = alloc_domheap_page(d, 0);
+                    if ( page == NULL ) {
+                        printk("p2m_populate_ram: failed to allocate page\n");
+                        goto out;
+                    }
+
+                    pte = mfn_to_p2m_entry(page_to_mfn(page), mattr);
+
+                    write_pte(&third[third_table_offset(addr)], pte);
+                }
+                break;
+            case INSERT:
+                {
+                    lpae_t pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr);
+                    write_pte(&third[third_table_offset(addr)], pte);
+                    maddr += PAGE_SIZE;
+                }
+                break;
+            case REMOVE:
+                {
+                    lpae_t pte;
+                    memset(&pte, 0x00, sizeof(pte));
+                    write_pte(&third[third_table_offset(addr)], pte);
+                    maddr += PAGE_SIZE;
+                }
+                break;
         }
     }
 
@@ -229,7 +249,7 @@ int p2m_populate_ram(struct domain *d,
                      paddr_t start,
                      paddr_t end)
 {
-    return create_p2m_entries(d, 1, start, end, 0, MATTR_MEM);
+    return create_p2m_entries(d, ALLOCATE, start, end, 0, MATTR_MEM);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -237,7 +257,7 @@ int map_mmio_regions(struct domain *d,
                      paddr_t end_gaddr,
                      paddr_t maddr)
 {
-    return create_p2m_entries(d, 0, start_gaddr, end_gaddr, maddr, MATTR_DEV);
+    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr, maddr, MATTR_DEV);
 }
 
 int guest_physmap_add_page(struct domain *d,
@@ -245,7 +265,7 @@ int guest_physmap_add_page(struct domain *d,
                            unsigned long mfn,
                            unsigned int page_order)
 {
-    return create_p2m_entries(d, 0, gpfn << PAGE_SHIFT,
+    return create_p2m_entries(d, INSERT, gpfn << PAGE_SHIFT,
                               (gpfn + (1<<page_order)) << PAGE_SHIFT,
                               mfn << PAGE_SHIFT, MATTR_MEM);
 }
@@ -254,7 +274,9 @@ void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order)
 {
-    ASSERT(0);
+    create_p2m_entries(d, REMOVE, gpfn << PAGE_SHIFT,
+                       (gpfn + (1<<page_order)) << PAGE_SHIFT,
+                       mfn << PAGE_SHIFT, MATTR_MEM);
 }
 
 int p2m_alloc_table(struct domain *d)
@@ -318,6 +340,13 @@ int p2m_init(struct domain *d)
 
     return 0;
 }
+
+unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
+{
+    paddr_t p = p2m_lookup(d, gpfn << PAGE_SHIFT);
+    return p >> PAGE_SHIFT;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.2.5

[PATCH v3 2/4] xen/arm: implement get/put_page_type

[Stefano Stabellini]

Add a basic get_page_type and put_page_type implementation: we don't
care about typecounts so just return success.

Also remove PGT_shared_page, that is unused.


Changes in v3:

- replace get_page_type and put_page_type with an empty implementation.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/dummy.S     |    4 ----
 xen/arch/arm/mm.c        |   13 +++++++++++++
 xen/include/asm-arm/mm.h |    1 -
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
index baced25..2b96d22 100644
--- a/xen/arch/arm/dummy.S
+++ b/xen/arch/arm/dummy.S
@@ -22,10 +22,6 @@ DUMMY(arch_get_info_guest);
 DUMMY(arch_vcpu_reset);
 NOP(update_vcpu_system_time);
 
-/* Page Reference & Type Maintenance */
-DUMMY(get_page_type);
-DUMMY(put_page_type);
-
 /* Grant Tables */
 DUMMY(steal_page);
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e963af9..96a4ca2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -594,6 +594,19 @@ int get_page(struct page_info *page, struct domain *domain)
     return 0;
 }
 
+/* Common code requires get_page_type and put_page_type.
+ * We don't care about typecounts so we just do the minimum to make it
+ * happy. */
+int get_page_type(struct page_info *page, unsigned long type)
+{
+    return 1;
+}
+
+void put_page_type(struct page_info *page)
+{
+    return;
+}
+
 void gnttab_clear_flag(unsigned long nr, uint16_t *addr)
 {
     /*
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 53801b0..b37bd35 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -71,7 +71,6 @@ struct page_info
 
 #define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
 #define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_shared_page   PG_mask(8, 4)  /* CoW sharable page              */
 #define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
 
  /* Owning guest has pinned this page to its current type? */
-- 
1.7.2.5

Re: [PATCH v3 2/4] xen/arm: implement get/put_page_type

[Ian Campbell]

On Thu, 2012-07-26 at 19:54 +0100, Stefano Stabellini wrote:
> Add a basic get_page_type and put_page_type implementation: we don't
> care about typecounts so just return success.
> 
> Also remove PGT_shared_page, that is unused.
> 
> 
> Changes in v3:
> 
> - replace get_page_type and put_page_type with an empty implementation.

Tim suggested that we consider adding an assert that the reference count
is non-zero in both of these, since it is incorrect to take a type count
without a reference count (we think).

In the future maybe we should consider refactoring the gnttab code (and
anything else using type counts directly) to use a "pg_make_writable"
arch abstraction so we an hide the type counts inside x86 code where
they properly belong.

> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/dummy.S     |    4 ----
>  xen/arch/arm/mm.c        |   13 +++++++++++++
>  xen/include/asm-arm/mm.h |    1 -
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
> index baced25..2b96d22 100644
> --- a/xen/arch/arm/dummy.S
> +++ b/xen/arch/arm/dummy.S
> @@ -22,10 +22,6 @@ DUMMY(arch_get_info_guest);
>  DUMMY(arch_vcpu_reset);
>  NOP(update_vcpu_system_time);
>  
> -/* Page Reference & Type Maintenance */
> -DUMMY(get_page_type);
> -DUMMY(put_page_type);
> -
>  /* Grant Tables */
>  DUMMY(steal_page);
>  
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index e963af9..96a4ca2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -594,6 +594,19 @@ int get_page(struct page_info *page, struct domain *domain)
>      return 0;
>  }
>  
> +/* Common code requires get_page_type and put_page_type.
> + * We don't care about typecounts so we just do the minimum to make it
> + * happy. */
> +int get_page_type(struct page_info *page, unsigned long type)
> +{
> +    return 1;
> +}
> +
> +void put_page_type(struct page_info *page)
> +{
> +    return;
> +}
> +
>  void gnttab_clear_flag(unsigned long nr, uint16_t *addr)
>  {
>      /*
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 53801b0..b37bd35 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -71,7 +71,6 @@ struct page_info
>  
>  #define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
>  #define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
> -#define PGT_shared_page   PG_mask(8, 4)  /* CoW sharable page              */
>  #define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
>  
>   /* Owning guest has pinned this page to its current type? */

[PATCH v3 3/4] xen/arm: create_p2m_entries should not call free_domheap_page

[Stefano Stabellini]

The guest is entitled to leak a page from its p2m (by overwriting it) if
it wants to. Since the memory is effectively lost to it (can't even be
recovered by XENMEM increase reservation etc).

In these cases we shouldn't call free_domheap_page to free the existing
page from create_p2m_entries, because it resets the reference counting
but the page is still allocated to the guest (even if not in the p2m
anymore) and common grant_table code is also going to call put_page on
it.


Changes in v3:

- rephrase commit message.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/p2m.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 7c23b7d..7ae4515 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -189,12 +189,7 @@ static int create_p2m_entries(struct domain *d,
         /* else: third already valid */
 
         if ( third[third_table_offset(addr)].p2m.valid )
-        {
-            /* p2m entry already present */
-            free_domheap_page(
-                    mfn_to_page(third[third_table_offset(addr)].p2m.base));
             flush_tlb_all_local();
-        }
 
         /* Allocate a new RAM page and attach */
         switch (op) {
-- 
1.7.2.5

[PATCH v3 4/4] xen/arm: grant table

[Stefano Stabellini]

Implement XENMAPSPACE_grant_table and grant_table_op.


Changes in v3:

- remove HVM_PARAM_GRANT_START_PFN.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/mm.c    |   29 ++++++++++++++++++++++++++++-
 xen/arch/arm/traps.c |    1 +
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 96a4ca2..08bc55b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -25,6 +25,7 @@
 #include <xen/mm.h>
 #include <xen/preempt.h>
 #include <xen/errno.h>
+#include <xen/grant_table.h>
 #include <xen/guest_access.h>
 #include <xen/domain_page.h>
 #include <asm/page.h>
@@ -465,11 +466,37 @@ static int xenmem_add_to_physmap_once(
     struct domain *d,
     const struct xen_add_to_physmap *xatp)
 {
-    unsigned long mfn = 0;
+    unsigned long mfn = 0, idx = 0;
     int rc;
 
     switch ( xatp->space )
     {
+    case XENMAPSPACE_grant_table:
+        spin_lock(&d->grant_table->lock);
+
+        if ( d->grant_table->gt_version == 0 )
+            d->grant_table->gt_version = 1;
+
+        idx = xatp->idx;
+        if ( d->grant_table->gt_version == 2 &&
+                (xatp->idx & XENMAPIDX_grant_table_status) )
+        {
+            idx &= ~XENMAPIDX_grant_table_status;
+            if ( idx < nr_status_frames(d->grant_table) )
+                mfn = virt_to_mfn(d->grant_table->status[idx]);
+        }
+        else
+        {
+            if ( (idx >= nr_grant_frames(d->grant_table)) &&
+                    (idx < max_nr_grant_frames) )
+                gnttab_grow_table(d, idx + 1);
+
+            if ( idx < nr_grant_frames(d->grant_table) )
+                mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
+        }
+
+        spin_unlock(&d->grant_table->lock);
+        break;
     case XENMAPSPACE_shared_info:
         if ( xatp->idx == 0 )
             mfn = virt_to_mfn(d->shared_info);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ad308c3..d77879e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -431,6 +431,7 @@ static arm_hypercall_t *arm_hypercall_table[] = {
     HYPERCALL(physdev_op),
     HYPERCALL(sysctl),
     HYPERCALL(hvm_op),
+    HYPERCALL(grant_table_op),
 };
 
 static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
-- 
1.7.2.5

Re: [PATCH v3 0/4] xen/arm: grant_table

[Ian Campbell]

On Thu, 2012-07-26 at 19:53 +0100, Stefano Stabellini wrote:
> Hi all,
> this patch series implements the basic mechanisms needed by the grant
> table to work properly.

I've applied all 4 of these to my "arm-for-4.3" branch which I'll
announce shortly.