All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V15 0/9] Mem_access for ARM
@ 2015-04-20 15:06 Tamas K Lengyel
  2015-04-20 15:06 ` [PATCH V15 1/9] xen/arm: groundwork for mem_access support on ARM Tamas K Lengyel
                   ` (9 more replies)
  0 siblings, 10 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

The ARM virtualization extension provides 2-stage paging, a similar mechanisms
to Intel's EPT, which can be used to trace the memory accesses performed by
the guest systems. This series sets up the necessary infrastructure in the
ARM code to deliver the event on R/W/X traps. Finally, we turn on the
compilation of mem_access and mem_event on ARM and perform the necessary
changes to the tools side.

This version of the series is based on master and each patch in the series has
been compile tested on both ARM and x86.

This PATCH series is also available at:
https://github.com/tklengyel/xen/tree/arm_memaccess15

Julien Grall (1):
  xen/arm: Implement domain_get_maximum_gpfn

Tamas K Lengyel (8):
  xen/arm: groundwork for mem_access support on ARM
  xen/arm: Allow hypervisor access to mem_access protected pages
  xen/arm: Data abort exception (R/W) mem_access events
  xen/arm: Instruction prefetch abort (X) mem_access event handling
  xen: Make gpfn related memops compatible with wider return values
  tools/libxc: Allocate magic page for mem access on ARM
  tools/tests: Enable xen-access on ARM
  xen/arm: Enable mem_access on ARM

 config/arm32.mk                         |   1 +
 config/arm64.mk                         |   1 +
 tools/libxc/include/xenguest.h          |   2 +-
 tools/libxc/xc_core.h                   |   4 +-
 tools/libxc/xc_core_arm.c               |   6 +-
 tools/libxc/xc_core_x86.c               |   8 +-
 tools/libxc/xc_dom_arm.c                |   6 +-
 tools/libxc/xc_domain.c                 |  14 +-
 tools/libxc/xc_domain_save.c            |   3 +-
 tools/libxc/xg_private.h                |   2 +-
 tools/tests/mce-test/tools/xen-mceinj.c |  28 +-
 tools/tests/xen-access/Makefile         |   4 +-
 tools/tests/xen-access/xen-access.c     |  38 +-
 xen/arch/arm/mm.c                       |   2 +-
 xen/arch/arm/p2m.c                      | 604 +++++++++++++++++++++++++++++---
 xen/arch/arm/traps.c                    |  78 ++++-
 xen/common/memory.c                     |  16 +-
 xen/include/asm-arm/arm32/page.h        |   7 +-
 xen/include/asm-arm/arm64/page.h        |   7 +-
 xen/include/asm-arm/domain.h            |   1 +
 xen/include/asm-arm/p2m.h               |  32 +-
 xen/include/asm-arm/page.h              |   4 +-
 xen/include/asm-arm/processor.h         |  13 +-
 xen/include/asm-x86/p2m.h               |  10 -
 xen/include/public/memory.h             |  24 +-
 xen/include/xen/p2m-common.h            |  14 +
 26 files changed, 796 insertions(+), 133 deletions(-)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH V15 1/9] xen/arm: groundwork for mem_access support on ARM
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-20 15:06 ` [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

Add necessary changes for page table construction routines to pass
the default access information and hypercall continuation mask. Also,
define necessary functions and data fields to be used later by mem_access.

The p2m_access_t info will be stored in a Radix tree as the PTE lacks
enough software programmable bits, thus in this patch we add the radix-tree
construction/destruction portions. The tree itself will be used later
by mem_access.

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
v14: - Update patch message to be more descriptive.
v13: - Rename access_in_use to mem_access_enabled.
     - Define p2m_get_mem_access function prototype but
        return -ENOSYS for now.
v11: - Move including common/mem_event.h down the series.
v10: - Typo fix and drop reshuffling things that no longer need
      shuffling.
v8: - Drop lock inputs as common mem_access_check is postponed.
    - Resurrect the radix tree with an extra boolean access_in_use flag
      to indicate if the tree is empty to avoid lookups.
v7: - Remove radix tree init/destroy and move p2m_access_t store to page_info.
    - Add p2m_gpfn_lock/unlock functions.
    - Add bool_t lock input to p2m_lookup and apply_p2m_changes so the caller
      can specify if locking should be performed. This is needed in order to
      support mem_access_check from common.
v6: - Move mem_event header include to first patch that needs it.
v5: - #include grouping style-fix.
v4: - Move p2m_get_hostp2m definition here.
---
 xen/arch/arm/p2m.c              | 52 +++++++++++++++++++++++++++--------------
 xen/include/asm-arm/domain.h    |  1 +
 xen/include/asm-arm/p2m.h       | 35 ++++++++++++++++++++++++++-
 xen/include/asm-arm/processor.h |  2 +-
 4 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8809f5a..137e5a0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -305,7 +305,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
 }
 
 static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
-                               p2m_type_t t)
+                               p2m_type_t t, p2m_access_t a)
 {
     paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
     /* sh, xn and write bit will be defined in the following switches
@@ -335,8 +335,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
         break;
     }
 
-    /* We pass p2m_access_rwx as a placeholder for now. */
-    p2m_set_permission(&e, t, p2m_access_rwx);
+    p2m_set_permission(&e, t, a);
 
     ASSERT(!(pa & ~PAGE_MASK));
     ASSERT(!(pa & ~PADDR_MASK));
@@ -394,7 +393,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
          for ( i=0 ; i < LPAE_ENTRIES; i++ )
          {
              pte = mfn_to_p2m_entry(base_pfn + (i<<(level_shift-LPAE_SHIFT)),
-                                    MATTR_MEM, t);
+                                    MATTR_MEM, t, p2m->default_access);
 
              /*
               * First and second level super pages set p2m.table = 0, but
@@ -414,7 +413,8 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
 
     unmap_domain_page(p);
 
-    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
+    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid,
+                           p2m->default_access);
 
     p2m_write_pte(entry, pte, flush_cache);
 
@@ -537,7 +537,8 @@ static int apply_one_level(struct domain *d,
                            paddr_t *maddr,
                            bool_t *flush,
                            int mattr,
-                           p2m_type_t t)
+                           p2m_type_t t,
+                           p2m_access_t a)
 {
     const paddr_t level_size = level_sizes[level];
     const paddr_t level_mask = level_masks[level];
@@ -566,7 +567,7 @@ static int apply_one_level(struct domain *d,
             page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
             if ( page )
             {
-                pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
+                pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
                 if ( level < 3 )
                     pte.p2m.table = 0;
                 p2m_write_pte(entry, pte, flush_cache);
@@ -601,7 +602,7 @@ static int apply_one_level(struct domain *d,
              (level == 3 || !p2m_table(orig_pte)) )
         {
             /* New mapping is superpage aligned, make it */
-            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
+            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
             if ( level < 3 )
                 pte.p2m.table = 0; /* Superpage entry */
 
@@ -770,7 +771,9 @@ static int apply_p2m_changes(struct domain *d,
                      paddr_t end_gpaddr,
                      paddr_t maddr,
                      int mattr,
-                     p2m_type_t t)
+                     uint32_t mask,
+                     p2m_type_t t,
+                     p2m_access_t a)
 {
     int rc, ret;
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -863,7 +866,7 @@ static int apply_p2m_changes(struct domain *d,
                                   level, flush_pt, op,
                                   start_gpaddr, end_gpaddr,
                                   &addr, &maddr, &flush,
-                                  mattr, t);
+                                  mattr, t, a);
             if ( ret < 0 ) { rc = ret ; goto out; }
             count += ret;
             /* L3 had better have done something! We cannot descend any further */
@@ -921,7 +924,7 @@ out:
          */
         apply_p2m_changes(d, REMOVE,
                           start_gpaddr, addr + level_sizes[level], orig_maddr,
-                          mattr, p2m_invalid);
+                          mattr, 0, p2m_invalid, d->arch.p2m.default_access);
     }
 
     for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
@@ -940,7 +943,8 @@ int p2m_populate_ram(struct domain *d,
                      paddr_t end)
 {
     return apply_p2m_changes(d, ALLOCATE, start, end,
-                             0, MATTR_MEM, p2m_ram_rw);
+                             0, MATTR_MEM, 0, p2m_ram_rw,
+                             d->arch.p2m.default_access);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -952,7 +956,8 @@ int map_mmio_regions(struct domain *d,
                              pfn_to_paddr(start_gfn),
                              pfn_to_paddr(start_gfn + nr),
                              pfn_to_paddr(mfn),
-                             MATTR_DEV, p2m_mmio_direct);
+                             MATTR_DEV, 0, p2m_mmio_direct,
+                             d->arch.p2m.default_access);
 }
 
 int unmap_mmio_regions(struct domain *d,
@@ -964,7 +969,8 @@ int unmap_mmio_regions(struct domain *d,
                              pfn_to_paddr(start_gfn),
                              pfn_to_paddr(start_gfn + nr),
                              pfn_to_paddr(mfn),
-                             MATTR_DEV, p2m_invalid);
+                             MATTR_DEV, 0, p2m_invalid,
+                             d->arch.p2m.default_access);
 }
 
 int guest_physmap_add_entry(struct domain *d,
@@ -976,7 +982,8 @@ int guest_physmap_add_entry(struct domain *d,
     return apply_p2m_changes(d, INSERT,
                              pfn_to_paddr(gpfn),
                              pfn_to_paddr(gpfn + (1 << page_order)),
-                             pfn_to_paddr(mfn), MATTR_MEM, t);
+                             pfn_to_paddr(mfn), MATTR_MEM, 0, t,
+                             d->arch.p2m.default_access);
 }
 
 void guest_physmap_remove_page(struct domain *d,
@@ -986,7 +993,8 @@ void guest_physmap_remove_page(struct domain *d,
     apply_p2m_changes(d, REMOVE,
                       pfn_to_paddr(gpfn),
                       pfn_to_paddr(gpfn + (1<<page_order)),
-                      pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
+                      pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid,
+                      d->arch.p2m.default_access);
 }
 
 int p2m_alloc_table(struct domain *d)
@@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
 
     p2m_free_vmid(d);
 
+    radix_tree_destroy(&p2m->mem_access_settings, NULL);
+
     spin_unlock(&p2m->lock);
 }
 
@@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
     p2m->max_mapped_gfn = 0;
     p2m->lowest_mapped_gfn = ULONG_MAX;
 
+    p2m->default_access = p2m_access_rwx;
+    p2m->mem_access_enabled = false;
+    radix_tree_init(&p2m->mem_access_settings);
+
 err:
     spin_unlock(&p2m->lock);
 
@@ -1129,7 +1143,8 @@ int relinquish_p2m_mapping(struct domain *d)
                               pfn_to_paddr(p2m->lowest_mapped_gfn),
                               pfn_to_paddr(p2m->max_mapped_gfn),
                               pfn_to_paddr(INVALID_MFN),
-                              MATTR_MEM, p2m_invalid);
+                              MATTR_MEM, 0, p2m_invalid,
+                              d->arch.p2m.default_access);
 }
 
 int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
@@ -1143,7 +1158,8 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
                              pfn_to_paddr(start_mfn),
                              pfn_to_paddr(end_mfn),
                              pfn_to_paddr(INVALID_MFN),
-                             MATTR_MEM, p2m_invalid);
+                             MATTR_MEM, 0, p2m_invalid,
+                             d->arch.p2m.default_access);
 }
 
 unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 9e0419e..f1a087e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -17,6 +17,7 @@ struct hvm_domain
 {
     uint64_t              params[HVM_NR_PARAMS];
     struct hvm_iommu      iommu;
+    bool_t                introspection_enabled;
 }  __cacheline_aligned;
 
 #ifdef CONFIG_ARM_64
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 11efb5b..fec191f 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -2,8 +2,9 @@
 #define _XEN_P2M_H
 
 #include <xen/mm.h>
-
+#include <xen/radix-tree.h>
 #include <xen/p2m-common.h>
+#include <public/memory.h>
 
 #define paddr_bits PADDR_BITS
 
@@ -48,6 +49,18 @@ struct p2m_domain {
     /* If true, and an access fault comes in and there is no vm_event listener,
      * pause domain. Otherwise, remove access restrictions. */
     bool_t access_required;
+
+    /* Defines if mem_access is in use for the domain. */
+    bool_t mem_access_enabled;
+
+    /* Default P2M access type for each page in the the domain: new pages,
+     * swapped in pages, cleared pages, and pages that are ambiguously
+     * retyped get this access type. See definition of p2m_access_t. */
+    p2m_access_t default_access;
+
+    /* Radix tree to store the p2m_access_t settings as the pte's don't have
+     * enough available bits to store this information. */
+    struct radix_tree_root mem_access_settings;
 };
 
 /* List of possible type for each page in the p2m entry.
@@ -225,6 +238,26 @@ static inline int get_page_and_type(struct page_info *page,
 /* get host p2m table */
 #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
 
+/* mem_event and mem_access are supported on any ARM guest */
+static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
+{
+    return 1;
+}
+
+static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
+{
+    return 1;
+}
+
+/* Get access type for a pfn
+ * If pfn == -1ul, gets the default access type */
+static inline
+int p2m_get_mem_access(struct domain *d, unsigned long pfn,
+                       xenmem_access_t *access)
+{
+    return -ENOSYS;
+}
+
 #endif /* _XEN_P2M_H */
 
 /*
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 5ccf61f..905c479 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -441,7 +441,7 @@ union hsr {
     struct hsr_dabt {
         unsigned long dfsc:6;  /* Data Fault Status Code */
         unsigned long write:1; /* Write / not Read */
-        unsigned long s1ptw:1; /* */
+        unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
         unsigned long cache:1; /* Cache Maintenance */
         unsigned long eat:1;   /* External Abort Type */
 #ifdef CONFIG_ARM_32
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
  2015-04-20 15:06 ` [PATCH V15 1/9] xen/arm: groundwork for mem_access support on ARM Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-21 13:14   ` Ian Campbell
  2015-04-20 15:06 ` [PATCH V15 3/9] xen/arm: Data abort exception (R/W) mem_access events Tamas K Lengyel
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

