thr3ads.net - Xen devel - [PATCH v2 0/6] xen/arm: grant

If this information is useful, please help other people find it:
Share via:

Stefano Stabellini

2012-Jul-20 15:05 UTC

[PATCH v2 0/6] xen/arm: grant_table

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 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 (6):
      Initialize lr_queue in vcpu_vgic_init for the first 32 irqs
      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: implement get/put_page_type
      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            |   13 --
 xen/arch/arm/mm.c               |  230 ++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/p2m.c              |   82 +++++++++-----
 xen/arch/arm/traps.c            |    1 +
 xen/arch/arm/vgic.c             |    3 +
 xen/include/asm-arm/mm.h        |    1 -
 xen/include/asm-arm/paging.h    |    4 +-
 xen/include/public/hvm/params.h |    4 +-
 9 files changed, 294 insertions(+), 47 deletions(-)


Cheers,

Stefano

Stefano Stabellini

2012-Jul-20 15:05 UTC

head link

[PATCH v2 1/6] Initialize lr_queue in vcpu_vgic_init for the first 32 irqs

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

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c0c7e81..b383e01 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -102,7 +102,10 @@ int vcpu_vgic_init(struct vcpu *v)
 
     memset(&v->arch.vgic.pending_irqs, 0,
sizeof(v->arch.vgic.pending_irqs));
     for (i = 0; i < 32; i++)
+    {
         INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight);
+        INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
+    }
     printk("vcpu_vgic_init irq[0] at %p desc is %p\n",
            &v->arch.vgic.pending_irqs[0],
            v->arch.vgic.pending_irqs[0].desc);
-- 
1.7.2.5

Stefano Stabellini

2012-Jul-20 15:05 UTC

head link

[PATCH v2 2/6] xen/arm: set paging_mode_translate and paging_mode_external

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

Stefano Stabellini

2012-Jul-20 15:05 UTC

head link

[PATCH v2 3/6] xen/arm: implement page reference and grant table functions needed by grant_table.c

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 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    |  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 1832e7f..01a6781 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -555,6 +555,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. 
+         */
+        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 */
+}
+
+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 )
+    {
+        printk("%s: read only mappings not implemented yet\n",
__func__);
+        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 6066aac..a4c7e6f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -156,8 +156,14 @@ static int p2m_create_entry(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,
@@ -227,25 +233,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;
         }
     }
 
@@ -265,7 +285,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,
@@ -273,7 +293,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,
@@ -290,7 +310,7 @@ int guest_physmap_add_page(struct domain *d,
         once = 0;
     }
 
-    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);
 }
@@ -299,7 +319,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)
@@ -348,6 +370,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

Stefano Stabellini

2012-Jul-20 15:05 UTC

head link

[PATCH v2 4/6] xen/arm: implement get/put_page_type

Add a basic get_page_type and put_page_type implementation: the
implementation is similar to the x86 one, without all the code to handle
shadow pagetables and other unneeded features.

Also remove PGT_shared_page, that is unused.



Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/dummy.S     |    4 --
 xen/arch/arm/mm.c        |   91 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/mm.h |    1 -
 3 files changed, 91 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 01a6781..7033023 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -28,8 +28,10 @@
 #include <xen/guest_access.h>
 #include <asm/page.h>
 #include <asm/current.h>
+#include <asm/flushtlb.h>
 #include <public/memory.h>
 #include <xen/sched.h>
+#include <xen/perfc.h>
 
 struct domain *dom_xen, *dom_io;
 
@@ -604,6 +606,95 @@ int get_page(struct page_info *page, struct domain *domain)
     return 0;
 }
 
