[PATCH 0/4] xen/arm: grant_table

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


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

 xen/arch/arm/domain.c           |    3 +
 xen/arch/arm/dummy.S            |    9 ---
 xen/arch/arm/mm.c               |  139 ++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/p2m.c              |   82 +++++++++++++++--------
 xen/arch/arm/traps.c            |    1 +
 xen/include/asm-arm/paging.h    |    4 +-
 xen/include/public/hvm/params.h |    4 +-
 7 files changed, 200 insertions(+), 42 deletions(-)

Cheers,

Stefano

[PATCH 1/4] xen/arm: set paging_mode_translate and paging_mode_external

[Stefano Stabellini]

On ARM, given the kind of guests we support, it makes sense to set
paging_mode_translate and paging_mode_external by default.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/include/asm-arm/paging.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/paging.h b/xen/include/asm-arm/paging.h
index 3d7dd95..fe1f203 100644
--- a/xen/include/asm-arm/paging.h
+++ b/xen/include/asm-arm/paging.h
@@ -1,8 +1,8 @@
 #ifndef _XEN_PAGING_H
 #define _XEN_PAGING_H
 
-#define paging_mode_translate(d)              (0)
-#define paging_mode_external(d)               (0)
+#define paging_mode_translate(d)              (1)
+#define paging_mode_external(d)               (1)
 
 #endif /* XEN_PAGING_H */
 
-- 
1.7.2.5

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/dummy.S |    9 ----
 xen/arch/arm/mm.c    |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c   |   77 ++++++++++++++++++++++++-----------
 3 files changed, 163 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 40ac176..ddf8c6c 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -525,6 +525,116 @@ 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. 
+         * Count == -2: Remaining unused ref is reserved for get_page_light().
+         */
+        if ( unlikely(((x + 2) & PGC_count_mask) <= 2) )
+            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 */
+}
+
+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 )
+        p2mt = p2m_grant_map_ro;
+    else
+        p2mt = p2m_grant_map_rw;
+    */
+    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 3/4] xen/arm: create_p2m_entries should not call free_domheap_page

[Stefano Stabellini]

The guest is entitled to use valid memory pages as pfns for grant_table
usage.

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.

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 4/4] xen/arm: grant table

[Stefano Stabellini]

Implement XENMAPSPACE_grant_table and grant_table_op.

Introduce an HVM_PARAM to tell the guest where to map the grant table
(HVM_PARAM_GRANT_START_PFN), similarly to what we do with
HVM_PARAM_CONSOLE_PFN and HVM_PARAM_STORE_PFN.
However HVM_PARAM_GRANT_START_PFN is also going to be used by dom0, so
we set the parameter in Xen rather than libxc.
Using HVM_PARAM_GRANT_START_PFN removes the need for a platform pci
device.

Statically set HVM_PARAM_GRANT_START_PFN to 0xb0000000 for now.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/domain.c           |    3 +++
 xen/arch/arm/mm.c               |   29 ++++++++++++++++++++++++++++-
 xen/arch/arm/traps.c            |    1 +
 xen/include/public/hvm/params.h |    4 +++-
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index ac6a30d..f33f6f1 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -341,6 +341,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
     if ( (rc = domain_vgic_init(d)) != 0 )
         goto fail;
 
+    /* XXX: select this dynamically */
+    d->arch.hvm_domain.params[HVM_PARAM_GRANT_START_PFN] = 0xb0000;
+
     /* Domain 0 gets a real UART not an emulated one */
     if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 )
         goto fail;
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ddf8c6c..43bbfef 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,7 +466,7 @@ 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 )
@@ -474,6 +475,32 @@ static int xenmem_add_to_physmap_once(
             if ( xatp->idx == 0 )
                 mfn = virt_to_mfn(d->shared_info);
             break;
+        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;
         default:
             return -ENOSYS;
     }
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6e6807..3975878 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)
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 55c1b57..a2da05d 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -147,6 +147,8 @@
 #define HVM_PARAM_ACCESS_RING_PFN   28
 #define HVM_PARAM_SHARING_RING_PFN  29
 
-#define HVM_NR_PARAMS          30
+#define HVM_PARAM_GRANT_START_PFN 30
+
+#define HVM_NR_PARAMS          31
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
-- 
1.7.2.5

Re: [PATCH 1/4] xen/arm: set paging_mode_translate and paging_mode_external

[Tim Deegan]

At 12:05 +0100 on 04 Jul (1341403531), Stefano Stabellini wrote:
> On ARM, given the kind of guests we support, it makes sense to set
> paging_mode_translate and paging_mode_external by default.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Tim Deegan <tim@xen.org>

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

[Tim Deegan]

At 12:05 +0100 on 04 Jul (1341403532), Stefano Stabellini wrote:
> 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.