The hypervisor may use the MMU to verify that the given guest has read/write
access to a given page during hypercalls. As we may have custom mem_access
permissions set on these pages, we do a software-based type checking in case
the MMU based approach failed, but only if mem_access_enabled is set.

These memory accesses are not forwarded to the mem_event listener. Accesses
performed by the hypervisor are currently not part of the mem_access scheme.
This is consistent behaviour with the x86 side as well.

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
---
v15: - Make p2m_lookup a locking-wrapper around non-locking __p2m_lookup
v14: - Move software-based lookup into p2m, add comments and clean it up a bit
       Only page type allowed is rw
       Extend gva_to_ipa to take flags input for lookup validation
v12: - Check for mfn_valid as well.
---
 xen/arch/arm/p2m.c               | 118 +++++++++++++++++++++++++++++++++++++--
 xen/arch/arm/traps.c             |   2 +-
 xen/include/asm-arm/arm32/page.h |   7 ++-
 xen/include/asm-arm/arm64/page.h |   7 ++-
 xen/include/asm-arm/page.h       |   4 +-
 5 files changed, 127 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 137e5a0..f62d283 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -139,7 +139,7 @@ void flush_tlb_domain(struct domain *d)
  * There are no processor functions to do a stage 2 only lookup therefore we
  * do a a software walk.
  */
-paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
+static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     const unsigned int offsets[4] = {
@@ -179,8 +179,6 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
     else
         root_table = 0;
 
-    spin_lock(&p2m->lock);
-
     map = __map_domain_page(p2m->root + root_table);
 
     ASSERT(P2M_ROOT_LEVEL < 4);
@@ -215,11 +213,22 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
         *t = pte.p2m.type;
     }
 
-    spin_unlock(&p2m->lock);
 err:
     return maddr;
 }
 
+paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
+{
+    paddr_t ret;
+    struct p2m_domain *p2m = &d->arch.p2m;
+
+    spin_lock(&p2m->lock);
+    ret = __p2m_lookup(d, paddr, t);
+    spin_unlock(&p2m->lock);
+
+    return ret;
+}
+
 int guest_physmap_mark_populate_on_demand(struct domain *d,
                                           unsigned long gfn,
                                           unsigned int order)
@@ -1168,6 +1177,103 @@ unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
     return p >> PAGE_SHIFT;
 }
 
+/*
+ * If mem_access is in use it might have been the reason why get_page_from_gva
+ * failed to fetch the page, as it uses the MMU for the permission checking.
+ * Only in these cases we do a software-based type check and fetch the page if
+ * we indeed found a conflicting mem_access setting.
+ */
+static struct page_info*
+p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
+{
+    long rc;
+    paddr_t ipa;
+    unsigned long maddr;
+    unsigned long mfn;
+    xenmem_access_t xma;
+    p2m_type_t t;
+    struct page_info *page = NULL;
+
+    rc = gva_to_ipa(gva, &ipa, flag);
+    if ( rc < 0 )
+        goto err;
+
+    /*
+     * We do this first as this is faster in the default case when no
+     * permission is set on the page.
+     */
+    rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
+    if ( rc < 0 )
+        goto err;
+
+    /* Let's check if mem_access limited the access. */
+    switch ( xma )
+    {
+    default:
+    case XENMEM_access_rwx:
+    case XENMEM_access_rw:
+        /*
+         * If mem_access contains no rw perm restrictions at all then the original
+         * fault was correct.
+         */
+        goto err;
+    case XENMEM_access_n2rwx:
+    case XENMEM_access_n:
+    case XENMEM_access_x:
+        /*
+         * If no r/w is permitted by mem_access, this was a fault caused by mem_access.
+         */
+        break;
+    case XENMEM_access_wx:
+    case XENMEM_access_w:
+        /*
+         * If this was a read then it was because of mem_access, but if it was
+         * a write then the original get_page_from_gva fault was correct.
+         */
+        if ( flag == GV2M_READ )
+            break;
+        else
+            goto err;
+    case XENMEM_access_rx2rw:
+    case XENMEM_access_rx:
+    case XENMEM_access_r:
+        /*
+         * If this was a write then it was because of mem_access, but if it was
+         * a read then the original get_page_from_gva fault was correct.
+         */
+        if ( flag == GV2M_WRITE )
+            break;
+        else
+            goto err;
+    }
+
+    /*
+     * We had a mem_access permission limiting the access, but the page type
+     * could also be limiting, so we need to check that as well.
+     */
+    maddr = __p2m_lookup(current->domain, ipa, &t);
+    if ( maddr == INVALID_PADDR )
+        goto err;
+
+    mfn = maddr >> PAGE_SHIFT;
+    if ( !mfn_valid(mfn) )
+        goto err;
+
+    /*
+     * Base type doesn't allow r/w
+     */
+    if ( t != p2m_ram_rw )
+        goto err;
+
+    page = mfn_to_page(mfn);
+
+    if ( unlikely(!get_page(page, current->domain)) )
+        page = NULL;
+
+err:
+    return page;
+}
+
 struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
                                     unsigned long flags)
 {
@@ -1208,7 +1314,11 @@ struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
         page = NULL;
 
 err:
+    if ( !page && p2m->mem_access_enabled )
+        page = p2m_mem_access_check_and_get_page(va, flags);
+
     spin_unlock(&p2m->lock);
+
     return page;
 }
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index aaa9d93..e5d762b 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2026,7 +2026,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     if (dabt.s1ptw)
         goto bad_data_abort;
 
-    rc = gva_to_ipa(info.gva, &info.gpa);
+    rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
     if ( rc == -EFAULT )
         goto bad_data_abort;
 
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index a07e217..bccdbfc 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -103,11 +103,14 @@ static inline uint64_t gva_to_ma_par(vaddr_t va, unsigned int flags)
     WRITE_CP64(tmp, PAR);
     return par;
 }
-static inline uint64_t gva_to_ipa_par(vaddr_t va)
+static inline uint64_t gva_to_ipa_par(vaddr_t va, unsigned int flags)
 {
     uint64_t par, tmp;
     tmp = READ_CP64(PAR);
-    WRITE_CP32(va, ATS1CPR);
+    if ( (flags & GV2M_WRITE) == GV2M_WRITE )
+        WRITE_CP32(va, ATS1CPW);
+    else
+        WRITE_CP32(va, ATS1CPR);
     isb(); /* Ensure result is available. */
     par = READ_CP64(PAR);
     WRITE_CP64(tmp, PAR);
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index e7a761d..29a32cf 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -98,11 +98,14 @@ static inline uint64_t gva_to_ma_par(vaddr_t va, unsigned int flags)
     return par;
 }
 
-static inline uint64_t gva_to_ipa_par(vaddr_t va)
+static inline uint64_t gva_to_ipa_par(vaddr_t va, unsigned int flags)
 {
     uint64_t par, tmp = READ_SYSREG64(PAR_EL1);
 
-    asm volatile ("at s1e1r, %0;" : : "r" (va));
+    if ( (flags & GV2M_WRITE) == GV2M_WRITE )
+        asm volatile ("at s1e1w, %0;" : : "r" (va));
+    else
+        asm volatile ("at s1e1r, %0;" : : "r" (va));
     isb();
     par = READ_SYSREG64(PAR_EL1);
     WRITE_SYSREG64(tmp, PAR_EL1);
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 8de5e26..3d89494 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -422,9 +422,9 @@ static inline uint64_t va_to_par(vaddr_t va)
     return par;
 }
 
-static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr)
+static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
 {
-    uint64_t par = gva_to_ipa_par(va);
+    uint64_t par = gva_to_ipa_par(va, flags);
     if ( par & PAR_F )
         return -EFAULT;
     *paddr = (par & PADDR_MASK & PAGE_MASK) | ((unsigned long) va & ~PAGE_MASK);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 3/9] xen/arm: Data abort exception (R/W) mem_access events
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
  2015-04-20 15:06 ` [PATCH V15 1/9] xen/arm: groundwork for mem_access support on ARM Tamas K Lengyel
  2015-04-20 15:06 ` [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-21 13:16   ` Ian Campbell
  2015-04-20 15:06 ` [PATCH V15 4/9] xen/arm: Instruction prefetch abort (X) mem_access event handling Tamas K Lengyel
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

This patch enables to store, set, check and deliver LPAE R/W mem_events.
As the LPAE PTE's lack enough available software programmable bits,
we store the permissions in a Radix tree. The tree is only looked at if
mem_access_enabled is turned on.

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
v15: - Style fixes
     - Rebase on staging
     - Make p2m_get_mem_access a locking wrapper around non-locking
        __p2m_get_mem_access
v14: - Make radix_set function easier to read when mem_access not in use.
     - Make dabt trap handler read HPFAR_EL2 when valid
     - Distinguish between read/write access based on dabt.write
     - Combine preemption in apply_p2m_changes
     - Combine shared p2m functions into common/p2m-common.h
v12: - Move the flush in apply_p2m_changes to the label out,
       so we it used in the memaccess preemption case as well.
v11: - Move including common/mem_event.h in here in p2m.h.
     - Flush the tlb in p2m_set_mem_access to cover both the preemption
       and successful finish cases.
v10: - Remove ASSERT from MEMACCESS case.
    - Flush the tlb in the MEMACCESS case as we progress.
    - Typos and style fixes.
v8: - Revert to arch specific p2m_mem_access_check.
    - Retire dabt_dfsc enum and use FSC_FLT defines.
    - Revert to Radix tree approach and use access_in_use flag to
      indicate if the tree is in use or not to avoid uneccessary lookups.
v7: - Removed p2m_shatter_page and p2m_set_permission into separate
      patch.
    - Replace Radix tree settings store with extended struct page_info
      approach. This way the trap handlers can use the MMU directly to
      locate the permission store instead of having to do a tree-lookup.
    - Add p2m_get_entry/set_entry compat functions which are required by
      the common mem_access_check function.
    - Typo fixes.
v6: - Add helper function p2m_shatter_page.
    - Only allocate 4k pages when mem_access is in use.
    - If no setting was found in radix tree but PTE exists,
      return rwx as permission.
    - Move the inclusion of various headers into this patch.
    - Make npfec a const.
v5: - Move p2m_set_entry's logic into apply_one_level via
      a new p2m_op, MEMACCESS.
v4: - Add p2m_mem_access_radix_set function to be called
      when inserting new PTE's and when updating existing entries.
    - Switch p2m_mem_access_check to return bool_t.
    - Use new struct npfec to pass violation info.
v3: - Add new function for updating the PTE entries, p2m_set_entry.
    - Use the new struct npfec to pass violation information.
    - Implement n2rwx, rx2rw and listener required routines.
v2: - Patch been split to ease the review process.
    - Add definitions of data abort data fetch status codes (enum dabt_dfsc)
      and only call p2m_mem_access_check for traps caused by permission violations.
    - Only call p2m_write_pte in p2m_lookup if the PTE permission actually changed.
    - Properly save settings in the Radix tree and pause the VCPU with
      mem_event_vcpu_pause.
---
 xen/arch/arm/p2m.c           | 436 ++++++++++++++++++++++++++++++++++++++++---
 xen/arch/arm/traps.c         |  33 +++-
 xen/include/asm-arm/p2m.h    |  17 +-
 xen/include/asm-x86/p2m.h    |  10 -
 xen/include/xen/p2m-common.h |  14 ++
 5 files changed, 460 insertions(+), 50 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f62d283..8dfee24 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -5,6 +5,9 @@
 #include <xen/errno.h>
 #include <xen/domain_page.h>
 #include <xen/bitops.h>
+#include <xen/vm_event.h>
+#include <xen/mem_access.h>
+#include <public/vm_event.h>
 #include <asm/flushtlb.h>
 #include <asm/gic.h>
 #include <asm/event.h>
@@ -430,12 +433,106 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
     return 0;
 }
 