+void put_page_type(struct page_info *page)
+{
+    unsigned long nx, x, y = page->u.inuse.type_info;
+
+    for ( ; ; )
+    {
+        x  = y;
+        nx = x - 1;
+
+        ASSERT((x & PGT_count_mask) != 0);
+
+        if ( unlikely((nx & PGT_count_mask) == 0) )
+            /*
+             * Record TLB information for flush later
+             */
+            page->tlbflush_timestamp = tlbflush_current_time();
+
+        if ( likely((y = cmpxchg(&page->u.inuse.type_info, x, nx)) == x)
)
+            break;
+    }
+}
+
+int get_page_type(struct page_info *page, unsigned long type)
+{
+    unsigned long nx, x, y = page->u.inuse.type_info;
+
+    for ( ; ; )
+    {
+        x  = y;
+        nx = x + 1;
+        if ( unlikely((nx & PGT_count_mask) == 0) )
+        {
+            printk("Type count overflow on pfn %lx",
page_to_mfn(page));
+            return -EINVAL;
+        }
+        else if ( (x & PGT_count_mask) == 0 )
+        {
+            struct domain *d = page_get_owner(page);
+
+            if ( (x & PGT_type_mask) != type )
+            {
+                /*
+                 * On type change we check to flush stale TLB entries. This 
+                 * may be unnecessary (e.g., page was GDT/LDT) but those 
+                 * circumstances should be very rare.
+                 */
+                cpumask_t mask;
+
+                cpumask_copy(&mask, d->domain_dirty_cpumask);
+
+                /* Don''t flush if the timestamp is old enough */
+                tlbflush_filter(mask, page->tlbflush_timestamp);
+
+                if ( unlikely(!cpumask_empty(&mask)) )
+                {
+                    perfc_incr(need_flush_tlb_flush);
+                    flush_tlb_mask(&mask);
+                }
+
+                /* We lose existing type and validity. */
+                nx &= ~(PGT_type_mask | PGT_validated);
+                nx |= type;
+
+                /* No special validation needed for writable pages. */
+                /* Page tables and GDT/LDT need to be scanned for validity. */
+                if ( type == PGT_writable_page )
+                    nx |= PGT_validated;
+            }
+        }
+        else if ( unlikely(!(x & PGT_validated)) )
+        {
+                /* Someone else is updating validation of this page. Wait... */
+                while ( (y = page->u.inuse.type_info) == x )
+                {
+                    cpu_relax();
+                }
+                continue;
+        }
+
+        if ( likely((y = cmpxchg(&page->u.inuse.type_info, x, nx)) == x)
)
+            break;
+    }
+
+    if ( unlikely(!(nx & PGT_validated)) )
+        BUG();
+
+    return 1;
+}
+
 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

Stefano Stabellini

2012-Jul-20 15:05 UTC

head link

[PATCH v2 5/6] xen/arm: create_p2m_entries should not call free_domheap_page

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 a4c7e6f..fbfef72 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -225,12 +225,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

Stefano Stabellini

2012-Jul-20 15:05 UTC

head link

[PATCH v2 6/6] xen/arm: grant table

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>
---
 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 6e1e08a..7c56cf3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -345,6 +345,9 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
 
         if ( (rc = p2m_alloc_table(d)) != 0 )
             goto fail;
+
+        /* XXX: select this dynamically */
+        d->arch.hvm_domain.params[HVM_PARAM_GRANT_START_PFN] = 0xb0000;
     }
 
     if ( (rc = domain_vgic_init(d)) != 0 )
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7033023..2f28a18 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 <asm/page.h>
 #include <asm/current.h>