Do we actually need these refcounts on ARM?  Or was it just the
typecount we thought we could do without?

> +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. 
> +         * Count == -2: Remaining unused ref is reserved for get_page_light().
> +         */
> +        if ( unlikely(((x + 2) & PGC_count_mask) <= 2) )
> +            return NULL;
> +    }
> +    while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );

get_page_light() is an x86-ism, so we don't need to leave room for it here. 

> +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;

A hard tab has snuck in here.  

> +	} while (cmpxchg(addr, old, old & mask) != old);

This could be done more efficiently with the underlying LRREXH/STREXH
instructions but maybe not worth it?  Is this on any hot paths?

> +}
> +
> +void gnttab_mark_dirty(struct domain *d, unsigned long l)
> +{
> +    /* XXX: mark dirty */
> +}
> +
> +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 )
> +        p2mt = p2m_grant_map_ro;
> +    else
> +        p2mt = p2m_grant_map_rw;
> +    */
> +    rc = guest_physmap_add_page(current->domain,
> +                                 addr >> PAGE_SHIFT, frame, 0);

I'm a little worried about taking this in, in case we never get round to
making the read-only grants read-only.  Could we get away with making
that an error case or are they needed right now?

Cheers,

Tim.

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

[Tim Deegan]

At 12:05 +0100 on 04 Jul (1341403533), Stefano Stabellini wrote:
> The guest is entitled to use valid memory pages as pfns for grant_table
> usage.
> 
> 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.

Does this break other users?  I guess this answers my earlier question
about needing refcounts, anyway -- we'll need to use them everywhere
to keep the grant-table code consistent.

Cheers,

Tim.

> 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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Tim Deegan]

At 12:05 +0100 on 04 Jul (1341403534), Stefano Stabellini wrote:
> Implement XENMAPSPACE_grant_table and grant_table_op.
> 
> Introduce an HVM_PARAM to tell the guest where to map the grant table
> (HVM_PARAM_GRANT_START_PFN), similarly to what we do with
> HVM_PARAM_CONSOLE_PFN and HVM_PARAM_STORE_PFN.
> However HVM_PARAM_GRANT_START_PFN is also going to be used by dom0, so
> we set the parameter in Xen rather than libxc.
> Using HVM_PARAM_GRANT_START_PFN removes the need for a platform pci
> device.
> 
> Statically set HVM_PARAM_GRANT_START_PFN to 0xb0000000 for now.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Tim Deegan <tim@xen.org>

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

[Stefano Stabellini]

On Thu, 12 Jul 2012, Tim Deegan wrote:
> At 12:05 +0100 on 04 Jul (1341403533), Stefano Stabellini wrote:
> > The guest is entitled to use valid memory pages as pfns for grant_table
> > usage.
> > 
> > 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.
> 
> Does this break other users?

I don't think so, unless we start calling map_mmio_regions passing valid
memory addresses in construct_dom0.



> I guess this answers my earlier question
> about needing refcounts, anyway -- we'll need to use them everywhere
> to keep the grant-table code consistent.

Yeah

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

[Stefano Stabellini]

On Thu, 12 Jul 2012, Tim Deegan wrote:
> At 12:05 +0100 on 04 Jul (1341403532), Stefano Stabellini wrote:
> > 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.
> 
> Do we actually need these refcounts on ARM?  Or was it just the
> typecount we thought we could do without?

Nope, we need them for the grant_table.


> > +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. 
> > +         * Count == -2: Remaining unused ref is reserved for get_page_light().
> > +         */
> > +        if ( unlikely(((x + 2) & PGC_count_mask) <= 2) )
> > +            return NULL;
> > +    }
> > +    while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
> 
> get_page_light() is an x86-ism, so we don't need to leave room for it here. 

I'll remove it


> > +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;
> 
> A hard tab has snuck in here.  

I'll fix


> > +	} while (cmpxchg(addr, old, old & mask) != old);
> 
> This could be done more efficiently with the underlying LRREXH/STREXH
> instructions but maybe not worth it?  Is this on any hot paths?

It is called whem mapping/unmapping grant references, I don't think it
counts as hot path.


> > +}
> > +
> > +void gnttab_mark_dirty(struct domain *d, unsigned long l)
> > +{
> > +    /* XXX: mark dirty */
> > +}
> > +
> > +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 )
> > +        p2mt = p2m_grant_map_ro;
> > +    else
> > +        p2mt = p2m_grant_map_rw;
> > +    */
> > +    rc = guest_physmap_add_page(current->domain,
> > +                                 addr >> PAGE_SHIFT, frame, 0);
> 
> I'm a little worried about taking this in, in case we never get round to
> making the read-only grants read-only.  Could we get away with making
> that an error case or are they needed right now?

Good idea, I'll return an error.