+static int __p2m_get_mem_access(struct domain *d, unsigned long gpfn,
+                                xenmem_access_t *access)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    void *i;
+    unsigned int index;
+
+    static const xenmem_access_t memaccess[] = {
+#define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac
+            ACCESS(n),
+            ACCESS(r),
+            ACCESS(w),
+            ACCESS(rw),
+            ACCESS(x),
+            ACCESS(rx),
+            ACCESS(wx),
+            ACCESS(rwx),
+            ACCESS(rx2rw),
+            ACCESS(n2rwx),
+#undef ACCESS
+    };
+
+    /* If no setting was ever set, just return rwx. */
+    if ( !p2m->mem_access_enabled )
+    {
+        *access = XENMEM_access_rwx;
+        return 0;
+    }
+
+    /* If request to get default access */
+    if ( gpfn == ~0ul )
+    {
+        *access = memaccess[p2m->default_access];
+        return 0;
+    }
+
+    i = radix_tree_lookup(&p2m->mem_access_settings, gpfn);
+
+    if ( !i )
+    {
+        /*
+         * No setting was found in the Radix tree. Check if the
+         * entry exists in the page-tables.
+         */
+        paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
+        if ( INVALID_PADDR == maddr )
+            return -ESRCH;
+
+        /* If entry exists then its rwx. */
+        *access = XENMEM_access_rwx;
+    }
+    else
+    {
+        /* Setting was found in the Radix tree. */
+        index = radix_tree_ptr_to_int(i);
+        if ( index >= ARRAY_SIZE(memaccess) )
+            return -ERANGE;
+
+        *access = memaccess[index];
+    }
+
+    return 0;
+}
+
+static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
+                                    p2m_access_t a)
+{
+    int rc;
+
+    if ( !p2m->mem_access_enabled )
+        return 0;
+
+    if ( p2m_access_rwx == a )
+    {
+        radix_tree_delete(&p2m->mem_access_settings, pfn);
+        return 0;
+    }
+
+    rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
+                           radix_tree_int_to_ptr(a));
+    if ( rc == -EEXIST )
+    {
+        /* If a setting already exists, change it to the new one */
+        radix_tree_replace_slot(
+            radix_tree_lookup_slot(
+                &p2m->mem_access_settings, pfn),
+            radix_tree_int_to_ptr(a));
+        rc = 0;
+    }
+
+    return rc;
+}
+
 enum p2m_operation {
     INSERT,
     ALLOCATE,
     REMOVE,
     RELINQUISH,
     CACHEFLUSH,
+    MEMACCESS,
 };
 
 /* Put any references on the single 4K page referenced by pte.  TODO:
@@ -569,13 +666,22 @@ static int apply_one_level(struct domain *d,
         if ( p2m_valid(orig_pte) )
             return P2M_ONE_DESCEND;
 
-        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
+        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
+           /* We only create superpages when mem_access is not in use. */
+             (level == 3 || (level < 3 && !p2m->mem_access_enabled)) )
         {
             struct page_info *page;
 
             page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
             if ( page )
             {
+                rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+                if ( rc < 0 )
+                {
+                    free_domheap_page(page);
+                    return rc;
+                }
+
                 pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
                 if ( level < 3 )
                     pte.p2m.table = 0;
@@ -596,8 +702,8 @@ static int apply_one_level(struct domain *d,
         /*
          * If we get here then we failed to allocate a sufficiently
          * large contiguous region for this level (which can't be
-         * L3). Create a page table and continue to descend so we try
-         * smaller allocations.
+         * L3) or mem_access is in use. Create a page table and
+         * continue to descend so we try smaller allocations.
          */
         rc = p2m_create_table(d, entry, 0, flush_cache);
         if ( rc < 0 )
@@ -607,9 +713,16 @@ static int apply_one_level(struct domain *d,
 
     case INSERT:
         if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) &&
-           /* We do not handle replacing an existing table with a superpage */
-             (level == 3 || !p2m_table(orig_pte)) )
+           /*
+            * We do not handle replacing an existing table with a superpage
+            * or when mem_access is in use.
+            */
+             (level == 3 || (!p2m_table(orig_pte) && !p2m->mem_access_enabled)) )
         {
+            rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+            if ( rc < 0 )
+                return rc;
+
             /* New mapping is superpage aligned, make it */
             pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
             if ( level < 3 )
@@ -725,6 +838,7 @@ static int apply_one_level(struct domain *d,
 
         memset(&pte, 0x00, sizeof(pte));
         p2m_write_pte(entry, pte, flush_cache);
+        p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), p2m_access_rwx);
 
         *addr += level_size;
         *maddr += level_size;
@@ -769,6 +883,44 @@ static int apply_one_level(struct domain *d,
             *addr += PAGE_SIZE;
             return P2M_ONE_PROGRESS_NOP;
         }
+
+    case MEMACCESS:
+        if ( level < 3 )
+        {
+            if ( !p2m_valid(orig_pte) )
+            {
+                *addr += level_size;
+                return P2M_ONE_PROGRESS_NOP;
+            }
+
+            /* Shatter large pages as we descend */
+            if ( p2m_mapping(orig_pte) )
+            {
+                rc = p2m_shatter_page(d, entry, level, flush_cache);
+                if ( rc < 0 )
+                    return rc;
+            } /* else: an existing table mapping -> descend */
+
+            return P2M_ONE_DESCEND;
+        }
+        else
+        {
+            pte = orig_pte;
+
+            if ( p2m_valid(pte) )
+            {
+                rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+                if ( rc < 0 )
+                    return rc;
+
+                p2m_set_permission(&pte, pte.p2m.type, a);
+                p2m_write_pte(entry, pte, flush_cache);
+            }
+
+            *addr += level_size;
+            *flush = true;
+            return P2M_ONE_PROGRESS;
+        }
     }
 
     BUG(); /* Should never get here */
@@ -792,6 +944,9 @@ static int apply_p2m_changes(struct domain *d,
     unsigned int cur_root_table = ~0;
     unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
     unsigned int count = 0;
+    const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
+                        egfn = paddr_to_pfn(end_gpaddr);
+    const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
     bool_t flush = false;
     bool_t flush_pt;
 
@@ -819,21 +974,49 @@ static int apply_p2m_changes(struct domain *d,
         };
 
         /*
-         * Arbitrarily, preempt every 512 operations or 8192 nops.
-         * 512*P2M_ONE_PROGRESS == 8192*P2M_ONE_PROGRESS_NOP == 0x2000
-         *
-         * count is initialised to 0 above, so we are guaranteed to
-         * always make at least one pass.
+         * Check if current iteration should be possibly preempted.
+         * Since count is initialised to 0 above we are guaranteed to
+         * always make at least one pass as long as preempt_count_limit is
+         * initialized with a value >= 1.
          */
-
-        if ( op == RELINQUISH && count >= 0x2000 )
+        if ( count >= preempt_count_limit && hypercall_preempt_check() )
         {
-            if ( hypercall_preempt_check() )
+            switch ( op )
             {
+            case RELINQUISH:
+                /*
+                 * Arbitrarily, preempt every 512 operations or 8192 nops.
+                 * 512*P2M_ONE_PROGRESS == 8192*P2M_ONE_PROGRESS_NOP == 0x2000
+                 * This is set in preempt_count_limit.
+                 *
+                 */
                 p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
                 rc = -ERESTART;
                 goto out;
+
+            case MEMACCESS:
+            {
+                /*
+                 * Preempt setting mem_access permissions as required by XSA-89,
+                 * if it's not the last iteration.
+                 */
+                uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
+
+                if ( (egfn - sgfn) > progress && !(progress & mask) )
+                {
+                    rc = progress;
+                    goto tlbflush;
+                }
+                break;
             }
+
+            default:
+                break;
+            };
+
+            /*
+             * Reset current iteration counter.
+             */
             count = 0;
         }
 
@@ -900,27 +1083,23 @@ static int apply_p2m_changes(struct domain *d,
         }
     }
 
-    if ( flush )
-    {
-        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
-        unsigned long egfn = paddr_to_pfn(end_gpaddr);
-
-        flush_tlb_domain(d);
-        iommu_iotlb_flush(d, sgfn, egfn - sgfn);
-    }
-
     if ( op == ALLOCATE || op == INSERT )
     {
-        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
-        unsigned long egfn = paddr_to_pfn(end_gpaddr);
-
         p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn);
         p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn);
     }
 
     rc = 0;
 
+tlbflush:
+    if ( flush )
+    {
+        flush_tlb_domain(d);
+        iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+    }
+
 out:
+
     if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
          addr != start_gpaddr )
     {
@@ -1202,7 +1381,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
      * We do this first as this is faster in the default case when no
      * permission is set on the page.
      */
-    rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
+    rc = __p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
     if ( rc < 0 )
         goto err;
 
@@ -1391,6 +1570,211 @@ void __init setup_virt_paging(void)
     smp_call_function(setup_virt_paging_one, (void *)val, 1);
 }
 
+bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
+{
+    int rc;
+    bool_t violation;
+    xenmem_access_t xma;
+    vm_event_request_t *req;
+    struct vcpu *v = current;
+    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
+
+    /* Mem_access is not in use. */
+    if ( !p2m->mem_access_enabled )
+        return true;
+
+    rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
+    if ( rc )
+        return true;
+
+    /* Now check for mem_access violation. */
+    switch ( xma )
+    {
+    case XENMEM_access_rwx:
+        violation = false;
+        break;
+    case XENMEM_access_rw:
+        violation = npfec.insn_fetch;
+        break;
+    case XENMEM_access_wx:
+        violation = npfec.read_access;
+        break;
+    case XENMEM_access_rx:
+    case XENMEM_access_rx2rw:
+        violation = npfec.write_access;
+        break;
+    case XENMEM_access_x:
+        violation = npfec.read_access || npfec.write_access;
+        break;
+    case XENMEM_access_w:
+        violation = npfec.read_access || npfec.insn_fetch;
+        break;
+    case XENMEM_access_r:
+        violation = npfec.write_access || npfec.insn_fetch;
+        break;
+    default:
+    case XENMEM_access_n:
+    case XENMEM_access_n2rwx:
+        violation = true;
+        break;
+    }
+
+    if ( !violation )
+        return true;
+
+    /* First, handle rx2rw and n2rwx conversion automatically. */
+    if ( npfec.write_access && xma == XENMEM_access_rx2rw )
+    {
+        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+                                0, ~0, XENMEM_access_rw);
+        return false;
+    }
+    else if ( xma == XENMEM_access_n2rwx )
+    {
+        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+                                0, ~0, XENMEM_access_rwx);
+    }
+
+    /* Otherwise, check if there is a vm_event monitor subscriber */
+    if ( !vm_event_check_ring(&v->domain->vm_event->monitor) )
+    {
+        /* No listener */
+        if ( p2m->access_required )
+        {
+            gdprintk(XENLOG_INFO, "Memory access permissions failure, "
+                                  "no vm_event listener VCPU %d, dom %d\n",
+                                  v->vcpu_id, v->domain->domain_id);
+            domain_crash(v->domain);
+        }
+        else
+        {
+            /* n2rwx was already handled */
+            if ( xma != XENMEM_access_n2rwx )
+            {
+                /* A listener is not required, so clear the access
+                 * restrictions. */
+                rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+                                        0, ~0, XENMEM_access_rwx);
+            }
+        }
+
+        /* No need to reinject */
+        return false;
+    }
+
+    req = xzalloc(vm_event_request_t);
+    if ( req )
+    {
+        req->reason = VM_EVENT_REASON_MEM_ACCESS;
+
+        /* Pause the current VCPU */
+        if ( xma != XENMEM_access_n2rwx )
+            req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+
+        /* Send request to mem access subscriber */
+        req->u.mem_access.gfn = gpa >> PAGE_SHIFT;
+        req->u.mem_access.offset =  gpa & ((1 << PAGE_SHIFT) - 1);
+        if ( npfec.gla_valid )
+        {
+            req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
+            req->u.mem_access.gla = gla;
+
+            if ( npfec.kind == npfec_kind_with_gla )
+                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
+            else if ( npfec.kind == npfec_kind_in_gpt )
+                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
+        }
+        req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
+        req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
+        req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
+        req->vcpu_id = v->vcpu_id;
+
+        mem_access_send_req(v->domain, req);
+        xfree(req);
+    }
+
+    /* Pause the current VCPU */
+    if ( xma != XENMEM_access_n2rwx )
+        vm_event_vcpu_pause(v);
+
+    return false;
+}
+
+/*
+ * Set access type for a region of pfns.
+ * If start_pfn == -1ul, sets the default access type.
+ */
+long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
+                        uint32_t start, uint32_t mask, xenmem_access_t access)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    p2m_access_t a;
+    long rc = 0;
+
+    static const p2m_access_t memaccess[] = {
+#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
+        ACCESS(n),
+        ACCESS(r),
+        ACCESS(w),
+        ACCESS(rw),
+        ACCESS(x),
+        ACCESS(rx),
+        ACCESS(wx),
+        ACCESS(rwx),
+        ACCESS(rx2rw),
+        ACCESS(n2rwx),
+#undef ACCESS
+    };
+
+    switch ( access )
+    {
+    case 0 ... ARRAY_SIZE(memaccess) - 1:
+        a = memaccess[access];
+        break;
+    case XENMEM_access_default:
+        a = p2m->default_access;
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    /*
+     * Flip mem_access_enabled to true when a permission is set, as to prevent
+     * allocating or inserting super-pages.
+     */
+    p2m->mem_access_enabled = true;
+
+    /* If request to set default access. */
+    if ( pfn == ~0ul )
+    {
+        p2m->default_access = a;
+        return 0;
+    }
+
+    rc = apply_p2m_changes(d, MEMACCESS,
+                           pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr),
+                           0, MATTR_MEM, mask, 0, a);
+    if ( rc < 0 )
+        return rc;
+    else if ( rc > 0 )
+        return start + rc;
+
+    return 0;
+}
+
+int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
+                       xenmem_access_t *access)
+{
+    int ret;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    spin_lock(&p2m->lock);
+    ret = __p2m_get_mem_access(d, gpfn, access);
+    spin_unlock(&p2m->lock);
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e5d762b..225514b 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2023,11 +2023,36 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     info.gva = READ_SYSREG64(FAR_EL2);
 #endif
 
-    if (dabt.s1ptw)
-        goto bad_data_abort;
+    if ( dabt.s1ptw )
+        info.gpa = READ_SYSREG(HPFAR_EL2);
+    else
+    {
+        rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
+        if ( rc == -EFAULT )
+            goto bad_data_abort;
+    }
+
+    switch ( dabt.dfsc & 0x3f )
+    {
+    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
+    {
+        const struct npfec npfec = {
+            .read_access = !dabt.write,
+            .write_access = dabt.write,
+            .gla_valid = 1,
+            .kind = dabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
+        };
+
+        rc = p2m_mem_access_check(info.gpa, info.gva, npfec);
+
+        /* Trap was triggered by mem_access, work here is done */
+        if ( !rc )
+            return;
+    }
+    break;
+    }
 
-    rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
-    if ( rc == -EFAULT )
+    if ( dabt.s1ptw )
         goto bad_data_abort;
 
     /* XXX: Decode the instruction if ISS is not valid */
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index fec191f..341df55 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -3,6 +3,8 @@
 
 #include <xen/mm.h>
 #include <xen/radix-tree.h>
+#include <public/vm_event.h> /* for vm_event_response_t */
+#include <public/memory.h>
 #include <xen/p2m-common.h>
 #include <public/memory.h>
 
@@ -238,25 +240,20 @@ static inline int get_page_and_type(struct page_info *page,
 /* get host p2m table */
 #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
 
-/* mem_event and mem_access are supported on any ARM guest */
+/* vm_event and mem_access are supported on any ARM guest */
 static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
 {
     return 1;
 }
 
-static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
+static inline bool_t p2m_vm_event_sanity_check(struct domain *d)
 {
     return 1;
 }
 
-/* Get access type for a pfn
- * If pfn == -1ul, gets the default access type */
-static inline
-int p2m_get_mem_access(struct domain *d, unsigned long pfn,
-                       xenmem_access_t *access)
-{
-    return -ENOSYS;
-}
+/* Send mem event based on the access. Boolean return value indicates if trap
+ * needs to be injected into guest. */
+bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec);
 
 #endif /* _XEN_P2M_H */
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2c5eda3..de38f0e 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -597,16 +597,6 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
                             struct npfec npfec,
                             vm_event_request_t **req_ptr);
 