@@ -477,7 +478,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 )
@@ -506,6 +507,32 @@ static int xenmem_add_to_physmap_once(
         rcu_unlock_domain(od);
         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 3900545..cdbd11f 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

Ian Campbell

2012-Jul-23 16:03 UTC

head link

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

On Fri, 2012-07-20 at 16:05 +0100, 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.
Can we add ASSERT(0) or BUG() or something on those paths, so it becomes
obvious when we start hitting them.

...

Having read the patch I now see that some of them return errors, but
others don''t and I think they should do something noisy...
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 1832e7f..01a6781 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> +                    rc = -ENOMEM;
> +                    page = alloc_domheap_page(d, 0);
> +                    if ( page == NULL ) {
> +                        printk("p2m_populate_ram: failed to allocate
page\n");
Should be a gdprintk or something, since a guest can indirectly trigger
this path.

Otherwise the patch looks good to me but I''d like to see Tim''s
opinion.

Ian.

Ian Campbell

2012-Jul-23 16:08 UTC

head link

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

On Fri, 2012-07-20 at 16:05 +0100, Stefano Stabellini
wrote:> Add a basic get_page_type and put_page_type implementation: the
> implementation is similar to the x86 one, without all the code to handle
> shadow pagetables and other unneeded features.
> 
> Also remove PGT_shared_page, that is unused.
I suppose it''ll come back some day?

Is the existing page sharing stuff completely contained within the x86
arch code and not in generic code at all?> +int get_page_type(struct page_info *page, unsigned long type)
> +{
> +    unsigned long nx, x, y = page->u.inuse.type_info;
> +
> +    for ( ; ; )
> +    {
> +        x  = y;
> +        nx = x + 1;
> +        if ( unlikely((nx & PGT_count_mask) == 0) )
> +        {
> +            printk("Type count overflow on pfn %lx",
page_to_mfn(page));
> +            return -EINVAL;
> +        }
> +        else if ( (x & PGT_count_mask) == 0 )
> +        {
> +            struct domain *d = page_get_owner(page);
> +
> +            if ( (x & PGT_type_mask) != type )
> +            {
> +                /*
> +                 * On type change we check to flush stale TLB entries.
This
> +                 * may be unnecessary (e.g., page was GDT/LDT) but those 
> +                 * circumstances should be very rare.
> +                 */
> +                cpumask_t mask;
> +
> +                cpumask_copy(&mask, d->domain_dirty_cpumask);
> +
> +                /* Don''t flush if the timestamp is old enough */
> +                tlbflush_filter(mask, page->tlbflush_timestamp);
> +
> +                if ( unlikely(!cpumask_empty(&mask)) )
> +                {
> +                    perfc_incr(need_flush_tlb_flush);
> +                    flush_tlb_mask(&mask);
> +                }
> +
> +                /* We lose existing type and validity. */
> +                nx &= ~(PGT_type_mask | PGT_validated);
> +                nx |= type;
> +
> +                /* No special validation needed for writable pages. */
> +                /* Page tables and GDT/LDT need to be scanned for
validity. */
There is no GDT/LDT on ARM.

And do we really need PGT_validated when there is no direct paging
support? (Hrm, maybe I''m confused about what PGT_validated means).

In fact, do the types make any sense at all for ARM? Is PGT_writable is
really only meaningful when you have direct paging?

Ian.

Ian Campbell

2012-Jul-23 16:13 UTC

head link

Re: [PATCH v2 6/6] xen/arm: grant table

On Fri, 2012-07-20 at 16:05 +0100, 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.
In principal the guest can use whatever free (or not free) PFN space it
wants for this region, right?

I suspect hybrid on x86 has a similar requirement and if we are going to
add this sort of thing we should make sure that we can satisfy both
architectures.

Or would it be better to use existing per-arch mechanisms to describe
the memory map (E820 on x86 and DTB on ARM)? They already describe the
rest of the memory layout.

The foreign page mapping stuff also needs to find a similar PFN region
to use.
> 
> 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>
> ---
>  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 6e1e08a..7c56cf3 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -345,6 +345,9 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
>  
>          if ( (rc = p2m_alloc_table(d)) != 0 )
>              goto fail;
> +
> +        /* XXX: select this dynamically */
> +        d->arch.hvm_domain.params[HVM_PARAM_GRANT_START_PFN] = 0xb0000;
>      }
>  
>      if ( (rc = domain_vgic_init(d)) != 0 )
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7033023..2f28a18 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 <asm/page.h>
>  #include <asm/current.h>
> @@ -477,7 +478,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 )
> @@ -506,6 +507,32 @@ static int xenmem_add_to_physmap_once(
>          rcu_unlock_domain(od);
>          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 3900545..cdbd11f 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__ */

Ian Campbell

2012-Jul-23 16:24 UTC

head link

Re: [PATCH v2 1/6] Initialize lr_queue in vcpu_vgic_init for the first 32 irqs

On Fri, 2012-07-20 at 16:05 +0100, Stefano Stabellini
wrote:> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vgic.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index c0c7e81..b383e01 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -102,7 +102,10 @@ int vcpu_vgic_init(struct vcpu *v)
>  
>      memset(&v->arch.vgic.pending_irqs, 0,
sizeof(v->arch.vgic.pending_irqs));
>      for (i = 0; i < 32; i++)
> +    {
>          INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight);
> +        INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
> +    }
>      printk("vcpu_vgic_init irq[0] at %p desc is %p\n",
>             &v->arch.vgic.pending_irqs[0],
>             v->arch.vgic.pending_irqs[0].desc);

Ian Campbell

2012-Jul-23 16:25 UTC

head link

Re: [PATCH v2 5/6] xen/arm: create_p2m_entries should not call free_domheap_page

On Fri, 2012-07-20 at 16:05 +0100, Stefano Stabellini
wrote:> The guest is entitled to use valid memory pages as pfns for grant_table
> usage.
I think it would be more accurate to say that the guest is entitled to
leak a page from its p2m (by overwriting it) if it wants to. Sincethe
memory is effectively lost to it (can''t even be recovered by XENMEM
increase reservation etc). Is that right?
> 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 a4c7e6f..fbfef72 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -225,12 +225,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) {

Ian Campbell

2012-Jul-23 16:30 UTC

head link

Re: [PATCH v2 0/6] xen/arm: grant_table

On Fri, 2012-07-20 at 16:05 +0100, Stefano Stabellini
wrote:> Hi all,
> this patch series implements the basic mechanisms needed by the grant
> table to work properly.
I''ve committed the first two (minor rejects in the first. please
check).
        xen/arm: set paging_mode_translate and paging_mode_external
        Initialize lr_queue in vcpu_vgic_init for the first 32 irqs

The rest I had comments on.

Ian.

Tim Deegan

2012-Jul-23 20:01 UTC

head link

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

At 16:05 +0100 on 20 Jul (1342800356), Stefano Stabellini
wrote:> Add a basic get_page_type and put_page_type implementation: the
> implementation is similar to the x86 one, without all the code to handle
> shadow pagetables and other unneeded features.
OK, we definitely decided that ARM didn''t need page typecounts. :)  Is
this needed for some common code?

Tim.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/dummy.S     |    4 --
>  xen/arch/arm/mm.c        |   91
++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/mm.h |    1 -
>  3 files changed, 91 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 01a6781..7033023 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -28,8 +28,10 @@
>  #include <xen/guest_access.h>
>  #include <asm/page.h>
>  #include <asm/current.h>
> +#include <asm/flushtlb.h>
>  #include <public/memory.h>
>  #include <xen/sched.h>
> +#include <xen/perfc.h>
>  
>  struct domain *dom_xen, *dom_io;
>  
> @@ -604,6 +606,95 @@ int get_page(struct page_info *page, struct domain
*domain)
>      return 0;
>  }
>  
> +void put_page_type(struct page_info *page)
> +{
> +    unsigned long nx, x, y = page->u.inuse.type_info;
> +
> +    for ( ; ; )
> +    {
> +        x  = y;
> +        nx = x - 1;
> +
> +        ASSERT((x & PGT_count_mask) != 0);
> +
> +        if ( unlikely((nx & PGT_count_mask) == 0) )
> +            /*
> +             * Record TLB information for flush later
> +             */
> +            page->tlbflush_timestamp = tlbflush_current_time();
> +
> +        if ( likely((y = cmpxchg(&page->u.inuse.type_info, x, nx))
== x) )
> +            break;
> +    }
> +}
> +
> +int get_page_type(struct page_info *page, unsigned long type)
> +{
> +    unsigned long nx, x, y = page->u.inuse.type_info;
> +
> +    for ( ; ; )
> +    {
> +        x  = y;
> +        nx = x + 1;
> +        if ( unlikely((nx & PGT_count_mask) == 0) )
> +        {
> +            printk("Type count overflow on pfn %lx",
page_to_mfn(page));
> +            return -EINVAL;
> +        }
> +        else if ( (x & PGT_count_mask) == 0 )
> +        {
> +            struct domain *d = page_get_owner(page);
> +
> +            if ( (x & PGT_type_mask) != type )
> +            {
> +                /*
> +                 * On type change we check to flush stale TLB entries.
This
> +                 * may be unnecessary (e.g., page was GDT/LDT) but those 
> +                 * circumstances should be very rare.
> +                 */
> +                cpumask_t mask;
> +
> +                cpumask_copy(&mask, d->domain_dirty_cpumask);
> +
> +                /* Don''t flush if the timestamp is old enough */
> +                tlbflush_filter(mask, page->tlbflush_timestamp);
> +
> +                if ( unlikely(!cpumask_empty(&mask)) )
> +                {
> +                    perfc_incr(need_flush_tlb_flush);
> +                    flush_tlb_mask(&mask);
> +                }
> +
> +                /* We lose existing type and validity. */
> +                nx &= ~(PGT_type_mask | PGT_validated);
> +                nx |= type;
> +
> +                /* No special validation needed for writable pages. */
> +                /* Page tables and GDT/LDT need to be scanned for
validity. */
> +                if ( type == PGT_writable_page )
> +                    nx |= PGT_validated;
> +            }
> +        }
> +        else if ( unlikely(!(x & PGT_validated)) )
> +        {
> +                /* Someone else is updating validation of this page.
Wait... */
> +                while ( (y = page->u.inuse.type_info) == x )
> +                {
> +                    cpu_relax();
> +                }
> +                continue;
> +        }
> +
> +        if ( likely((y = cmpxchg(&page->u.inuse.type_info, x, nx))
== x) )
> +            break;
> +    }
> +
> +    if ( unlikely(!(nx & PGT_validated)) )
> +        BUG();
> +
> +    return 1;
> +}
> +
>  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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Stefano Stabellini

2012-Jul-24 10:21 UTC

head link

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

On Mon, 23 Jul 2012, Tim Deegan wrote:> At 16:05 +0100 on 20 Jul (1342800356), Stefano Stabellini wrote:
> > Add a basic get_page_type and put_page_type implementation: the
> > implementation is similar to the x86 one, without all the code to
handle
> > shadow pagetables and other unneeded features.
> 
> OK, we definitely decided that ARM didn''t need page typecounts. :)
Is
> this needed for some common code?
> 
Yes, they are needed by grant_table.c :)
I was very tempted to just implement get_page_type as "return 1".

Stefano Stabellini

2012-Jul-24 10:25 UTC

head link

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

On Mon, 23 Jul 2012, Ian Campbell wrote:> On Fri, 2012-07-20 at 16:05 +0100, 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.
> 
> Can we add ASSERT(0) or BUG() or something on those paths, so it becomes
> obvious when we start hitting them.
> 
> ...
> 
> Having read the patch I now see that some of them return errors, but
> others don''t and I think they should do something noisy...
The only path that is unimplemented is in create_grant_host_mapping, it is
marked with "XXX", and prints an error message and returns an error.

> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 1832e7f..01a6781 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > +                    rc = -ENOMEM;
> > +                    page = alloc_domheap_page(d, 0);
> > +                    if ( page == NULL ) {
> > +                        printk("p2m_populate_ram: failed to
allocate page\n");
> 
> Should be a gdprintk or something, since a guest can indirectly trigger
> this path.
yes, good idea.

Stefano Stabellini

2012-Jul-24 10:27 UTC

head link

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

On Mon, 23 Jul 2012, Ian Campbell wrote:> On Fri, 2012-07-20 at 16:05 +0100, Stefano Stabellini wrote:
> > Add a basic get_page_type and put_page_type implementation: the
> > implementation is similar to the x86 one, without all the code to
handle
> > shadow pagetables and other unneeded features.
> > 
> > Also remove PGT_shared_page, that is unused.
> 
> I suppose it''ll come back some day?
Only if we want to: it is not needed by common code.

> Is the existing page sharing stuff completely contained within the x86
> arch code and not in generic code at all?
Yes.

> > +int get_page_type(struct page_info *page, unsigned long type)
> > +{
> > +    unsigned long nx, x, y = page->u.inuse.type_info;
> > +
> > +    for ( ; ; )
> > +    {
> > +        x  = y;
> > +        nx = x + 1;
> > +        if ( unlikely((nx & PGT_count_mask) == 0) )
> > +        {
> > +            printk("Type count overflow on pfn %lx",
page_to_mfn(page));
> > +            return -EINVAL;
> > +        }
> > +        else if ( (x & PGT_count_mask) == 0 )
> > +        {
> > +            struct domain *d = page_get_owner(page);
> > +
> > +            if ( (x & PGT_type_mask) != type )
> > +            {
> > +                /*
> > +                 * On type change we check to flush stale TLB
entries. This
> > +                 * may be unnecessary (e.g., page was GDT/LDT) but
those
> > +                 * circumstances should be very rare.
> > +                 */
> > +                cpumask_t mask;
> > +
> > +                cpumask_copy(&mask, d->domain_dirty_cpumask);
> > +
> > +                /* Don''t flush if the timestamp is old
enough */
> > +                tlbflush_filter(mask, page->tlbflush_timestamp);
> > +
> > +                if ( unlikely(!cpumask_empty(&mask)) )
> > +                {
> > +                    perfc_incr(need_flush_tlb_flush);
> > +                    flush_tlb_mask(&mask);
> > +                }
> > +
> > +                /* We lose existing type and validity. */
> > +                nx &= ~(PGT_type_mask | PGT_validated);
> > +                nx |= type;
> > +
> > +                /* No special validation needed for writable pages.
*/
> > +                /* Page tables and GDT/LDT need to be scanned for
validity. */
> 
> There is no GDT/LDT on ARM.
> 
> And do we really need PGT_validated when there is no direct paging
> support? (Hrm, maybe I''m confused about what PGT_validated means).
> 
> In fact, do the types make any sense at all for ARM? Is PGT_writable is
> really only meaningful when you have direct paging?
I am starting to think that a better way to implement get_page_type is:

int get_page_type(struct page_info *page, unsigned long type)
{
    return 1;
}

Stefano Stabellini

2012-Jul-24 10:28 UTC

head link

Re: [PATCH v2 5/6] xen/arm: create_p2m_entries should not call free_domheap_page

On Mon, 23 Jul 2012, Ian Campbell wrote:> On Fri, 2012-07-20 at 16:05 +0100, Stefano Stabellini wrote:
> > The guest is entitled to use valid memory pages as pfns for
grant_table
> > usage.
> 
> I think it would be more accurate to say that the guest is entitled to
> leak a page from its p2m (by overwriting it) if it wants to. Sincethe
> memory is effectively lost to it (can''t even be recovered by
XENMEM
> increase reservation etc). Is that right?
Yes, it is correct.

Stefano Stabellini

2012-Jul-24 10:30 UTC

head link

Re: [PATCH v2 6/6] xen/arm: grant table

On Mon, 23 Jul 2012, Ian Campbell wrote:> On Fri, 2012-07-20 at 16:05 +0100, 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.
> 
> In principal the guest can use whatever free (or not free) PFN space it
> wants for this region, right?
> 
> I suspect hybrid on x86 has a similar requirement and if we are going to
> add this sort of thing we should make sure that we can satisfy both
> architectures.
> 
> Or would it be better to use existing per-arch mechanisms to describe
> the memory map (E820 on x86 and DTB on ARM)? They already describe the
> rest of the memory layout.
> 
> The foreign page mapping stuff also needs to find a similar PFN region
> to use.
I think that you are right.
In fact I have a prototype implementation using a memory range specified
under a "/xen" node in the DT.

Ian Campbell

2012-Jul-24 10:41 UTC

head link

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

On Tue, 2012-07-24 at 11:25 +0100, Stefano Stabellini
wrote:> On Mon, 23 Jul 2012, Ian Campbell wrote:
> > On Fri, 2012-07-20 at 16:05 +0100, 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.
> > 
> > Can we add ASSERT(0) or BUG() or something on those paths, so it
becomes
> > obvious when we start hitting them.
> > 
> > ...
> > 
> > Having read the patch I now see that some of them return errors, but
> > others don''t and I think they should do something noisy...
> 
> The only path that is unimplemented is in create_grant_host_mapping, it is
> marked with "XXX", and prints an error message and returns an
error.
What about gnttab_mark_dirty?
> 
> 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 1832e7f..01a6781 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > +                    rc = -ENOMEM;
> > > +                    page = alloc_domheap_page(d, 0);
> > > +                    if ( page == NULL ) {
> > > +                        printk("p2m_populate_ram: failed to
allocate page\n");
> > 
> > Should be a gdprintk or something, since a guest can indirectly
trigger
> > this path.
> 
> yes, good idea.

Ian Campbell

2012-Jul-24 10:42 UTC

head link

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

On Tue, 2012-07-24 at 11:27 +0100, Stefano Stabellini
wrote:> > In fact, do the types make any sense at all for ARM? Is PGT_writable
is
> > really only meaningful when you have direct paging?
> 
> I am starting to think that a better way to implement get_page_type is:
> 
> int get_page_type(struct page_info *page, unsigned long type)
> {
>     return 1;
> }
Would you need to take/release a regular reference count too? Or does
common code always ensure it has a normal reference when manipulating
types?

Ian.

Stefano Stabellini

2012-Jul-26 17:56 UTC

head link

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

On Tue, 24 Jul 2012, Ian Campbell wrote:> On Tue, 2012-07-24 at 11:25 +0100, Stefano Stabellini wrote:
> > On Mon, 23 Jul 2012, Ian Campbell wrote:
> > > On Fri, 2012-07-20 at 16:05 +0100, 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.
> > > 
> > > Can we add ASSERT(0) or BUG() or something on those paths, so it
becomes
> > > obvious when we start hitting them.
> > > 
> > > ...
> > > 
> > > Having read the patch I now see that some of them return errors,
but
> > > others don''t and I think they should do something
noisy...
> > 
> > The only path that is unimplemented is in create_grant_host_mapping,
it is
> > marked with "XXX", and prints an error message and returns
an error.
> 
> What about gnttab_mark_dirty?
The problem with gnttab_mark_dirty is that it is called rather often.
I''ll add a static variable so that it is going to print a warning only
the first time it is called.

Stefano Stabellini

2012-Jul-26 18:03 UTC

head link

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

On Tue, 24 Jul 2012, Ian Campbell wrote:> On Tue, 2012-07-24 at 11:27 +0100, Stefano Stabellini wrote:
> > > In fact, do the types make any sense at all for ARM? Is
PGT_writable is
> > > really only meaningful when you have direct paging?
> > 
> > I am starting to think that a better way to implement get_page_type
is:
> > 
> > int get_page_type(struct page_info *page, unsigned long type)
> > {
> >     return 1;
> > }
> 
> Would you need to take/release a regular reference count too? Or does
> common code always ensure it has a normal reference when manipulating
> types?
It seems to me that the common code always takes a normal reference to
the page before increasing the typecount.

Ian Campbell

2012-Jul-27 07:57 UTC

head link

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

On Thu, 2012-07-26 at 18:56 +0100, Stefano Stabellini
wrote:> On Tue, 24 Jul 2012, Ian Campbell wrote:
> > On Tue, 2012-07-24 at 11:25 +0100, Stefano Stabellini wrote:
> > > On Mon, 23 Jul 2012, Ian Campbell wrote:
> > > > On Fri, 2012-07-20 at 16:05 +0100, 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.
> > > > 
> > > > Can we add ASSERT(0) or BUG() or something on those paths,
so it becomes
> > > > obvious when we start hitting them.
> > > > 
> > > > ...
> > > > 
> > > > Having read the patch I now see that some of them return
errors, but
> > > > others don''t and I think they should do something
noisy...
> > > 
> > > The only path that is unimplemented is in
create_grant_host_mapping, it is
> > > marked with "XXX", and prints an error message and
returns an error.
> > 
> > What about gnttab_mark_dirty?
> 
> The problem with gnttab_mark_dirty is that it is called rather often.
> I''ll add a static variable so that it is going to print a warning
only
> the first time it is called.
That sounds good.

Ian.

Xen devel - Jul 2012 - [PATCH v2 0/6] xen/arm: grant_table

[PATCH v2 0/6] xen/arm: grant_table

[PATCH v2 1/6] Initialize lr_queue in vcpu_vgic_init for the first 32 irqs

[PATCH v2 2/6] xen/arm: set paging_mode_translate and paging_mode_external

[PATCH v2 3/6] xen/arm: implement page reference and grant table functions needed by grant_table.c

[PATCH v2 4/6] xen/arm: implement get/put_page_type

[PATCH v2 5/6] xen/arm: create_p2m_entries should not call free_domheap_page

[PATCH v2 6/6] xen/arm: grant table

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

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

Re: [PATCH v2 6/6] xen/arm: grant table

Re: [PATCH v2 1/6] Initialize lr_queue in vcpu_vgic_init for the first 32 irqs

Re: [PATCH v2 5/6] xen/arm: create_p2m_entries should not call free_domheap_page

Re: [PATCH v2 0/6] xen/arm: grant_table

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

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

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

Re: [PATCH v2 5/6] xen/arm: create_p2m_entries should not call free_domheap_page

Re: [PATCH v2 6/6] xen/arm: grant table

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

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

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

Re: [PATCH v2 4/6] xen/arm: implement get/put_page_type

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