-/* Set access type for a region of pfns.
- * If start_pfn == -1ul, sets the default access type */
-long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, xenmem_access_t access);
-
-/* Get access type for a pfn
- * If pfn == -1ul, gets the default access type */
-int p2m_get_mem_access(struct domain *d, unsigned long pfn,
-                       xenmem_access_t *access);
-
 /*
  * Emulating a memory access requires custom handling. These non-atomic
  * functions should be called under domctl lock.
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 5da8a2d..bd36826 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -44,4 +44,18 @@ int unmap_mmio_regions(struct domain *d,
                        unsigned long nr,
                        unsigned long mfn);
 
+/*
+ * Set access type for a region of pfns.
+ * If start_pfn == -1ul, sets the default access type.
+ */
+long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr,
+                        uint32_t start, uint32_t mask, xenmem_access_t access);
+
+/*
+ * Get access type for a pfn.
+ * If pfn == -1ul, gets the default access type.
+ */
+int p2m_get_mem_access(struct domain *d, unsigned long pfn,
+                       xenmem_access_t *access);
+
 #endif /* _XEN_P2M_COMMON_H */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 4/9] xen/arm: Instruction prefetch abort (X) mem_access event handling
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (2 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 3/9] xen/arm: Data abort exception (R/W) mem_access events Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-21 13:19   ` Ian Campbell
  2015-04-20 15:06 ` [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values Tamas K Lengyel
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

Add missing structure definition for iabt and update the trap handling
mechanism to only inject the exception if the mem_access checker
decides to do so.

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
v15: - Use flush_tlb_local
       Only get gpa if trap was triggered by permission fault
v14: - Query HPFAR_EL2 when valid in iabt handler
     - Flush TLB before doing GVA->IPA translation in iabt handler
v10: - Minor comment fix for describing s1ptw.
v8: - Revert to arch specific p2m_mem_access_check.
    - Retire iabt_fsc enum and use FSC_FLT instead.
    - Complete the struct definition of hsr_iabt.
v7: - Use the new common mem_access_check.
v6: - Make npfec a const.
v4: - Don't mark instruction fetch violation as read violation.
    - Use new struct npfec to pass violation info.
v2: - Add definition for instruction abort instruction fetch status codes
       (enum iabt_ifsc) and only call p2m_mem_access_check for traps triggered
       for permission violations.
---
 xen/arch/arm/traps.c            | 45 +++++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/processor.h | 11 ++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 225514b..d9f9a6f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -40,6 +40,7 @@
 #include <asm/psci.h>
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
+#include <asm/flushtlb.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -1999,8 +2000,48 @@ done:
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
                                       union hsr hsr)
 {
-    register_t addr = READ_SYSREG(FAR_EL2);
-    inject_iabt_exception(regs, addr, hsr.len);
+    int rc;
+    register_t gva = READ_SYSREG(FAR_EL2);
+
+    switch ( hsr.iabt.ifsc & 0x3f )
+    {
+    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
+    {
+        paddr_t gpa;
+        const struct npfec npfec = {
+            .insn_fetch = 1,
+            .gla_valid = 1,
+            .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
+        };
+
+        if ( hsr.iabt.s1ptw )
+            gpa = READ_SYSREG(HPFAR_EL2);
+        else
+        {
+            /*
+             * Flush the TLB to make sure the DTLB is clear before
+             * doing GVA->IPA translation. If we got here because of
+             * an entry only present in the ITLB, this translation may
+             * still be inaccurate.
+             */
+            flush_tlb_local();
+
+            rc = gva_to_ipa(gva, &gpa, GV2M_READ);
+            if ( rc == -EFAULT )
+                goto bad_insn_abort;
+        }
+
+        rc = p2m_mem_access_check(gpa, gva, npfec);
+
+        /* Trap was triggered by mem_access, work here is done */
+        if ( !rc )
+            return;
+    }
+    break;
+    }
+
+bad_insn_abort:
+    inject_iabt_exception(regs, gva, hsr.len);
 }
 
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 905c479..7e6eb66 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -438,6 +438,17 @@ union hsr {
     } sysreg; /* HSR_EC_SYSREG */
 #endif
 
+    struct hsr_iabt {
+        unsigned long ifsc:6;  /* Instruction fault status code */
+        unsigned long res0:1;
+        unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
+        unsigned long res1:1;
+        unsigned long eat:1;   /* External abort type */
+        unsigned long res2:15;
+        unsigned long len:1;   /* Instruction length */
+        unsigned long ec:6;    /* Exception Class */
+    } iabt; /* HSR_EC_INSTR_ABORT_* */
+
     struct hsr_dabt {
         unsigned long dfsc:6;  /* Data Fault Status Code */
         unsigned long write:1; /* Write / not Read */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (3 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 4/9] xen/arm: Instruction prefetch abort (X) mem_access event handling Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-20 15:22   ` Andrew Cooper
  2015-04-20 15:06 ` [PATCH V15 6/9] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

The current implementation of three memops, XENMEM_current_reservation,
XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
int. However, in ARM64 we could potentially have 36-bit pfn's, thus
in preparation for the ARM patch, in this patch we update the existing
memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
as a uin64_t.

This patch also adds error checking on the toolside in case the memop
fails.

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
 tools/libxc/include/xenguest.h          |  2 +-
 tools/libxc/xc_core.h                   |  4 ++--
 tools/libxc/xc_core_arm.c               |  6 +++---
 tools/libxc/xc_core_x86.c               |  8 ++++----
 tools/libxc/xc_domain.c                 | 14 ++++++--------
 tools/libxc/xc_domain_save.c            |  3 ++-
 tools/libxc/xg_private.h                |  2 +-
 tools/tests/mce-test/tools/xen-mceinj.c | 28 ++++++++++++++++++----------
 xen/common/memory.c                     | 16 ++++++++++------
 xen/include/public/memory.h             | 24 ++++++++++++++++++++----
 10 files changed, 67 insertions(+), 40 deletions(-)

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 601b108..5ee2848 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -315,7 +315,7 @@ struct xc_domain_meminfo {
     unsigned int guest_width;
     xen_pfn_t *pfn_type;
     xen_pfn_t *p2m_table;
-    unsigned long p2m_size;
+    xen_pfn_t p2m_size;
 };
 
 int xc_map_domain_meminfo(xc_interface *xch, int domid,
diff --git a/tools/libxc/xc_core.h b/tools/libxc/xc_core.h
index 5867030..99a87e6 100644
--- a/tools/libxc/xc_core.h
+++ b/tools/libxc/xc_core.h
@@ -141,12 +141,12 @@ int xc_core_arch_memory_map_get(xc_interface *xch,
                                 unsigned int *nr_entries);
 int xc_core_arch_map_p2m(xc_interface *xch, unsigned int guest_width,
                          xc_dominfo_t *info, shared_info_any_t *live_shinfo,
-                         xen_pfn_t **live_p2m, unsigned long *pfnp);
+                         xen_pfn_t **live_p2m, xen_pfn_t *pfnp);
 
 int xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width,
                                   xc_dominfo_t *info,
                                   shared_info_any_t *live_shinfo,
-                                  xen_pfn_t **live_p2m, unsigned long *pfnp);
+                                  xen_pfn_t **live_p2m, xen_pfn_t *pfnp);
 
 int xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
                                   xen_pfn_t *gpfn);
diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 57d4715..75ac630 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -66,7 +66,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
 static int
 xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
                         shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
-                        unsigned long *pfnp, int rw)
+                        xen_pfn_t *pfnp, int rw)
 {
     errno = ENOSYS;
     return -1;
@@ -75,7 +75,7 @@ xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc
 int
 xc_core_arch_map_p2m(xc_interface *xch, unsigned int guest_width, xc_dominfo_t *info,
                         shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
-                        unsigned long *pfnp)
+                        xen_pfn_t *pfnp)
 {
     struct domain_info_context _dinfo = { .guest_width = guest_width };
     struct domain_info_context *dinfo = &_dinfo;
@@ -86,7 +86,7 @@ xc_core_arch_map_p2m(xc_interface *xch, unsigned int guest_width, xc_dominfo_t *
 int
 xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_dominfo_t *info,
                               shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
-                              unsigned long *pfnp)
+                              xen_pfn_t *pfnp)
 {
     struct domain_info_context _dinfo = { .guest_width = guest_width };
     struct domain_info_context *dinfo = &_dinfo;
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index 93ebcbb..c2bb059 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -71,7 +71,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
 static int
 xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
                         shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
-                        unsigned long *pfnp, int rw)
+                        xen_pfn_t *pfnp, int rw)
 {
     /* Double and single indirect references to the live P2M table */
     xen_pfn_t *live_p2m_frame_list_list = NULL;
@@ -188,8 +188,8 @@ out:
 
 int
 xc_core_arch_map_p2m(xc_interface *xch, unsigned int guest_width, xc_dominfo_t *info,
-                        shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
-                        unsigned long *pfnp)
+                     shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
+                     xen_pfn_t *pfnp)
 {
     struct domain_info_context _dinfo = { .guest_width = guest_width };
     struct domain_info_context *dinfo = &_dinfo;
@@ -200,7 +200,7 @@ xc_core_arch_map_p2m(xc_interface *xch, unsigned int guest_width, xc_dominfo_t *
 int
 xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_dominfo_t *info,
                               shared_info_any_t *live_shinfo, xen_pfn_t **live_p2m,
-                              unsigned long *pfnp)
+                              xen_pfn_t *pfnp)
 {
     struct domain_info_context _dinfo = { .guest_width = guest_width };
     struct domain_info_context *dinfo = &_dinfo;
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 7cb36d9..2ebbe24 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -796,16 +796,14 @@ int xc_domain_get_tsc_info(xc_interface *xch,
     return rc;
 }
 
-
 int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns)
 {
-    int rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+    struct xen_get_gpfn xgg = { .domid = domid };
+    int rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xgg, sizeof(struct xen_get_gpfn));
+
+    if ( !rc )
+        *gpfns = xgg.gpfn;
 
-    if ( rc >= 0 )
-    {
-        *gpfns = rc;
-        rc = 0;
-    }
     return rc;
 }
 
@@ -813,7 +811,7 @@ int xc_domain_nr_gpfns(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns)
 {
     int rc = xc_domain_maximum_gpfn(xch, domid, gpfns);
 
-    if ( rc >= 0 )
+    if ( !rc )
         *gpfns += 1;
 
     return rc;
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 59323b8..a94c59a 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -811,7 +811,8 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     int live  = (flags & XCFLAGS_LIVE);
     int debug = (flags & XCFLAGS_DEBUG);
     int superpages = !!hvm;
-    int race = 0, sent_last_iter, skip_this_iter = 0;
+    int race = 0, skip_this_iter = 0;
+    xen_pfn_t sent_last_iter = 0;
     unsigned int sent_this_iter = 0;
     int tmem_saved = 0;
 
diff --git a/tools/libxc/xg_private.h b/tools/libxc/xg_private.h
index 1910361..53893fc 100644
--- a/tools/libxc/xg_private.h
+++ b/tools/libxc/xg_private.h
@@ -129,7 +129,7 @@ typedef uint64_t l4_pgentry_64_t;
 
 struct domain_info_context {
     unsigned int guest_width;
-    unsigned long p2m_size;
+    xen_pfn_t p2m_size;
 };
 
 static inline xen_pfn_t xc_pfn_to_mfn(xen_pfn_t pfn, xen_pfn_t *p2m,
diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
index 8ad045f..722cc68 100644
--- a/tools/tests/mce-test/tools/xen-mceinj.c
+++ b/tools/tests/mce-test/tools/xen-mceinj.c
@@ -294,17 +294,21 @@ static uint64_t guest_mfn(xc_interface *xc_handle,
     unsigned long max_mfn = 0; /* max mfn of the whole machine */
     unsigned long m2p_mfn0;
     unsigned int guest_width;
-    long max_gpfn,i;
+    xen_pfn_t max_gpfn;
+    long i;
     uint64_t mfn = MCE_INVALID_MFN;
+    struct xen_get_gpfn xgg = { .domid = domain };
 
     if ( domain > DOMID_FIRST_RESERVED )
         return MCE_INVALID_MFN;
 
     /* Get max gpfn */
-    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain, 
-                               sizeof(domain)) + 1;
-    if ( max_gpfn <= 0 )
-        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
+    rc = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &xgg,
+                               sizeof(struct xen_get_gpfn));
+    if ( rc )
+        err(xc_handle, "Failed to get max_gpfn 0x%lx", rc);
+
+    max_gpfn = xgg.gpfn + 1;
 
     Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
 
@@ -355,16 +359,20 @@ static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
                                 uint64_t gfn)
 {
     uint64_t index;
-    long max_gpfn;
+    xen_pfn_t max_gpfn;
+    struct xen_get_gpfn xgg = { .domid = domain };
 
     /* If domain is xen, means we want pass index directly */
     if ( domain == DOMID_XEN )
         return gfn;
 
-    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain, 
-                               sizeof(domain)) + 1;
-    if ( max_gpfn <= 0 )
-        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
+    rc = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &xgg,
+                               sizeof(struct xen_get_gpfn));
+    if ( rc )
+        err(xc_handle, "Failed to get max_gpfn 0x%lx", rc);
+
+    max_gpfn = xgg.gpfn + 1;
+
     index = gfn % max_gpfn;
 
     return guest_mfn(xc_handle, domain, index);
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 063a1c5..0a79d73 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_current_reservation:
     case XENMEM_maximum_reservation:
     case XENMEM_maximum_gpfn:
+    {
+        struct xen_get_gpfn xgg;
+
         if ( unlikely(start_extent) )
             return -ENOSYS;
 
-        if ( copy_from_guest(&domid, arg, 1) )
+        if ( copy_from_guest(&xgg, arg, 1) )
             return -EFAULT;
 
+        domid = xgg.domid;
         d = rcu_lock_domain_by_any_id(domid);
         if ( d == NULL )
             return -ESRCH;
@@ -858,20 +862,20 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         switch ( op )
         {
         case XENMEM_current_reservation:
-            rc = d->tot_pages;
+            xgg.gpfn = d->tot_pages;
             break;
         case XENMEM_maximum_reservation:
-            rc = d->max_pages;
+            xgg.gpfn = d->max_pages;
             break;
         default:
             ASSERT(op == XENMEM_maximum_gpfn);
-            rc = domain_get_maximum_gpfn(d);
+            xgg.gpfn = domain_get_maximum_gpfn(d);
             break;
         }
 
         rcu_unlock_domain(d);
-
-        break;
+        return __copy_to_guest(arg, &xgg, 1) ? -EFAULT : 0;
+    }
 
     case XENMEM_add_to_physmap:
     {
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 832559a..6567d45 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -146,19 +146,35 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_exchange_t);
 #define XENMEM_maximum_ram_page     2
 
 /*
- * Returns the current or maximum memory reservation, in pages, of the
- * specified domain (may be DOMID_SELF). Returns -ve errcode on failure.
- * arg == addr of domid_t.
+ * Sets gpfn to the current or maximum memory reservation, in pages, of the
+ * specified domain (may be DOMID_SELF) on struct xen_get_gpfn.
+ * Returns -ve errcode on failure.
  */
 #define XENMEM_current_reservation  3
 #define XENMEM_maximum_reservation  4
 
 /*
- * Returns the maximum GPFN in use by the guest, or -ve errcode on failure.
+ * Sets gpfn to the maximum GPFN in use by the guest on struct xen_get_gpfn.
+ * Returns -ve errcode on failure.
  */
 #define XENMEM_maximum_gpfn         14
 
 /*
+ * Struct to hold gpfn return value for calls of
+ *  XENMEM_current_reservation
+ *  XENMEM_maximum_reservation
+ *  XENMEM_maximum_gpfn
+ */
+struct xen_get_gpfn {
+    /* OUT */
+    xen_pfn_t gpfn;
+    /* IN */
+    domid_t domid;
+};
+typedef struct xen_get_gpfn xen_get_gpfn_t;
+DEFINE_XEN_GUEST_HANDLE(xen_get_gpfn_t);
+
+/*
  * Returns a list of MFN bases of 2MB extents comprising the machine_to_phys
  * mapping table. Architectures which do not have a m2p table do not implement
  * this command.
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 6/9] xen/arm: Implement domain_get_maximum_gpfn
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (4 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-21 13:24   ` Ian Campbell
  2015-04-20 15:06 ` [PATCH V15 7/9] tools/libxc: Allocate magic page for mem access on ARM Tamas K Lengyel
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir

From: Julien Grall <julien.grall@linaro.org>

The function domain_get_maximum_gpfn is returning the maximum gpfn ever
mapped in the guest. We can use d->arch.p2m.max_mapped_gfn for this purpose.

We use this in xenaccess as to avoid the user attempting to set page
permissions on pages which don't exist for the domain, as a non-arch specific
sanity check.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 11a0be8..a91ea77 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -996,7 +996,7 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
 
 unsigned long domain_get_maximum_gpfn(struct domain *d)
 {
-    return -ENOSYS;
+    return d->arch.p2m.max_mapped_gfn;
 }
 
 void share_xen_page_with_guest(struct page_info *page,
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 7/9] tools/libxc: Allocate magic page for mem access on ARM
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (5 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 6/9] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-20 15:06 ` [PATCH V15 8/9] tools/tests: Enable xen-access " Tamas K Lengyel
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_arm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index b9fa66d..065debb 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -26,9 +26,10 @@
 #include "xg_private.h"
 #include "xc_dom.h"
 
-#define NR_MAGIC_PAGES 2
+#define NR_MAGIC_PAGES 3
 #define CONSOLE_PFN_OFFSET 0
 #define XENSTORE_PFN_OFFSET 1
+#define MEMACCESS_PFN_OFFSET 2
 
 #define LPAE_SHIFT 9
 
@@ -87,10 +88,13 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
 
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
             dom->console_pfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
             dom->xenstore_pfn);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
+            base + MEMACCESS_PFN_OFFSET);
     /* allocated by toolstack */
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
             dom->console_evtchn);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 8/9] tools/tests: Enable xen-access on ARM
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (6 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 7/9] tools/libxc: Allocate magic page for mem access on ARM Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-21 13:28   ` Ian Campbell
  2015-04-20 15:06 ` [PATCH V15 9/9] xen/arm: Enable mem_access " Tamas K Lengyel
  2015-04-22 13:29 ` [PATCH V15 0/9] Mem_access for ARM Ian Campbell
  9 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

Define the ARM specific test_and_set_bit functions and switch
to use maximum gpfn as the limit to setting permissions. Also,
move HAS_MEM_ACCESS definition into config.

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
v15: - Rebase on staging
v6: - Just use xc_domain_maximum_gpfn to get max_gpfn.
v5: - Use the new information returned by getdomaininfo, max_gpfn, to
      set access permissions. On ARM this will include the potential
      memory hole as well which the hypervisor just loops over.
v4: - Take into account multiple guest ram banks on ARM.
    - Move HAS_MEM_ACCESS definition into config/*.mk and only compile
      xen-access when it is defined.
    - Pass CONFIG_X86/CONFIG_ARM flags during compilation in xen-access
      Makefile.
---
 tools/tests/xen-access/Makefile     |  4 ++--
 tools/tests/xen-access/xen-access.c | 38 +++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/tools/tests/xen-access/Makefile b/tools/tests/xen-access/Makefile
index f0e94fd..f810543 100644
--- a/tools/tests/xen-access/Makefile
+++ b/tools/tests/xen-access/Makefile
@@ -7,8 +7,8 @@ CFLAGS += $(CFLAGS_libxenctrl)
 CFLAGS += $(CFLAGS_libxenguest)
 CFLAGS += $(CFLAGS_xeninclude)
 
-TARGETS-y := 
-TARGETS-$(CONFIG_X86) += xen-access
+TARGETS-y :=
+TARGETS-$(HAS_MEM_ACCESS) := xen-access
 TARGETS := $(TARGETS-y)
 
 .PHONY: all
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 8a899da..12ab921 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -41,6 +41,13 @@
 #include <xenctrl.h>
 #include <xen/vm_event.h>
 
+#if defined(__arm__) || defined(__aarch64__)
+#include <xen/arch-arm.h>
+#define START_PFN (GUEST_RAM0_BASE >> 12)
+#elif defined(__i386__) || defined(__x86_64__)
+#define START_PFN 0ULL
+#endif
+
 #define DPRINTF(a, b...) fprintf(stderr, a, ## b)
 #define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
 #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
@@ -57,7 +64,7 @@ typedef struct vm_event {
 typedef struct xenaccess {
     xc_interface *xc_handle;
 
-    xc_domaininfo_t    *domain_info;
+    xen_pfn_t max_gpfn;
 
     vm_event_t vm_event;
 } xenaccess_t;
@@ -165,7 +172,6 @@ int xenaccess_teardown(xc_interface *xch, xenaccess_t *xenaccess)
     }
     xenaccess->xc_handle = NULL;
 
-    free(xenaccess->domain_info);
     free(xenaccess);
 
     return 0;
@@ -243,23 +249,18 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t domain_id)
                    (vm_event_sring_t *)xenaccess->vm_event.ring_page,
                    XC_PAGE_SIZE);
 
-    /* Get domaininfo */
-    xenaccess->domain_info = malloc(sizeof(xc_domaininfo_t));
-    if ( xenaccess->domain_info == NULL )
-    {
-        ERROR("Error allocating memory for domain info");
-        goto err;
-    }
+    /* Get max_gpfn */
+    rc = xc_domain_maximum_gpfn(xenaccess->xc_handle,
+                                xenaccess->vm_event.domain_id,
+                                &xenaccess->max_gpfn);
 
-    rc = xc_domain_getinfolist(xenaccess->xc_handle, domain_id, 1,
-                               xenaccess->domain_info);
-    if ( rc != 1 )
+    if ( rc )
     {
-        ERROR("Error getting domain info");
+        ERROR("Failed to get max gpfn");
         goto err;
     }
 
-    DPRINTF("max_pages = %"PRIx64"\n", xenaccess->domain_info->max_pages);
+    DPRINTF("max_gpfn = %"PRIx64"\n", xenaccess->max_gpfn);
 
     return xenaccess;
 
@@ -422,8 +423,9 @@ int main(int argc, char *argv[])
         goto exit;
     }
 
-    rc = xc_set_mem_access(xch, domain_id, default_access, 0,
-                           xenaccess->domain_info->max_pages);
+    rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN,
+                           (xenaccess->max_gpfn - START_PFN) );
+
     if ( rc < 0 )
     {
         ERROR("Error %d setting all memory to access type %d\n", rc,
@@ -450,8 +452,8 @@ int main(int argc, char *argv[])
 
             /* Unregister for every event */
             rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0);
-            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, 0,
-                                   xenaccess->domain_info->max_pages);
+            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN,
+                                   (xenaccess->max_gpfn - START_PFN) );
             rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
 
             shutting_down = 1;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH V15 9/9] xen/arm: Enable mem_access on ARM
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (7 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 8/9] tools/tests: Enable xen-access " Tamas K Lengyel
@ 2015-04-20 15:06 ` Tamas K Lengyel
  2015-04-22 13:29 ` [PATCH V15 0/9] Mem_access for ARM Ian Campbell
  9 siblings, 0 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:06 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
	julien.grall, tim, stefano.stabellini, keir, Tamas K Lengyel

Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 config/arm32.mk | 1 +
 config/arm64.mk | 1 +
 2 files changed, 2 insertions(+)

diff --git a/config/arm32.mk b/config/arm32.mk
index 268ca9c..cd97e42 100644
--- a/config/arm32.mk
+++ b/config/arm32.mk
@@ -14,6 +14,7 @@ HAS_EXYNOS4210 := y
 HAS_OMAP := y
 HAS_SCIF := y
 HAS_NS16550 := y
+HAS_MEM_ACCESS := y
 
 # Use only if calling $(LD) directly.
 LDFLAGS_DIRECT += -EL
diff --git a/config/arm64.mk b/config/arm64.mk
index df6ad0a..e24c1d1 100644
--- a/config/arm64.mk
+++ b/config/arm64.mk
@@ -9,6 +9,7 @@ CFLAGS += #-marm -march= -mcpu= etc
 HAS_PL011 := y
 HAS_CADENCE_UART := y
 HAS_NS16550 := y
+HAS_MEM_ACCESS := y
 
 # Use only if calling $(LD) directly.
 LDFLAGS_DIRECT += -EL
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-20 15:06 ` [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values Tamas K Lengyel
@ 2015-04-20 15:22   ` Andrew Cooper
  2015-04-20 15:25     ` Tamas K Lengyel
  2015-04-21 13:23     ` Ian Campbell
  0 siblings, 2 replies; 34+ messages in thread
From: Andrew Cooper @ 2015-04-20 15:22 UTC (permalink / raw)
  To: Tamas K Lengyel, xen-devel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, tim, julien.grall,
	ian.jackson, stefano.stabellini, keir

On 20/04/15 16:06, Tamas K Lengyel wrote:
> The current implementation of three memops, XENMEM_current_reservation,
> XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
> int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> in preparation for the ARM patch, in this patch we update the existing
> memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
> as a uin64_t.
>
> This patch also adds error checking on the toolside in case the memop
> fails.
>
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>

XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.

You cannot make adjustments like this, but you can add a brand new op
with appropriate parameters and list the old ops as deprecated.

~Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-20 15:22   ` Andrew Cooper
@ 2015-04-20 15:25     ` Tamas K Lengyel
  2015-04-21 13:23     ` Ian Campbell
  1 sibling, 0 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-20 15:25 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
	Julien Grall, Ian Jackson, Xen-devel, Stefano Stabellini,
	Keir Fraser


[-- Attachment #1.1: Type: text/plain, Size: 920 bytes --]

On Mon, Apr 20, 2015 at 5:22 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 20/04/15 16:06, Tamas K Lengyel wrote:
> > The current implementation of three memops, XENMEM_current_reservation,
> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> > in preparation for the ARM patch, in this patch we update the existing
> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
> > as a uin64_t.
> >
> > This patch also adds error checking on the toolside in case the memop
> > fails.
> >
> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>
> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
>
> You cannot make adjustments like this, but you can add a brand new op
> with appropriate parameters and list the old ops as deprecated.
>
> ~Andrew
>

Ack, thanks.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1521 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages
  2015-04-20 15:06 ` [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
@ 2015-04-21 13:14   ` Ian Campbell
  2015-04-21 14:10     ` Tamas K Lengyel
  0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 13:14 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
	xen-devel, stefano.stabellini, keir

On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
> -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> +static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)

I'd have been inclined to make this take a p2m_domain* rather than a
domain*, on the basis that the caller must have one in hand to have
locked it. I don't think __p2m_lookup uses d other than to find the p2m.

Not worth changing unless there is some other reason to resend though,
so with or without that:

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

Ian.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 3/9] xen/arm: Data abort exception (R/W) mem_access events
  2015-04-20 15:06 ` [PATCH V15 3/9] xen/arm: Data abort exception (R/W) mem_access events Tamas K Lengyel
@ 2015-04-21 13:16   ` Ian Campbell
  0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 13:16 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
	xen-devel, stefano.stabellini, keir

On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
> This patch enables to store, set, check and deliver LPAE R/W mem_events.
> As the LPAE PTE's lack enough available software programmable bits,
> we store the permissions in a Radix tree. The tree is only looked at if
> mem_access_enabled is turned on.
> 
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 4/9] xen/arm: Instruction prefetch abort (X) mem_access event handling
  2015-04-20 15:06 ` [PATCH V15 4/9] xen/arm: Instruction prefetch abort (X) mem_access event handling Tamas K Lengyel
@ 2015-04-21 13:19   ` Ian Campbell
  0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 13:19 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
	xen-devel, stefano.stabellini, keir

On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
> Add missing structure definition for iabt and update the trap handling
> mechanism to only inject the exception if the mem_access checker
> decides to do so.
> 
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>

I'm not super thrilled about the TLB flush here, but it does seem to be
unavoidable, at least with our combined level of cunning right at the
moment, so:

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

Ian.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-20 15:22   ` Andrew Cooper
  2015-04-20 15:25     ` Tamas K Lengyel
@ 2015-04-21 13:23     ` Ian Campbell
  2015-04-21 14:14       ` Jan Beulich
  1 sibling, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 13:23 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: wei.liu2, stefano.stabellini, tim, julien.grall, ian.jackson,
	xen-devel, stefano.stabellini, keir, Tamas K Lengyel

On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
> On 20/04/15 16:06, Tamas K Lengyel wrote:
> > The current implementation of three memops, XENMEM_current_reservation,
> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> > in preparation for the ARM patch, in this patch we update the existing
> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
> > as a uin64_t.
> >
> > This patch also adds error checking on the toolside in case the memop
> > fails.
> >
> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> 
> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
> 
> You cannot make adjustments like this, but you can add a brand new op
> with appropriate parameters and list the old ops as deprecated.

Right. For the benefit of callers using the old API it seems what we
usually do is rename the old op XENMEM_foo_compat and use the name with
a new number for the new functionality, then use a
__XEN_INTERFACE_VERSION__ to #define back to the old name.

The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
reasonable example, I couldn't find one specifically for the memory ops.

Ian.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 6/9] xen/arm: Implement domain_get_maximum_gpfn
  2015-04-20 15:06 ` [PATCH V15 6/9] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
@ 2015-04-21 13:24   ` Ian Campbell
  0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 13:24 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
	xen-devel, stefano.stabellini, keir

On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
> From: Julien Grall <julien.grall@linaro.org>
> 
> The function domain_get_maximum_gpfn is returning the maximum gpfn ever
> mapped in the guest. We can use d->arch.p2m.max_mapped_gfn for this purpose.
> 
> We use this in xenaccess as to avoid the user attempting to set page
> permissions on pages which don't exist for the domain, as a non-arch specific
> sanity check.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 8/9] tools/tests: Enable xen-access on ARM
  2015-04-20 15:06 ` [PATCH V15 8/9] tools/tests: Enable xen-access " Tamas K Lengyel
@ 2015-04-21 13:28   ` Ian Campbell
  2015-04-21 14:13     ` Tamas K Lengyel
  0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 13:28 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
	xen-devel, stefano.stabellini, keir

On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:

Did this patch go away for a bit? It wasn't in v14 and the last on I
seem to have was v11? (I don't mind, just surprised not to see a bunch
of versions)

> Define the ARM specific test_and_set_bit functions

Not any more.

>  and switch
> to use maximum gpfn as the limit to setting permissions. Also,
> move HAS_MEM_ACCESS definition into config.
> 
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>

With an updated commit log:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages
  2015-04-21 13:14   ` Ian Campbell
@ 2015-04-21 14:10     ` Tamas K Lengyel
  2015-04-21 14:22       ` Ian Campbell
  0 siblings, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-21 14:10 UTC (permalink / raw)
  To: Ian Campbell
  Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
	Tim Deegan, Xen-devel, Stefano Stabellini, Keir Fraser


[-- Attachment #1.1: Type: text/plain, Size: 796 bytes --]

On Tue, Apr 21, 2015 at 3:14 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:

> On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
> > -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> > +static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t
> *t)
>
> I'd have been inclined to make this take a p2m_domain* rather than a
> domain*, on the basis that the caller must have one in hand to have
> locked it. I don't think __p2m_lookup uses d other than to find the p2m.
>
> Not worth changing unless there is some other reason to resend though,
> so with or without that:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Ian.
>

Thanks, I made the change as I'll have another round anyway to introduce
XENMEM_maximum_gpfn2 in the series.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1334 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 8/9] tools/tests: Enable xen-access on ARM
  2015-04-21 13:28   ` Ian Campbell
@ 2015-04-21 14:13     ` Tamas K Lengyel
  0 siblings, 0 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-21 14:13 UTC (permalink / raw)
  To: Ian Campbell
  Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
	Tim Deegan, Xen-devel, Stefano Stabellini, Keir Fraser


[-- Attachment #1.1: Type: text/plain, Size: 885 bytes --]

On Tue, Apr 21, 2015 at 3:28 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:

> On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
>
> Did this patch go away for a bit? It wasn't in v14 and the last on I
> seem to have was v11? (I don't mind, just surprised not to see a bunch
> of versions)
>

Yes, this one patch I didn't send in v15 as I was waiting for the cleanup
series to be merged. This one had a bunch of conflicts with it so it didn't
make much sense to send it at the time.


>
> > Define the ARM specific test_and_set_bit functions
>
> Not any more.
>

Right.. =)


>
> >  and switch
> > to use maximum gpfn as the limit to setting permissions. Also,
> > move HAS_MEM_ACCESS definition into config.
> >
> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>
> With an updated commit log:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>

Thanks!
Tamas

[-- Attachment #1.2: Type: text/html, Size: 1772 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 13:23     ` Ian Campbell
@ 2015-04-21 14:14       ` Jan Beulich
  2015-04-21 14:24         ` Ian Campbell
  2015-04-21 14:33         ` Tamas K Lengyel
  0 siblings, 2 replies; 34+ messages in thread
From: Jan Beulich @ 2015-04-21 14:14 UTC (permalink / raw)
  To: Ian Campbell, Tamas K Lengyel
  Cc: tim, wei.liu2, stefano.stabellini, Andrew Cooper, julien.grall,
	ian.jackson, xen-devel, stefano.stabellini, keir

>>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
>> On 20/04/15 16:06, Tamas K Lengyel wrote:
>> > The current implementation of three memops, XENMEM_current_reservation,
>> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
>> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
>> > in preparation for the ARM patch, in this patch we update the existing
>> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
>> > as a uin64_t.
>> >
>> > This patch also adds error checking on the toolside in case the memop
>> > fails.
>> >
>> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>> 
>> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
>> 
>> You cannot make adjustments like this, but you can add a brand new op
>> with appropriate parameters and list the old ops as deprecated.
> 
> Right. For the benefit of callers using the old API it seems what we
> usually do is rename the old op XENMEM_foo_compat and use the name with
> a new number for the new functionality, then use a
> __XEN_INTERFACE_VERSION__ to #define back to the old name.
> 
> The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
> reasonable example, I couldn't find one specifically for the memory ops.

And there's no need to afaict: This complication isn't needed in the
first place. The patch's context already makes this clear:

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)

Note the "long" return type. Yet the patch description, for
whatever reason, claims the hypercall to only return an "int".
Maybe because (as pointed out before) the respective Linux
hypercall stub wrongly an "int" return type?

(Tamas, while I had asked you to avoid Cc-ing me on ARM only
patches, I would have been able to spot this earlier if you had
Cc-ed me on this non-ARM change. Again, please Cc
maintainers, but especially on larger series make sure you
don't Cc everyone on every patch. I'm sure avoiding the extra
mail load is being appreciated not just by me.)

Jan

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages
  2015-04-21 14:10     ` Tamas K Lengyel
@ 2015-04-21 14:22       ` Ian Campbell
  0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 14:22 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
	Tim Deegan, Xen-devel, Stefano Stabellini, Keir Fraser

On Tue, 2015-04-21 at 16:10 +0200, Tamas K Lengyel wrote:
> 
> 
> On Tue, Apr 21, 2015 at 3:14 PM, Ian Campbell
> <ian.campbell@citrix.com> wrote:
>         On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:
>         > -paddr_t p2m_lookup(struct domain *d, paddr_t paddr,
>         p2m_type_t *t)
>         > +static paddr_t __p2m_lookup(struct domain *d, paddr_t
>         paddr, p2m_type_t *t)
>         
>         I'd have been inclined to make this take a p2m_domain* rather
>         than a
>         domain*, on the basis that the caller must have one in hand to
>         have
>         locked it. I don't think __p2m_lookup uses d other than to
>         find the p2m.
>         
>         Not worth changing unless there is some other reason to resend
>         though,
>         so with or without that:
>         
>         Acked-by: Ian Campbell <ian.campbell@citrix.com>
>         
>         Ian.
> 
> 
> Thanks, I made the change as I'll have another round anyway to
> introduce XENMEM_maximum_gpfn2 in the series.

Great, thanks.

FWIW there was another similar wrapper in another patch later on, but
that one did use d so I didn't suggest the same change there for that
reason.

Ian.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:14       ` Jan Beulich
@ 2015-04-21 14:24         ` Ian Campbell
  2015-04-21 14:29           ` Jan Beulich
  2015-04-21 14:33         ` Tamas K Lengyel
  1 sibling, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 14:24 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tim, wei.liu2, stefano.stabellini, Andrew Cooper, julien.grall,
	ian.jackson, xen-devel, stefano.stabellini, keir,
	Tamas K Lengyel

On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
> >> > The current implementation of three memops, XENMEM_current_reservation,
> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> >> > in preparation for the ARM patch, in this patch we update the existing
> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
> >> > as a uin64_t.
> >> >
> >> > This patch also adds error checking on the toolside in case the memop
> >> > fails.
> >> >
> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> >> 
> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
> >> 
> >> You cannot make adjustments like this, but you can add a brand new op
> >> with appropriate parameters and list the old ops as deprecated.
> > 
> > Right. For the benefit of callers using the old API it seems what we
> > usually do is rename the old op XENMEM_foo_compat and use the name with
> > a new number for the new functionality, then use a
> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
> > 
> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
> > reasonable example, I couldn't find one specifically for the memory ops.
> 
> And there's no need to afaict: This complication isn't needed in the
> first place. The patch's context already makes this clear:
> 
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> 
> Note the "long" return type. Yet the patch description, for
> whatever reason, claims the hypercall to only return an "int".
> Maybe because (as pointed out before) the respective Linux
> hypercall stub wrongly an "int" return type?

Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a 64
bit host (maximum pfn more than 2^32)?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:24         ` Ian Campbell
@ 2015-04-21 14:29           ` Jan Beulich
  2015-04-21 14:42             ` Tamas K Lengyel
  2015-04-21 14:47             ` Ian Campbell
  0 siblings, 2 replies; 34+ messages in thread
From: Jan Beulich @ 2015-04-21 14:29 UTC (permalink / raw)
  To: Ian Campbell
  Cc: tim, wei.liu2, stefano.stabellini, Andrew Cooper, julien.grall,
	ian.jackson, xen-devel, stefano.stabellini, keir,
	Tamas K Lengyel

>>> On 21.04.15 at 16:24, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
>> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
>> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
>> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
>> >> > The current implementation of three memops, XENMEM_current_reservation,
>> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
>> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
>> >> > in preparation for the ARM patch, in this patch we update the existing
>> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
>> >> > as a uin64_t.
>> >> >
>> >> > This patch also adds error checking on the toolside in case the memop
>> >> > fails.
>> >> >
>> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>> >> 
>> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
>> >> 
>> >> You cannot make adjustments like this, but you can add a brand new op
>> >> with appropriate parameters and list the old ops as deprecated.
>> > 
>> > Right. For the benefit of callers using the old API it seems what we
>> > usually do is rename the old op XENMEM_foo_compat and use the name with
>> > a new number for the new functionality, then use a
>> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
>> > 
>> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
>> > reasonable example, I couldn't find one specifically for the memory ops.
>> 
>> And there's no need to afaict: This complication isn't needed in the
>> first place. The patch's context already makes this clear:
>> 
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>> 
>> Note the "long" return type. Yet the patch description, for
>> whatever reason, claims the hypercall to only return an "int".
>> Maybe because (as pointed out before) the respective Linux
>> hypercall stub wrongly an "int" return type?
> 
> Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a 64
> bit host (maximum pfn more than 2^32)?

It is, but do we really want to introduce other than just compat
mode helper interfaces (i.e. leaving the current ones alone, and
perhaps even making the new ones tools only) if we really care
about such setups in the first place?

Jan

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:14       ` Jan Beulich
  2015-04-21 14:24         ` Ian Campbell
@ 2015-04-21 14:33         ` Tamas K Lengyel
  2015-04-21 14:41           ` Jan Beulich
  1 sibling, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-21 14:33 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tim Deegan, wei.liu2, Ian Campbell, Stefano Stabellini,
	Andrew Cooper, Julien Grall, Ian Jackson, Xen-devel,
	Stefano Stabellini, Keir Fraser


[-- Attachment #1.1: Type: text/plain, Size: 2768 bytes --]

On Tue, Apr 21, 2015 at 4:14 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
> >> > The current implementation of three memops,
> XENMEM_current_reservation,
> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> >> > in preparation for the ARM patch, in this patch we update the existing
> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn
> info
> >> > as a uin64_t.
> >> >
> >> > This patch also adds error checking on the toolside in case the memop
> >> > fails.
> >> >
> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> >>
> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
> >>
> >> You cannot make adjustments like this, but you can add a brand new op
> >> with appropriate parameters and list the old ops as deprecated.
> >
> > Right. For the benefit of callers using the old API it seems what we
> > usually do is rename the old op XENMEM_foo_compat and use the name with
> > a new number for the new functionality, then use a
> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
> >
> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
> > reasonable example, I couldn't find one specifically for the memory ops.
>
> And there's no need to afaict: This complication isn't needed in the
> first place. The patch's context already makes this clear:
>
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>
> Note the "long" return type. Yet the patch description, for
> whatever reason, claims the hypercall to only return an "int".
> Maybe because (as pointed out before) the respective Linux
> hypercall stub wrongly an "int" return type?
>

The privcmd driver on Linux certainly does return an int via the ioctl.


>
> (Tamas, while I had asked you to avoid Cc-ing me on ARM only
> patches, I would have been able to spot this earlier if you had
> Cc-ed me on this non-ARM change. Again, please Cc
> maintainers, but especially on larger series make sure you
> don't Cc everyone on every patch. I'm sure avoiding the extra
> mail load is being appreciated not just by me.)
>
> Jan
>

Ack, I took you off only after you repeatedly asked me to exclude you from
this series. I agree that patches should be only cc-d to the respective
maintainers and not all maintainers who are involved in the series. I'll
look into how that could be automated because doing that by hand is not
very pleasant.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 3918 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:33         ` Tamas K Lengyel
@ 2015-04-21 14:41           ` Jan Beulich
  0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2015-04-21 14:41 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Tim Deegan, wei.liu2, Ian Campbell, Stefano Stabellini,
	Andrew Cooper, Julien Grall, Ian Jackson, Xen-devel,
	Stefano Stabellini, Keir Fraser

>>> On 21.04.15 at 16:33, <tklengyel@sec.in.tum.de> wrote:
> On Tue, Apr 21, 2015 at 4:14 PM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
>> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
>> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
>> >> > The current implementation of three memops,
>> XENMEM_current_reservation,
>> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
>> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
>> >> > in preparation for the ARM patch, in this patch we update the existing
>> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn
>> info
>> >> > as a uin64_t.
>> >> >
>> >> > This patch also adds error checking on the toolside in case the memop
>> >> > fails.
>> >> >
>> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>> >>
>> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
>> >>
>> >> You cannot make adjustments like this, but you can add a brand new op
>> >> with appropriate parameters and list the old ops as deprecated.
>> >
>> > Right. For the benefit of callers using the old API it seems what we
>> > usually do is rename the old op XENMEM_foo_compat and use the name with
>> > a new number for the new functionality, then use a
>> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
>> >
>> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
>> > reasonable example, I couldn't find one specifically for the memory ops.
>>
>> And there's no need to afaict: This complication isn't needed in the
>> first place. The patch's context already makes this clear:
>>
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>
>> Note the "long" return type. Yet the patch description, for
>> whatever reason, claims the hypercall to only return an "int".
>> Maybe because (as pointed out before) the respective Linux
>> hypercall stub wrongly an "int" return type?
> 
> The privcmd driver on Linux certainly does return an int via the ioctl.

That's clearly a bug in Linux:

static long privcmd_ioctl(struct file *file,
			  unsigned int cmd, unsigned long data)
{
	int ret = -ENOSYS;
	void __user *udata = (void __user *) data;

	switch (cmd) {
	case IOCTL_PRIVCMD_HYPERCALL:
		ret = privcmd_ioctl_hypercall(udata);
		break;
[...]
	return ret;
}

Why in the world is ret an int? That's certainly not something
inherited from XenoLinux, where all involved types are long.

Jan

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:29           ` Jan Beulich
@ 2015-04-21 14:42             ` Tamas K Lengyel
  2015-04-21 15:13               ` Jan Beulich
  2015-04-21 14:47             ` Ian Campbell
  1 sibling, 1 reply; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-21 14:42 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tim Deegan, wei.liu2, Ian Campbell, Stefano Stabellini,
	Andrew Cooper, Julien Grall, Ian Jackson, Xen-devel,
	Stefano Stabellini, Keir Fraser


[-- Attachment #1.1: Type: text/plain, Size: 2827 bytes --]

On Tue, Apr 21, 2015 at 4:29 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 21.04.15 at 16:24, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
> >> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
> >> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
> >> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
> >> >> > The current implementation of three memops,
> XENMEM_current_reservation,
> >> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values
> as an
> >> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> >> >> > in preparation for the ARM patch, in this patch we update the
> existing
> >> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn
> info
> >> >> > as a uin64_t.
> >> >> >
> >> >> > This patch also adds error checking on the toolside in case the
> memop
> >> >> > fails.
> >> >> >
> >> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> >> >>
> >> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
> >> >>
> >> >> You cannot make adjustments like this, but you can add a brand new op
> >> >> with appropriate parameters and list the old ops as deprecated.
> >> >
> >> > Right. For the benefit of callers using the old API it seems what we
> >> > usually do is rename the old op XENMEM_foo_compat and use the name
> with
> >> > a new number for the new functionality, then use a
> >> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
> >> >
> >> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
> >> > reasonable example, I couldn't find one specifically for the memory
> ops.
> >>
> >> And there's no need to afaict: This complication isn't needed in the
> >> first place. The patch's context already makes this clear:
> >>
> >> --- a/xen/common/memory.c
> >> +++ b/xen/common/memory.c
> >> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >>
> >> Note the "long" return type. Yet the patch description, for
> >> whatever reason, claims the hypercall to only return an "int".
> >> Maybe because (as pointed out before) the respective Linux
> >> hypercall stub wrongly an "int" return type?
> >
> > Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a 64
> > bit host (maximum pfn more than 2^32)?
>
> It is, but do we really want to introduce other than just compat
> mode helper interfaces (i.e. leaving the current ones alone, and
> perhaps even making the new ones tools only) if we really care
> about such setups in the first place?
>
> Jan
>

At the moment I just followed Andrew's advice and will introduce a new
XENMEM_maximum_gpfn2 memop that returns the gpfn in a struct as xen_pfn_t.
The old memops I'll leave untouched if that's OK.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4064 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:29           ` Jan Beulich
  2015-04-21 14:42             ` Tamas K Lengyel
@ 2015-04-21 14:47             ` Ian Campbell
  1 sibling, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-04-21 14:47 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tim, wei.liu2, stefano.stabellini, Andrew Cooper, julien.grall,
	ian.jackson, xen-devel, stefano.stabellini, keir,
	Tamas K Lengyel

On Tue, 2015-04-21 at 15:29 +0100, Jan Beulich wrote:
> >>> On 21.04.15 at 16:24, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
> >> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
> >> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
> >> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
> >> >> > The current implementation of three memops, XENMEM_current_reservation,
> >> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
> >> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
> >> >> > in preparation for the ARM patch, in this patch we update the existing
> >> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn info
> >> >> > as a uin64_t.
> >> >> >
> >> >> > This patch also adds error checking on the toolside in case the memop
> >> >> > fails.
> >> >> >
> >> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> >> >> 
> >> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
> >> >> 
> >> >> You cannot make adjustments like this, but you can add a brand new op
> >> >> with appropriate parameters and list the old ops as deprecated.
> >> > 
> >> > Right. For the benefit of callers using the old API it seems what we
> >> > usually do is rename the old op XENMEM_foo_compat and use the name with
> >> > a new number for the new functionality, then use a
> >> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
> >> > 
> >> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
> >> > reasonable example, I couldn't find one specifically for the memory ops.
> >> 
> >> And there's no need to afaict: This complication isn't needed in the
> >> first place. The patch's context already makes this clear:
> >> 
> >> --- a/xen/common/memory.c
> >> +++ b/xen/common/memory.c
> >> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >> 
> >> Note the "long" return type. Yet the patch description, for
> >> whatever reason, claims the hypercall to only return an "int".
> >> Maybe because (as pointed out before) the respective Linux
> >> hypercall stub wrongly an "int" return type?
> > 
> > Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a 64
> > bit host (maximum pfn more than 2^32)?
> 
> It is, but do we really want to introduce other than just compat
> mode helper interfaces (i.e. leaving the current ones alone, and
> perhaps even making the new ones tools only) if we really care
> about such setups in the first place?

IIRC the original impetus for at least one part of this interface was
for in guest use. I don't recall what the use was, I think it was the
max pfn one which was used though.

Perhaps in that case we can assume that a signed long is sufficient for
any pfn they might see. But is that true? A 32-bit guest could see
PFN>2^31 I think.

Perhaps we should add these fixed version as a tools interface and
consider only doing a fixed guest visible version of the max pfn one?
Modulo confirming that I'm not misremembering...

Ian.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 14:42             ` Tamas K Lengyel
@ 2015-04-21 15:13               ` Jan Beulich
  2015-04-21 15:21                 ` Tamas K Lengyel
  2015-04-25 16:54                 ` Julien Grall
  0 siblings, 2 replies; 34+ messages in thread
From: Jan Beulich @ 2015-04-21 15:13 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Tim Deegan, wei.liu2, Ian Campbell, Stefano Stabellini,
	Andrew Cooper, Julien Grall, Ian Jackson, Xen-devel,
	Stefano Stabellini, Keir Fraser

>>> On 21.04.15 at 16:42, <tklengyel@sec.in.tum.de> wrote:
> On Tue, Apr 21, 2015 at 4:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 21.04.15 at 16:24, <ian.campbell@citrix.com> wrote:
>> > On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
>> >> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
>> >> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
>> >> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
>> >> >> > The current implementation of three memops,
>> XENMEM_current_reservation,
>> >> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values
>> as an
>> >> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
>> >> >> > in preparation for the ARM patch, in this patch we update the
>> existing
>> >> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn
>> info
>> >> >> > as a uin64_t.
>> >> >> >
>> >> >> > This patch also adds error checking on the toolside in case the
>> memop
>> >> >> > fails.
>> >> >> >
>> >> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>> >> >>
>> >> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
>> >> >>
>> >> >> You cannot make adjustments like this, but you can add a brand new op
>> >> >> with appropriate parameters and list the old ops as deprecated.
>> >> >
>> >> > Right. For the benefit of callers using the old API it seems what we
>> >> > usually do is rename the old op XENMEM_foo_compat and use the name
>> with
>> >> > a new number for the new functionality, then use a
>> >> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
>> >> >
>> >> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
>> >> > reasonable example, I couldn't find one specifically for the memory
>> ops.
>> >>
>> >> And there's no need to afaict: This complication isn't needed in the
>> >> first place. The patch's context already makes this clear:
>> >>
>> >> --- a/xen/common/memory.c
>> >> +++ b/xen/common/memory.c
>> >> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd,
>> > XEN_GUEST_HANDLE_PARAM(void) arg)
>> >>
>> >> Note the "long" return type. Yet the patch description, for
>> >> whatever reason, claims the hypercall to only return an "int".
>> >> Maybe because (as pointed out before) the respective Linux
>> >> hypercall stub wrongly an "int" return type?
>> >
>> > Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a 64
>> > bit host (maximum pfn more than 2^32)?
>>
>> It is, but do we really want to introduce other than just compat
>> mode helper interfaces (i.e. leaving the current ones alone, and
>> perhaps even making the new ones tools only) if we really care
>> about such setups in the first place?
> 
> At the moment I just followed Andrew's advice and will introduce a new
> XENMEM_maximum_gpfn2 memop that returns the gpfn in a struct as xen_pfn_t.
> The old memops I'll leave untouched if that's OK.

For this specific one - is there a reasonable use case? Other than
for host PFN, we have control over guest ones, and I'm not sure
managing a guest with GPFNs extending past 4 billion can be
expected to work if only this one hypercall got fixed. IOW I'm
expecting to NAK any such addition without proper rationale.

Jan

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 15:13               ` Jan Beulich
@ 2015-04-21 15:21                 ` Tamas K Lengyel
  2015-04-25 16:54                 ` Julien Grall
  1 sibling, 0 replies; 34+ messages in thread
From: Tamas K Lengyel @ 2015-04-21 15:21 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tim Deegan, wei.liu2, Ian Campbell, Stefano Stabellini,
	Andrew Cooper, Julien Grall, Ian Jackson, Xen-devel,
	Stefano Stabellini, Keir Fraser


[-- Attachment #1.1: Type: text/plain, Size: 3692 bytes --]

On Tue, Apr 21, 2015 at 5:13 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 21.04.15 at 16:42, <tklengyel@sec.in.tum.de> wrote:
> > On Tue, Apr 21, 2015 at 4:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> >> >>> On 21.04.15 at 16:24, <ian.campbell@citrix.com> wrote:
> >> > On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
> >> >> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> wrote:
> >> >> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
> >> >> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
> >> >> >> > The current implementation of three memops,
> >> XENMEM_current_reservation,
> >> >> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values
> >> as an
> >> >> >> > int. However, in ARM64 we could potentially have 36-bit pfn's,
> thus
> >> >> >> > in preparation for the ARM patch, in this patch we update the
> >> existing
> >> >> >> > memop routines to use a struct, xen_get_gpfn, to exchange the
> gpfn
> >> info
> >> >> >> > as a uin64_t.
> >> >> >> >
> >> >> >> > This patch also adds error checking on the toolside in case the
> >> memop
> >> >> >> > fails.
> >> >> >> >
> >> >> >> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> >> >> >>
> >> >> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable
> ABI/API.
> >> >> >>
> >> >> >> You cannot make adjustments like this, but you can add a brand
> new op
> >> >> >> with appropriate parameters and list the old ops as deprecated.
> >> >> >
> >> >> > Right. For the benefit of callers using the old API it seems what
> we
> >> >> > usually do is rename the old op XENMEM_foo_compat and use the name
> >> with
> >> >> > a new number for the new functionality, then use a
> >> >> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
> >> >> >
> >> >> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
> >> >> > reasonable example, I couldn't find one specifically for the memory
> >> ops.
> >> >>
> >> >> And there's no need to afaict: This complication isn't needed in the
> >> >> first place. The patch's context already makes this clear:
> >> >>
> >> >> --- a/xen/common/memory.c
> >> >> +++ b/xen/common/memory.c
> >> >> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd,
> >> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >> >>
> >> >> Note the "long" return type. Yet the patch description, for
> >> >> whatever reason, claims the hypercall to only return an "int".
> >> >> Maybe because (as pointed out before) the respective Linux
> >> >> hypercall stub wrongly an "int" return type?
> >> >
> >> > Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a
> 64
> >> > bit host (maximum pfn more than 2^32)?
> >>
> >> It is, but do we really want to introduce other than just compat
> >> mode helper interfaces (i.e. leaving the current ones alone, and
> >> perhaps even making the new ones tools only) if we really care
> >> about such setups in the first place?
> >
> > At the moment I just followed Andrew's advice and will introduce a new
> > XENMEM_maximum_gpfn2 memop that returns the gpfn in a struct as
> xen_pfn_t.
> > The old memops I'll leave untouched if that's OK.
>
> For this specific one - is there a reasonable use case? Other than
> for host PFN, we have control over guest ones, and I'm not sure
> managing a guest with GPFNs extending past 4 billion can be
> expected to work if only this one hypercall got fixed. IOW I'm
> expecting to NAK any such addition without proper rationale.
>
> Jan
>

AFAIK everything works for me as it is already without this patch. I'm not
sure if the cornercase of pfn's > 32-bit wide on ARM64 is actually
something that would work/is supported at the moment.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 5494 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 0/9] Mem_access for ARM
  2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
                   ` (8 preceding siblings ...)
  2015-04-20 15:06 ` [PATCH V15 9/9] xen/arm: Enable mem_access " Tamas K Lengyel
@ 2015-04-22 13:29 ` Ian Campbell
  9 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-04-22 13:29 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
	xen-devel, stefano.stabellini, keir

On Mon, 2015-04-20 at 17:06 +0200, Tamas K Lengyel wrote:

I've now applied everything except:

>   xen: Make gpfn related memops compatible with wider return values

Which was the subject of some discussion and which you told me on IRC
wasn't critical in this round.

I also updated one of the commit messages as discussed.

Thanks!

Ian.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-21 15:13               ` Jan Beulich
  2015-04-21 15:21                 ` Tamas K Lengyel
@ 2015-04-25 16:54                 ` Julien Grall
  2015-04-27  7:02                   ` Jan Beulich
  1 sibling, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-04-25 16:54 UTC (permalink / raw)
  To: Jan Beulich, Tamas K Lengyel
  Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Andrew Cooper,
	Julien Grall, Tim Deegan, Xen-devel, Stefano Stabellini,
	Keir Fraser, Ian Jackson

Hi Jan,

On 21/04/2015 20:13, Jan Beulich wrote:
> For this specific one - is there a reasonable use case? Other than
> for host PFN, we have control over guest ones, and I'm not sure
> managing a guest with GPFNs extending past 4 billion can be
> expected to work if only this one hypercall got fixed. IOW I'm
> expecting to NAK any such addition without proper rationale.

There is hardware coming out with 48 bits address support (i.e 36 bit pfn).

Even though the current layout of 64bit address space is using 40 bits 
IPA, I wouldn't be surprise if we decide to extend it soon (I have in 
mind PCI passthrough).

Without this new hypercall, you rule out the possibility to run the 
toolstack (included memaccess or any software requiring the maximum PFN 
used by a domain) in a 32bit domain or 32bit userspace on 64bit domain.

I don't have a good use case, but I don't see why we should omit a such 
possibility. It would be better if we can fix now rather than waiting 
until someone need it.

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-25 16:54                 ` Julien Grall
@ 2015-04-27  7:02                   ` Jan Beulich
  2015-04-27 10:18                     ` Julien Grall
  0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2015-04-27  7:02 UTC (permalink / raw)
  To: julien.grall, tklengyel
  Cc: tim, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3,
	julien.grall, ian.jackson, xen-devel, stefano.stabellini, keir

>>> Julien Grall <julien.grall@citrix.com> 04/25/15 10:37 PM >>>
>On 21/04/2015 20:13, Jan Beulich wrote:
>> For this specific one - is there a reasonable use case? Other than
>> for host PFN, we have control over guest ones, and I'm not sure
>> managing a guest with GPFNs extending past 4 billion can be
>> expected to work if only this one hypercall got fixed. IOW I'm
>> expecting to NAK any such addition without proper rationale.
>
>There is hardware coming out with 48 bits address support (i.e 36 bit pfn).
>
>Even though the current layout of 64bit address space is using 40 bits 
>IPA, I wouldn't be surprise if we decide to extend it soon (I have in 
>mind PCI passthrough).
>
>Without this new hypercall, you rule out the possibility to run the 
>toolstack (included memaccess or any software requiring the maximum PFN 
>used by a domain) in a 32bit domain or 32bit userspace on 64bit domain.

For a 32-bit domain, I suppose there are more limitations (unsigned long
being used for MFNs/PFNs), so I don't see how this one addition would
help. For 32-bit userspace on 64-bit domains the hypercall again isn't
the limiting factor, but the kernel's hypercall interface is. (And again I
doubt widening the MFN/PFN/GFN representation just here would
really make 32-bit userspace work on such large hosts.)

Jan

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values
  2015-04-27  7:02                   ` Jan Beulich
@ 2015-04-27 10:18                     ` Julien Grall
  0 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-04-27 10:18 UTC (permalink / raw)
  To: Jan Beulich, julien.grall, tklengyel
  Cc: wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3,
	julien.grall, tim, xen-devel, stefano.stabellini, keir,
	ian.jackson

Hi Jan,

On 27/04/2015 08:02, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@citrix.com> 04/25/15 10:37 PM >>>
>> On 21/04/2015 20:13, Jan Beulich wrote:
>>> For this specific one - is there a reasonable use case? Other than
>>> for host PFN, we have control over guest ones, and I'm not sure
>>> managing a guest with GPFNs extending past 4 billion can be
>>> expected to work if only this one hypercall got fixed. IOW I'm
>>> expecting to NAK any such addition without proper rationale.
>>
>> There is hardware coming out with 48 bits address support (i.e 36 bit pfn).
>>
>> Even though the current layout of 64bit address space is using 40 bits
>> IPA, I wouldn't be surprise if we decide to extend it soon (I have in
>> mind PCI passthrough).
>>
>> Without this new hypercall, you rule out the possibility to run the
>> toolstack (included memaccess or any software requiring the maximum PFN
>> used by a domain) in a 32bit domain or 32bit userspace on 64bit domain.
>
> For a 32-bit domain, I suppose there are more limitations (unsigned long
> being used for MFNs/PFNs), so I don't see how this one addition would
> help.  For 32-bit userspace on 64-bit domains the hypercall again isn't
> the limiting factor, but the kernel's hypercall interface is. (And again I
> doubt widening the MFN/PFN/GFN representation just here would
> really make 32-bit userspace work on such large hosts.)

On ARM 32/64 bits xen_pfn_t is defined as uint64_t. So I don't see any 
problem, unless the toolstack uses unsigned long which is already 
obviously wrong...

Although, I still don't see why it prevents us to fix this hypercall... 
It's the first step to make the toolstack x86 agnostic (because unsigned 
long is x86 specific).

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2015-04-27 10:18 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-20 15:06 [PATCH V15 0/9] Mem_access for ARM Tamas K Lengyel
2015-04-20 15:06 ` [PATCH V15 1/9] xen/arm: groundwork for mem_access support on ARM Tamas K Lengyel
2015-04-20 15:06 ` [PATCH V15 2/9] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
2015-04-21 13:14   ` Ian Campbell
2015-04-21 14:10     ` Tamas K Lengyel
2015-04-21 14:22       ` Ian Campbell
2015-04-20 15:06 ` [PATCH V15 3/9] xen/arm: Data abort exception (R/W) mem_access events Tamas K Lengyel
2015-04-21 13:16   ` Ian Campbell
2015-04-20 15:06 ` [PATCH V15 4/9] xen/arm: Instruction prefetch abort (X) mem_access event handling Tamas K Lengyel
2015-04-21 13:19   ` Ian Campbell
2015-04-20 15:06 ` [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values Tamas K Lengyel
2015-04-20 15:22   ` Andrew Cooper
2015-04-20 15:25     ` Tamas K Lengyel
2015-04-21 13:23     ` Ian Campbell
2015-04-21 14:14       ` Jan Beulich
2015-04-21 14:24         ` Ian Campbell
2015-04-21 14:29           ` Jan Beulich
2015-04-21 14:42             ` Tamas K Lengyel
2015-04-21 15:13               ` Jan Beulich
2015-04-21 15:21                 ` Tamas K Lengyel
2015-04-25 16:54                 ` Julien Grall
2015-04-27  7:02                   ` Jan Beulich
2015-04-27 10:18                     ` Julien Grall
2015-04-21 14:47             ` Ian Campbell
2015-04-21 14:33         ` Tamas K Lengyel
2015-04-21 14:41           ` Jan Beulich
2015-04-20 15:06 ` [PATCH V15 6/9] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
2015-04-21 13:24   ` Ian Campbell
2015-04-20 15:06 ` [PATCH V15 7/9] tools/libxc: Allocate magic page for mem access on ARM Tamas K Lengyel
2015-04-20 15:06 ` [PATCH V15 8/9] tools/tests: Enable xen-access " Tamas K Lengyel
2015-04-21 13:28   ` Ian Campbell
2015-04-21 14:13     ` Tamas K Lengyel
2015-04-20 15:06 ` [PATCH V15 9/9] xen/arm: Enable mem_access " Tamas K Lengyel
2015-04-22 13:29 ` [PATCH V15 0/9] Mem_access for ARM Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.