[PATCH 0/4 v2] xen/arm: fix guest builder cache cohenrency (again, again)

[PATCH 0/4 v2] xen/arm: fix guest builder cache cohenrency (again, again)

[Ian Campbell]

Changes in v2:
        Flush on page alloc and do targeted flushes at domain build time
        rather than a big flush after domain build. This adds a new call
        to common code, which is stubbed out on x86. This avoid needing
        to worry about preemptability of the new domctl and also catches
        cases related to ballooning where things might not be flushed
        (e.g. a guest scrubs a page but doesn't clean the cache)

This has done 12000 boot loops on arm32 and 10000 on arm64.

Given the security aspect I would like to put this in 4.4.

Original blurb:

On ARM we need to take care of cache coherency for guests which we have
just built because they start with their caches disabled.

Our current strategy for dealing with this, which is to make guest
memory default to cacheable regardless of the in guest configuration
(the HCR.DC bit), is flawed because it doesn't handle guests which
enable their MMU before enabling their caches, which at least FreeBSD
does. (NB: Setting HCR.DC while the guest MMU is enabled is
UNPREDICTABLE, hence we must disable it when the guest turns its MMU
one).

There is also a security aspect here since the current strategy means
that a guest which enables its MMU before its caches can potentially see
unscrubbed data in RAM (because the scrubbed bytes are still held in the
cache).

As well as the new stuff this series removes the HCR.DC support and
performs two purely cosmetic renames.

Ian.

[PATCH v2 1/4] xen: arm: rename create_p2m_entries to apply_p2m_changes

[Ian Campbell]

This function hasn't been only about creating for quite a while.

This is purely a rename.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/p2m.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 85ca330..ace3c54 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -230,7 +230,7 @@ enum p2m_operation {
     RELINQUISH,
 };
 
-static int create_p2m_entries(struct domain *d,
+static int apply_p2m_changes(struct domain *d,
                      enum p2m_operation op,
                      paddr_t start_gpaddr,
                      paddr_t end_gpaddr,
@@ -438,8 +438,8 @@ int p2m_populate_ram(struct domain *d,
                      paddr_t start,
                      paddr_t end)
 {
-    return create_p2m_entries(d, ALLOCATE, start, end,
-                              0, MATTR_MEM, p2m_ram_rw);
+    return apply_p2m_changes(d, ALLOCATE, start, end,
+                             0, MATTR_MEM, p2m_ram_rw);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -447,8 +447,8 @@ int map_mmio_regions(struct domain *d,
                      paddr_t end_gaddr,
                      paddr_t maddr)
 {
-    return create_p2m_entries(d, INSERT, start_gaddr, end_gaddr,
-                              maddr, MATTR_DEV, p2m_mmio_direct);
+    return apply_p2m_changes(d, INSERT, start_gaddr, end_gaddr,
+                             maddr, MATTR_DEV, p2m_mmio_direct);
 }
 
 int guest_physmap_add_entry(struct domain *d,
@@ -457,20 +457,20 @@ int guest_physmap_add_entry(struct domain *d,
                             unsigned long page_order,
                             p2m_type_t t)
 {
-    return create_p2m_entries(d, INSERT,
-                              pfn_to_paddr(gpfn),
-                              pfn_to_paddr(gpfn + (1 << page_order)),
-                              pfn_to_paddr(mfn), MATTR_MEM, t);
+    return apply_p2m_changes(d, INSERT,
+                             pfn_to_paddr(gpfn),
+                             pfn_to_paddr(gpfn + (1 << page_order)),
+                             pfn_to_paddr(mfn), MATTR_MEM, t);
 }
 
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order)
 {
-    create_p2m_entries(d, REMOVE,
-                       pfn_to_paddr(gpfn),
-                       pfn_to_paddr(gpfn + (1<<page_order)),
-                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
+    apply_p2m_changes(d, REMOVE,
+                      pfn_to_paddr(gpfn),
+                      pfn_to_paddr(gpfn + (1<<page_order)),
+                      pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
 }
 
 int p2m_alloc_table(struct domain *d)
@@ -618,7 +618,7 @@ int relinquish_p2m_mapping(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
 
-    return create_p2m_entries(d, RELINQUISH,
+    return apply_p2m_changes(d, RELINQUISH,
                               pfn_to_paddr(p2m->next_gfn_to_relinquish),
                               pfn_to_paddr(p2m->max_mapped_gfn),
                               pfn_to_paddr(INVALID_MFN),
-- 
1.7.10.4

[PATCH v2 2/4] xen: arm: rename p2m next_gfn_to_relinquish to lowest_mapped_gfn

[Ian Campbell]

This has other uses other than during relinquish, so rename it for clarity.

This is a pure rename.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/p2m.c        |    9 ++++-----
 xen/include/asm-arm/p2m.h |    8 +++++---
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ace3c54..a61edeb 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -388,7 +388,7 @@ static int apply_p2m_changes(struct domain *d,
         {
             if ( hypercall_preempt_check() )
             {
-                p2m->next_gfn_to_relinquish = addr >> PAGE_SHIFT;
+                p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
                 rc = -EAGAIN;
                 goto out;
             }
@@ -415,8 +415,7 @@ static int apply_p2m_changes(struct domain *d,
         unsigned long egfn = paddr_to_pfn(end_gpaddr);
 
         p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
-        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
-        p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn);
+        p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn);
     }
 
     rc = 0;
@@ -606,7 +605,7 @@ int p2m_init(struct domain *d)
     p2m->first_level = NULL;
 
     p2m->max_mapped_gfn = 0;
-    p2m->next_gfn_to_relinquish = ULONG_MAX;
+    p2m->lowest_mapped_gfn = ULONG_MAX;
 
 err:
     spin_unlock(&p2m->lock);
@@ -619,7 +618,7 @@ int relinquish_p2m_mapping(struct domain *d)
     struct p2m_domain *p2m = &d->arch.p2m;
 
     return apply_p2m_changes(d, RELINQUISH,
-                              pfn_to_paddr(p2m->next_gfn_to_relinquish),
+                              pfn_to_paddr(p2m->lowest_mapped_gfn),
                               pfn_to_paddr(p2m->max_mapped_gfn),
                               pfn_to_paddr(INVALID_MFN),
                               MATTR_MEM, p2m_invalid);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 53b3266..e9c884a 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -24,9 +24,11 @@ struct p2m_domain {
      */
     unsigned long max_mapped_gfn;
 
-    /* When releasing mapped gfn's in a preemptible manner, recall where
-     * to resume the search */
-    unsigned long next_gfn_to_relinquish;
+    /* Lowest mapped gfn in the p2m. When releasing mapped gfn's in a
+     * preemptible manner this is update to track recall where to
+     * resume the search. Apart from during teardown this can only
+     * decrease. */
+    unsigned long lowest_mapped_gfn;
 };
 
 /* List of possible type for each page in the p2m entry.
-- 
1.7.10.4

[PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Campbell]

Guests are initially started with caches disabled and so we need to make sure
they see consistent data in RAM (requiring a cache clean) but also that they
do not have old stale data suddenly appear in the caches when they enable
their caches (requiring the invalidate).

This can be split into two halves. First we must flush each page as it is
allocated to the guest. It is not sufficient to do the flush at scrub time
since this will miss pages which are ballooned out by the guest (where the
guest must scrub if it cares about not leaking the pagecontent). We need to
clean as well as invalidate to make sure that any scrubbing which has occured
gets committed to real RAM. To achieve this add a new cacheflush_page function,
which is a stub on x86.

Secondly we need to flush anything which the domain builder touches, which we
do via a new domctl.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: jbeulich@suse.com
Cc: keir@xen.org
Cc: ian.jackson@eu.citrix.com
--
v2:
   Switch to cleaning at page allocation time + explicit flushing of tha eregions
   which the toolstack touches.

   Add XSM for new domctl.

   New domctl restricts the amount of space it is willing to flush, to avoid
   thinking about preemption.
---
 tools/libxc/xc_dom_boot.c           |    4 ++++
 tools/libxc/xc_dom_core.c           |    3 +++
 tools/libxc/xc_domain.c             |   10 ++++++++++
 tools/libxc/xc_private.c            |    2 ++
 tools/libxc/xenctrl.h               |    3 ++-
 xen/arch/arm/domctl.c               |   14 ++++++++++++++
 xen/arch/arm/mm.c                   |    9 +++++++++
 xen/arch/arm/p2m.c                  |   24 ++++++++++++++++++++++++
 xen/common/page_alloc.c             |    5 +++++
 xen/include/asm-arm/p2m.h           |    3 +++
 xen/include/asm-arm/page.h          |    3 +++
 xen/include/asm-x86/page.h          |    3 +++
 xen/include/public/domctl.h         |   13 +++++++++++++
 xen/xsm/flask/hooks.c               |    3 +++
 xen/xsm/flask/policy/access_vectors |    2 ++
 15 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 5a9cfc6..3b3f2fb 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -351,6 +351,10 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
         return -1;
     }
 
+    /* Guest shouldn't really touch its grant table until it has
+     * enabled its caches. But lets be nice. */
+    xc_domain_cacheflush(xch, domid, gnttab_gmfn, gnttab_gmfn + 1);
+
     return 0;
 }
 
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 77a4e64..306b414 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -603,6 +603,9 @@ void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn)
         prev->next = phys->next;
     else
         dom->phys_pages = phys->next;
+
+    xc_domain_cacheflush(dom->xch, dom->guest_domid,
+                         phys->first, phys->first + phys->count);
 }
 
 void xc_dom_unmap_all(struct xc_dom_image *dom)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index c2fdd74..092d610 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -48,6 +48,16 @@ int xc_domain_create(xc_interface *xch,
     return 0;
 }
 
+int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
+                         xen_pfn_t start_pfn, xen_pfn_t end_pfn)
+{
+    DECLARE_DOMCTL;
+    domctl.cmd = XEN_DOMCTL_cacheflush;
+    domctl.domain = (domid_t)domid;
+    domctl.u.cacheflush.start_pfn = start_pfn;
+    domctl.u.cacheflush.end_pfn = end_pfn;
+    return do_domctl(xch, &domctl);
+}
 
 int xc_domain_pause(xc_interface *xch,
                     uint32_t domid)
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 838fd21..556810f 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -628,6 +628,7 @@ int xc_copy_to_domain_page(xc_interface *xch,
         return -1;
     memcpy(vaddr, src_page, PAGE_SIZE);
     munmap(vaddr, PAGE_SIZE);
+    xc_domain_cacheflush(xch, domid, dst_pfn, dst_pfn+1);
     return 0;
 }
 
@@ -641,6 +642,7 @@ int xc_clear_domain_page(xc_interface *xch,
         return -1;
     memset(vaddr, 0, PAGE_SIZE);
     munmap(vaddr, PAGE_SIZE);
+    xc_domain_cacheflush(xch, domid, dst_pfn, dst_pfn+1);
     return 0;
 }
 
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 13f816b..80c397e 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -453,7 +453,8 @@ int xc_domain_create(xc_interface *xch,
                      xen_domain_handle_t handle,
                      uint32_t flags,
                      uint32_t *pdomid);
-
+int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
+			 xen_pfn_t start_pfn, xen_pfn_t end_pfn);
 
 /* Functions to produce a dump of a given domain
  *  xc_domain_dumpcore - produces a dump to a specified file
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 546e86b..8916e49 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -17,6 +17,20 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
 {
     switch ( domctl->cmd )
     {
+    case XEN_DOMCTL_cacheflush:
+    {
+        unsigned long s = domctl->u.cacheflush.start_pfn;
+        unsigned long e = domctl->u.cacheflush.end_pfn;
+
+        if ( e < s )
+            return -EINVAL;
+
+        if ( get_order_from_pages(e-s) > MAX_ORDER )
+            return -EINVAL;
+
+        return p2m_cache_flush(d, s, e);
+    }
+
     default:
         return subarch_do_domctl(domctl, d, u_domctl);
     }
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 2f48347..0c1a7b9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -338,6 +338,15 @@ unsigned long domain_page_map_to_mfn(const void *va)
 }
 #endif
 
+void cacheflush_page(unsigned long mfn)
+{
+    void *v = map_domain_page(mfn);
+
+    flush_xen_dcache_va_range(v, PAGE_SIZE);
+
+    unmap_domain_page(v);
+}
+
 void __init arch_init_memory(void)
 {
     /*
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a61edeb..d452814 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -8,6 +8,7 @@
 #include <asm/gic.h>
 #include <asm/event.h>
 #include <asm/hardirq.h>
+#include <asm/page.h>
 
 /* First level P2M is 2 consecutive pages */
 #define P2M_FIRST_ORDER 1
@@ -228,6 +229,7 @@ enum p2m_operation {
     ALLOCATE,
     REMOVE,
     RELINQUISH,
+    CACHEFLUSH,
 };
 
 static int apply_p2m_changes(struct domain *d,
@@ -381,6 +383,14 @@ static int apply_p2m_changes(struct domain *d,
                     count++;
                 }
                 break;
+
+            case CACHEFLUSH:
+                {
+                    if ( !pte.p2m.valid || !p2m_is_ram(pte.p2m.type) )
+                        break;
+                    cacheflush_page(pte.p2m.base);
+                }
+                break;
         }
 
         /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
@@ -624,6 +634,20 @@ int relinquish_p2m_mapping(struct domain *d)
                               MATTR_MEM, p2m_invalid);
 }
 
+int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
+{
+    struct p2m_domain *p2m = &d->arch.p2m;
+
+    start_mfn = MAX(start_mfn, p2m->lowest_mapped_gfn);
+    end_mfn = MIN(end_mfn, p2m->max_mapped_gfn);
+
+    return apply_p2m_changes(d, CACHEFLUSH,
+                             pfn_to_paddr(start_mfn),
+                             pfn_to_paddr(end_mfn),
+                             pfn_to_paddr(INVALID_MFN),
+                             MATTR_MEM, p2m_invalid);
+}
+
 unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
 {
     paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5f484a2..16bb739 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -710,6 +710,11 @@ static struct page_info *alloc_heap_pages(
         /* Initialise fields which have other uses for free pages. */
         pg[i].u.inuse.type_info = 0;
         page_set_owner(&pg[i], NULL);
+
+        /* Ensure cache and RAM are consistent for platforms where the
+         * guest can control its own visibility of/through the cache.
+         */
+        cacheflush_page(page_to_mfn(&pg[i]));
     }
 
     spin_unlock(&heap_lock);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index e9c884a..3b39c45 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -78,6 +78,9 @@ void p2m_load_VTTBR(struct domain *d);
 /* Look up the MFN corresponding to a domain's PFN. */
 paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
 
+/* Clean & invalidate caches corresponding to a region of guest address space */
+int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
+
 /* Setup p2m RAM mapping for domain d from start-end. */
 int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
 /* Map MMIO regions in the p2m: start_gaddr and end_gaddr is the range
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 670d4e7..342dde8 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -253,6 +253,9 @@ static inline void flush_xen_dcache_va_range(void *p, unsigned long size)
             : : "r" (_p), "m" (*_p));                                   \
 } while (0)
 
+/* Flush the dcache for an entire page. */
+void cacheflush_page(unsigned long mfn);
+
 /* Print a walk of an arbitrary page table */
 void dump_pt_walk(lpae_t *table, paddr_t addr);
 
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 7a46af5..271517f 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -346,6 +346,9 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
     return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
 }
 
+/* No cache maintenance required on x86 architecture. */
+static inline void cacheflush_page(unsigned long mfn) {}
+
 /* return true if permission increased */
 static inline bool_t
 perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 91f01fa..d8d8727 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -885,6 +885,17 @@ struct xen_domctl_set_max_evtchn {
 typedef struct xen_domctl_set_max_evtchn xen_domctl_set_max_evtchn_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_max_evtchn_t);
 
+/*
+ * ARM: Clean and invalidate caches associated with given region of
+ * guest memory.
+ */
+struct xen_domctl_cacheflush {
+    /* IN: page range to flush: [start, end). */
+    xen_pfn_t start_pfn, end_pfn;
+};
+typedef struct xen_domctl_cacheflush xen_domctl_cacheflush_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_cacheflush_t);
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -954,6 +965,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_setnodeaffinity               68
 #define XEN_DOMCTL_getnodeaffinity               69
 #define XEN_DOMCTL_set_max_evtchn                70
+#define XEN_DOMCTL_cacheflush                    71
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1012,6 +1024,7 @@ struct xen_domctl {
         struct xen_domctl_set_max_evtchn    set_max_evtchn;
         struct xen_domctl_gdbsx_memio       gdbsx_guest_memio;
         struct xen_domctl_set_broken_page_p2m set_broken_page_p2m;
+        struct xen_domctl_cacheflush        cacheflush;
         struct xen_domctl_gdbsx_pauseunp_vcpu gdbsx_pauseunp_vcpu;
         struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
         uint8_t                             pad[128];
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 50a35fc..1345d7e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -737,6 +737,9 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_set_max_evtchn:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_MAX_EVTCHN);
 
+    case XEN_DOMCTL_cacheflush:
+	    return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CACHEFLUSH);
+
     default:
         printk("flask_domctl: Unknown op %d\n", cmd);
         return -EPERM;
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 1fbe241..a0ed13d 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -196,6 +196,8 @@ class domain2
     setclaim
 # XEN_DOMCTL_set_max_evtchn
     set_max_evtchn
+# XEN_DOMCTL_cacheflush
+    cacheflush
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
1.7.10.4

[PATCH v2 4/4] Revert "xen: arm: force guest memory accesses to cacheable when MMU is disabled"

[Ian Campbell]

This reverts commit 89eb02c2204a0b42a0aa169f107bc346a3fef802.

This approach has a short coming in that it breaks when a guest enables its
MMU (SCTLR.M, disabling HCR.DC) without enabling caches (SCTLR.C) first/at the
same time. It turns out that FreeBSD does this.

This has now been fixed (yet) another way (third time is the charm!) so remove
this support. The original commit contained some fixes which are still
relevant even with the revert of the bulk of the patch:
 - Correction to HSR_SYSREG_CRN_MASK
 - Rename of HSR_SYSCTL macros to avoid naming clash
 - Definition of some additional cp reg specifications

Since these are still useful they are not reverted.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Move to end of series
    Do not revert useful bits
---
 xen/arch/arm/domain.c        |    7 --
 xen/arch/arm/traps.c         |  159 +-----------------------------------------
 xen/include/asm-arm/domain.h |    2 -
 3 files changed, 1 insertion(+), 167 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 635a9a4..124cccf 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -19,7 +19,6 @@
 #include <xen/errno.h>
 #include <xen/bitops.h>
 #include <xen/grant_table.h>
-#include <xen/stdbool.h>
 
 #include <asm/current.h>
 #include <asm/event.h>
@@ -220,11 +219,6 @@ static void ctxt_switch_to(struct vcpu *n)
     else
         hcr |= HCR_RW;
 
-    if ( n->arch.default_cache )
-        hcr |= (HCR_TVM|HCR_DC);
-    else
-        hcr &= ~(HCR_TVM|HCR_DC);
-
     WRITE_SYSREG(hcr, HCR_EL2);
     isb();
 
@@ -475,7 +469,6 @@ int vcpu_initialise(struct vcpu *v)
         return rc;
 
     v->arch.sctlr = SCTLR_GUEST_INIT;
-    v->arch.default_cache = true;
 
     /*
      * By default exposes an SMP system with AFF0 set to the VCPU ID
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b8f2e82..ec51d1b 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -29,14 +29,12 @@
 #include <xen/hypercall.h>
 #include <xen/softirq.h>
 #include <xen/domain_page.h>
-#include <xen/stdbool.h>
 #include <public/sched.h>
 #include <public/xen.h>
 #include <asm/event.h>
 #include <asm/regs.h>
 #include <asm/cpregs.h>
 #include <asm/psci.h>
-#include <asm/flushtlb.h>
 
 #include "decode.h"
 #include "io.h"
@@ -1292,29 +1290,6 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
     regs->pc += hsr.len ? 4 : 2;
 }
 
-static void update_sctlr(struct vcpu *v, uint32_t val)
-{
-    /*
-     * If MMU (SCTLR_M) is now enabled then we must disable HCR.DC
-     * because they are incompatible.
-     *
-     * Once HCR.DC is disabled then we do not need HCR_TVM either,
-     * since it's only purpose was to catch the MMU being enabled.
-     *
-     * Both are set appropriately on context switch but we need to
-     * clear them now since we may not context switch on return to
-     * guest.
-     */
-    if ( val & SCTLR_M )
-    {
-        WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
-        /* ARM ARM 0406C.b B3.2.1: Disabling HCR.DC without changing
-         * VMID requires us to flush the TLB for that VMID. */
-        flush_tlb();
-        v->arch.default_cache = false;
-    }
-}
-
 static void do_cp15_32(struct cpu_user_regs *regs,
                        union hsr hsr)
 {
@@ -1374,89 +1349,6 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
-
-/* Passthru a 32-bit AArch32 register which is also 32-bit under AArch64 */
-#define CP32_PASSTHRU32(R...) do {              \
-    if ( cp32.read )                            \
-        *r = READ_SYSREG32(R);                  \
-    else                                        \
-        WRITE_SYSREG32(*r, R);                  \
-} while(0)
-
-/*
- * Passthru a 32-bit AArch32 register which is 64-bit under AArch64.
- * Updates the lower 32-bits and clears the upper bits.
- */
-#define CP32_PASSTHRU64(R...) do {              \
-    if ( cp32.read )                            \
-        *r = (uint32_t)READ_SYSREG64(R);        \
-    else                                        \
-        WRITE_SYSREG64((uint64_t)*r, R);        \
-} while(0)
-
-/*
- * Passthru a 32-bit AArch32 register which is 64-bit under AArch64.
- * Updates either the HI ([63:32]) or LO ([31:0]) 32-bits preserving
- * the other half.
- */
-#ifdef CONFIG_ARM_64
-#define CP32_PASSTHRU64_HI(R...) do {                   \
-    if ( cp32.read )                                    \
-        *r = (uint32_t)(READ_SYSREG64(R) >> 32);        \
-    else                                                \
-    {                                                   \
-        uint64_t t = READ_SYSREG64(R) & 0xffffffffUL;   \
-        t |= ((uint64_t)(*r)) << 32;                    \
-        WRITE_SYSREG64(t, R);                           \
-    }                                                   \
-} while(0)
-#define CP32_PASSTHRU64_LO(R...) do {                           \
-    if ( cp32.read )                                            \
-        *r = (uint32_t)(READ_SYSREG64(R) & 0xffffffff);         \
-    else                                                        \
-    {                                                           \
-        uint64_t t = READ_SYSREG64(R) & 0xffffffff00000000UL;   \
-        t |= *r;                                                \
-        WRITE_SYSREG64(t, R);                                   \
-    }                                                           \
-} while(0)
-#endif
-
-    /* HCR.TVM */
-    case HSR_CPREG32(SCTLR):
-        CP32_PASSTHRU32(SCTLR_EL1);
-        update_sctlr(v, *r);
-        break;
-    case HSR_CPREG32(TTBR0_32):   CP32_PASSTHRU64(TTBR0_EL1);      break;
-    case HSR_CPREG32(TTBR1_32):   CP32_PASSTHRU64(TTBR1_EL1);      break;
-    case HSR_CPREG32(TTBCR):      CP32_PASSTHRU32(TCR_EL1);        break;
-    case HSR_CPREG32(DACR):       CP32_PASSTHRU32(DACR32_EL2);     break;
-    case HSR_CPREG32(DFSR):       CP32_PASSTHRU32(ESR_EL1);        break;
-    case HSR_CPREG32(IFSR):       CP32_PASSTHRU32(IFSR32_EL2);     break;
-    case HSR_CPREG32(ADFSR):      CP32_PASSTHRU32(AFSR0_EL1);      break;
-    case HSR_CPREG32(AIFSR):      CP32_PASSTHRU32(AFSR1_EL1);      break;
-    case HSR_CPREG32(CONTEXTIDR): CP32_PASSTHRU32(CONTEXTIDR_EL1); break;
-
-#ifdef CONFIG_ARM_64
-    case HSR_CPREG32(DFAR):       CP32_PASSTHRU64_LO(FAR_EL1);     break;
-    case HSR_CPREG32(IFAR):       CP32_PASSTHRU64_HI(FAR_EL1);     break;
-    case HSR_CPREG32(MAIR0):      CP32_PASSTHRU64_LO(MAIR_EL1);    break;
-    case HSR_CPREG32(MAIR1):      CP32_PASSTHRU64_HI(MAIR_EL1);    break;
-    case HSR_CPREG32(AMAIR0):     CP32_PASSTHRU64_LO(AMAIR_EL1);   break;
-    case HSR_CPREG32(AMAIR1):     CP32_PASSTHRU64_HI(AMAIR_EL1);   break;
-#else
-    case HSR_CPREG32(DFAR):       CP32_PASSTHRU32(DFAR);           break;
-    case HSR_CPREG32(IFAR):       CP32_PASSTHRU32(IFAR);           break;
-    case HSR_CPREG32(MAIR0):      CP32_PASSTHRU32(MAIR0);          break;
-    case HSR_CPREG32(MAIR1):      CP32_PASSTHRU32(MAIR1);          break;
-    case HSR_CPREG32(AMAIR0):     CP32_PASSTHRU32(AMAIR0);         break;
-    case HSR_CPREG32(AMAIR1):     CP32_PASSTHRU32(AMAIR1);         break;
-#endif
-
-#undef CP32_PASSTHRU32
-#undef CP32_PASSTHRU64
-#undef CP32_PASSTHRU64_LO
-#undef CP32_PASSTHRU64_HI
     default:
         printk("%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
                cp32.read ? "mrc" : "mcr",
@@ -1470,9 +1362,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
                        union hsr hsr)
 {
     struct hsr_cp64 cp64 = hsr.cp64;
-    uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1);
-    uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2);
-    uint64_t r;
 
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -1490,26 +1379,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
             domain_crash_synchronous();
         }
         break;
-
-#define CP64_PASSTHRU(R...) do {                                  \
-    if ( cp64.read )                                            \
-    {                                                           \
-        r = READ_SYSREG64(R);                                   \
-        *r1 = r & 0xffffffffUL;                                 \
-        *r2 = r >> 32;                                          \
-    }                                                           \
-    else                                                        \
-    {                                                           \
-        r = (*r1) | (((uint64_t)(*r2))<<32);                    \
-        WRITE_SYSREG64(r, R);                                   \
-    }                                                           \
-} while(0)
-
-    case HSR_CPREG64(TTBR0): CP64_PASSTHRU(TTBR0_EL1); break;
-    case HSR_CPREG64(TTBR1): CP64_PASSTHRU(TTBR1_EL1); break;
-
-#undef CP64_PASSTHRU
-
     default:
         printk("%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
                cp64.read ? "mrrc" : "mcrr",
@@ -1524,8 +1393,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
                       union hsr hsr)
 {
     struct hsr_sysreg sysreg = hsr.sysreg;
-    register_t *x = select_user_reg(regs, sysreg.reg);
-    struct vcpu *v = current;
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
@@ -1538,31 +1405,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
             domain_crash_synchronous();
         }
         break;
-
-#define SYSREG_PASSTHRU(R...) do {              \
-    if ( sysreg.read )                          \
-        *x = READ_SYSREG(R);                    \
-    else                                        \
-        WRITE_SYSREG(*x, R);                    \
-} while(0)
-
-    case HSR_SYSREG_SCTLR_EL1:
-        SYSREG_PASSTHRU(SCTLR_EL1);
-        update_sctlr(v, *x);
-        break;
-    case HSR_SYSREG_TTBR0_EL1:      SYSREG_PASSTHRU(TTBR0_EL1);      break;
-    case HSR_SYSREG_TTBR1_EL1:      SYSREG_PASSTHRU(TTBR1_EL1);      break;
-    case HSR_SYSREG_TCR_EL1:        SYSREG_PASSTHRU(TCR_EL1);        break;
-    case HSR_SYSREG_ESR_EL1:        SYSREG_PASSTHRU(ESR_EL1);        break;
-    case HSR_SYSREG_FAR_EL1:        SYSREG_PASSTHRU(FAR_EL1);        break;
-    case HSR_SYSREG_AFSR0_EL1:      SYSREG_PASSTHRU(AFSR0_EL1);      break;
-    case HSR_SYSREG_AFSR1_EL1:      SYSREG_PASSTHRU(AFSR1_EL1);      break;
-    case HSR_SYSREG_MAIR_EL1:       SYSREG_PASSTHRU(MAIR_EL1);       break;
-    case HSR_SYSREG_AMAIR_EL1:      SYSREG_PASSTHRU(AMAIR_EL1);      break;
-    case HSR_SYSREG_CONTEXTIDR_EL1: SYSREG_PASSTHRU(CONTEXTIDR_EL1); break;
-
-#undef SYSREG_PASSTHRU
-
     default:
         printk("%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
                sysreg.read ? "mrs" : "msr",
@@ -1635,6 +1477,7 @@ done:
     if (first) unmap_domain_page(first);
 }
 
+
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
                                       union hsr hsr)
 {
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index af8c64b..bc20a15 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -257,8 +257,6 @@ struct arch_vcpu
     uint64_t event_mask;
     uint64_t lr_mask;
 
-    bool_t default_cache;
-
     struct {
         /*
          * SGIs and PPIs are per-VCPU, SPIs are domain global and in
-- 
1.7.10.4

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Jan Beulich]

>>> On 04.02.14 at 15:22, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -351,6 +351,10 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
>          return -1;
>      }
>  
> +    /* Guest shouldn't really touch its grant table until it has
> +     * enabled its caches. But lets be nice. */
> +    xc_domain_cacheflush(xch, domid, gnttab_gmfn, gnttab_gmfn + 1);

Looking at this and further similar code I think it would be cleaner
for the xc interface to take a start MFN and a count, and for the
hypervisor interface to use an inclusive range (such that overflow
is not a problem).

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -346,6 +346,9 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
>      return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
>  }
>  
> +/* No cache maintenance required on x86 architecture. */
> +static inline void cacheflush_page(unsigned long mfn) {}

The function name is certainly sub-optimal: If I needed a page-range
cache flush and found a function named like this, I'd assume it does
what its name says - flush the page from the cache. sync_page() or
some such may be better suited to express that this is something
that may be a no-op on certain architectures.

Jan

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Jackson]

Ian Campbell writes ("[PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> Guests are initially started with caches disabled and so we need to make sure
> they see consistent data in RAM (requiring a cache clean) but also that they
> do not have old stale data suddenly appear in the caches when they enable
> their caches (requiring the invalidate).
> 
> This can be split into two halves. First we must flush each page as it is
> allocated to the guest. It is not sufficient to do the flush at scrub time
> since this will miss pages which are ballooned out by the guest (where the
> guest must scrub if it cares about not leaking the pagecontent). We need to
> clean as well as invalidate to make sure that any scrubbing which has occured
> gets committed to real RAM. To achieve this add a new cacheflush_page function,
> which is a stub on x86.

> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index 77a4e64..306b414 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -603,6 +603,9 @@ void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn)
>          prev->next = phys->next;
>      else
>          dom->phys_pages = phys->next;
> +
> +    xc_domain_cacheflush(dom->xch, dom->guest_domid,
> +                         phys->first, phys->first + phys->count);
>  }

The approach you are taking here is that for pages explicitly mapped
by some libxc caller, you do the flush on unmap.  But what about
callers who don't unmap ?  Are there callers which don't unmap and
which instead are relying on memory coherency assumptions which aren't
true on arm ?

Aside from this question, the code in this patch looks fine to me.

Ian.

Re: [PATCH v2 4/4] Revert "xen: arm: force guest memory accesses to cacheable when MMU is disabled"

[Julien Grall]

On 02/04/2014 02:22 PM, Ian Campbell wrote:
> This reverts commit 89eb02c2204a0b42a0aa169f107bc346a3fef802.
> 
> This approach has a short coming in that it breaks when a guest enables its
> MMU (SCTLR.M, disabling HCR.DC) without enabling caches (SCTLR.C) first/at the
> same time. It turns out that FreeBSD does this.
> 
> This has now been fixed (yet) another way (third time is the charm!) so remove
> this support. The original commit contained some fixes which are still
> relevant even with the revert of the bulk of the patch:
>  - Correction to HSR_SYSREG_CRN_MASK
>  - Rename of HSR_SYSCTL macros to avoid naming clash
>  - Definition of some additional cp reg specifications
> 
> Since these are still useful they are not reverted.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Except the spurious line toward the end of the patch:

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

> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index b8f2e82..ec51d1b 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c

[..]

>      default:
>          printk("%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
>                 sysreg.read ? "mrs" : "msr",
> @@ -1635,6 +1477,7 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> +

Spurious change?

>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        union hsr hsr)


-- 
Julien Grall

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Campbell]

On Tue, 2014-02-04 at 15:32 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> > Guests are initially started with caches disabled and so we need to make sure
> > they see consistent data in RAM (requiring a cache clean) but also that they
> > do not have old stale data suddenly appear in the caches when they enable
> > their caches (requiring the invalidate).
> > 
> > This can be split into two halves. First we must flush each page as it is
> > allocated to the guest. It is not sufficient to do the flush at scrub time
> > since this will miss pages which are ballooned out by the guest (where the
> > guest must scrub if it cares about not leaking the pagecontent). We need to
> > clean as well as invalidate to make sure that any scrubbing which has occured
> > gets committed to real RAM. To achieve this add a new cacheflush_page function,
> > which is a stub on x86.
> 
> > diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> > index 77a4e64..306b414 100644
> > --- a/tools/libxc/xc_dom_core.c
> > +++ b/tools/libxc/xc_dom_core.c
> > @@ -603,6 +603,9 @@ void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn)
> >          prev->next = phys->next;
> >      else
> >          dom->phys_pages = phys->next;
> > +
> > +    xc_domain_cacheflush(dom->xch, dom->guest_domid,
> > +                         phys->first, phys->first + phys->count);
> >  }
> 
> The approach you are taking here is that for pages explicitly mapped
> by some libxc caller, you do the flush on unmap.  But what about
> callers who don't unmap ?  Are there callers which don't unmap and
> which instead are relying on memory coherency assumptions which aren't
> true on arm ?

Callers which don't unmap would be leaking mappings and therefore buggy
in a long running toolstack.

Also after the initial start of day period we require that the guest
enable its caches.

> Aside from this question, the code in this patch looks fine to me.

Thanks.

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Campbell]

On Tue, 2014-02-04 at 15:00 +0000, Jan Beulich wrote:
> >>> On 04.02.14 at 15:22, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/tools/libxc/xc_dom_boot.c
> > +++ b/tools/libxc/xc_dom_boot.c
> > @@ -351,6 +351,10 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
> >          return -1;
> >      }
> >  
> > +    /* Guest shouldn't really touch its grant table until it has
> > +     * enabled its caches. But lets be nice. */
> > +    xc_domain_cacheflush(xch, domid, gnttab_gmfn, gnttab_gmfn + 1);
> 
> Looking at this and further similar code I think it would be cleaner
> for the xc interface to take a start MFN and a count, and for the
> hypervisor interface to use an inclusive range (such that overflow
> is not a problem).

I think you might be right, I'll give that a go.

> 
> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -346,6 +346,9 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
> >      return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
> >  }
> >  
> > +/* No cache maintenance required on x86 architecture. */
> > +static inline void cacheflush_page(unsigned long mfn) {}
> 
> The function name is certainly sub-optimal:

Yes, I should have said that I wasn't really happy with it in the
context of x86.

>  If I needed a page-range
> cache flush and found a function named like this, I'd assume it does
> what its name says - flush the page from the cache. sync_page() or
> some such may be better suited to express that this is something
> that may be a no-op on certain architectures.

sync_page is a much better suggestion. Perhaps even
sync_page_to/from_cache, I suppose that might actually be more
confusing.

Ian.

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Jan Beulich]

>>> On 04.02.14 at 16:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> sync_page is a much better suggestion. Perhaps even
> sync_page_to/from_cache, I suppose that might actually be more
> confusing.

Hmm, on one hand it's even more precise, but otoh - how would a
sync_page_to_cache() look like?

Jan

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> On Tue, 2014-02-04 at 15:32 +0000, Ian Jackson wrote:
> > The approach you are taking here is that for pages explicitly mapped
> > by some libxc caller, you do the flush on unmap.  But what about
> > callers who don't unmap ?  Are there callers which don't unmap and
> > which instead are relying on memory coherency assumptions which aren't
> > true on arm ?
> 
> Callers which don't unmap would be leaking mappings and therefore buggy
> in a long running toolstack.

What I mean is that they might map the guest pages, and expect to
exchange data with the guest through the pages while they were still
mapped ...

> Also after the initial start of day period we require that the guest
> enable its caches.

... but before the guest enables caching.

Ian.

Re: [PATCH v2 4/4] Revert "xen: arm: force guest memory accesses to cacheable when MMU is disabled"

[Ian Campbell]

On Tue, 2014-02-04 at 15:37 +0000, Julien Grall wrote:
> On 02/04/2014 02:22 PM, Ian Campbell wrote:
> > This reverts commit 89eb02c2204a0b42a0aa169f107bc346a3fef802.
> > 
> > This approach has a short coming in that it breaks when a guest enables its
> > MMU (SCTLR.M, disabling HCR.DC) without enabling caches (SCTLR.C) first/at the
> > same time. It turns out that FreeBSD does this.
> > 
> > This has now been fixed (yet) another way (third time is the charm!) so remove
> > this support. The original commit contained some fixes which are still
> > relevant even with the revert of the bulk of the patch:
> >  - Correction to HSR_SYSREG_CRN_MASK
> >  - Rename of HSR_SYSCTL macros to avoid naming clash
> >  - Definition of some additional cp reg specifications
> > 
> > Since these are still useful they are not reverted.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Except the spurious line toward the end of the patch:

Seems it was spuriously removed by the original, so this was just
reverting that, But I think it is correct to keep it out so I have done
so.

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

Thanks.

> 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index b8f2e82..ec51d1b 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> 
> [..]
> 
> >      default:
> >          printk("%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
> >                 sysreg.read ? "mrs" : "msr",
> > @@ -1635,6 +1477,7 @@ done:
> >      if (first) unmap_domain_page(first);
> >  }
> >  
> > +
> 
> Spurious change?
> 
> >  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> >                                        union hsr hsr)
> 
> 

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Campbell]

On Tue, 2014-02-04 at 15:48 +0000, Jan Beulich wrote:
> >>> On 04.02.14 at 16:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > sync_page is a much better suggestion. Perhaps even
> > sync_page_to/from_cache, I suppose that might actually be more
> > confusing.
> 
> Hmm, on one hand it's even more precise, but otoh - how would a
> sync_page_to_cache() look like?

That suggests moving something into the cache, where this function is
actually pushing something out of the cache and into RAM.

sync_page_to_ram then? I think that is better than _from_cache.

Ian.

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Campbell]

On Tue, 2014-02-04 at 15:55 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> > On Tue, 2014-02-04 at 15:32 +0000, Ian Jackson wrote:
> > > The approach you are taking here is that for pages explicitly mapped
> > > by some libxc caller, you do the flush on unmap.  But what about
> > > callers who don't unmap ?  Are there callers which don't unmap and
> > > which instead are relying on memory coherency assumptions which aren't
> > > true on arm ?
> > 
> > Callers which don't unmap would be leaking mappings and therefore buggy
> > in a long running toolstack.
> 
> What I mean is that they might map the guest pages, and expect to
> exchange data with the guest through the pages while they were still
> mapped ...
> 
> > Also after the initial start of day period we require that the guest
> > enable its caches.
> 
> ... but before the guest enables caching.

We basically rule that out in the ABI requirements.

The cases where this would happen are things like xenstore and
xenconsole but those are driven from the guest end, which requires the
caches to be on.

Ian.

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> On Tue, 2014-02-04 at 15:55 +0000, Ian Jackson wrote:
> > What I mean is that they might map the guest pages, and expect to
> > exchange data with the guest through the pages while they were still
> > mapped ...
> > 
> > > Also after the initial start of day period we require that the guest
> > > enable its caches.
> > 
> > ... but before the guest enables caching.
> 
> We basically rule that out in the ABI requirements.
> 
> The cases where this would happen are things like xenstore and
> xenconsole but those are driven from the guest end, which requires the
> caches to be on.

OK, good.  Thanks for the explanation.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

Re: [PATCH v2 3/4] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Jan Beulich]

>>> On 04.02.14 at 17:07, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-02-04 at 15:48 +0000, Jan Beulich wrote:
>> >>> On 04.02.14 at 16:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > sync_page is a much better suggestion. Perhaps even
>> > sync_page_to/from_cache, I suppose that might actually be more
>> > confusing.
>> 
>> Hmm, on one hand it's even more precise, but otoh - how would a
>> sync_page_to_cache() look like?
> 
> That suggests moving something into the cache,

My problem is that I don't see how one would do this in a way
that _guarantees_ that at the end the data is in cache.

> where this function is
> actually pushing something out of the cache and into RAM.
> 
> sync_page_to_ram then? I think that is better than _from_cache.

Fine with me.

Jan

[PATCH v2 4/4] Revert "xen: arm: force guest memory accesses to cacheable when MMU is disabled"

[Ian Campbell]

This reverts commit 89eb02c2204a0b42a0aa169f107bc346a3fef802.

This approach has a short coming in that it breaks when a guest enables its
MMU (SCTLR.M, disabling HCR.DC) without enabling caches (SCTLR.C) first/at the
same time. It turns out that FreeBSD does this.

This has now been fixed (yet) another way (third time is the charm!) so remove
this support. The original commit contained some fixes which are still
relevant even with the revert of the bulk of the patch:
 - Correction to HSR_SYSREG_CRN_MASK
 - Rename of HSR_SYSCTL macros to avoid naming clash
 - Definition of some additional cp reg specifications

Since these are still useful they are not reverted.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
v2: Move to end of series
    Do not revert useful bits
---
 xen/arch/arm/domain.c        |    7 --
 xen/arch/arm/traps.c         |  158 ------------------------------------------
 xen/include/asm-arm/domain.h |    2 -
 3 files changed, 167 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 635a9a4..124cccf 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -19,7 +19,6 @@
 #include <xen/errno.h>
 #include <xen/bitops.h>
 #include <xen/grant_table.h>
-#include <xen/stdbool.h>
 
 #include <asm/current.h>
 #include <asm/event.h>
@@ -220,11 +219,6 @@ static void ctxt_switch_to(struct vcpu *n)
     else
         hcr |= HCR_RW;
 
-    if ( n->arch.default_cache )
-        hcr |= (HCR_TVM|HCR_DC);
-    else
-        hcr &= ~(HCR_TVM|HCR_DC);
-
     WRITE_SYSREG(hcr, HCR_EL2);
     isb();
 
@@ -475,7 +469,6 @@ int vcpu_initialise(struct vcpu *v)
         return rc;
 
     v->arch.sctlr = SCTLR_GUEST_INIT;
-    v->arch.default_cache = true;
 
     /*
      * By default exposes an SMP system with AFF0 set to the VCPU ID
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b8f2e82..a15b59e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -29,14 +29,12 @@
 #include <xen/hypercall.h>
 #include <xen/softirq.h>
 #include <xen/domain_page.h>
-#include <xen/stdbool.h>
 #include <public/sched.h>
 #include <public/xen.h>
 #include <asm/event.h>
 #include <asm/regs.h>
 #include <asm/cpregs.h>
 #include <asm/psci.h>
-#include <asm/flushtlb.h>
 
 #include "decode.h"
 #include "io.h"
@@ -1292,29 +1290,6 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
     regs->pc += hsr.len ? 4 : 2;
 }
 
-static void update_sctlr(struct vcpu *v, uint32_t val)
-{
-    /*
-     * If MMU (SCTLR_M) is now enabled then we must disable HCR.DC
-     * because they are incompatible.
-     *
-     * Once HCR.DC is disabled then we do not need HCR_TVM either,
-     * since it's only purpose was to catch the MMU being enabled.
-     *
-     * Both are set appropriately on context switch but we need to
-     * clear them now since we may not context switch on return to
-     * guest.
-     */
-    if ( val & SCTLR_M )
-    {
-        WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
-        /* ARM ARM 0406C.b B3.2.1: Disabling HCR.DC without changing
-         * VMID requires us to flush the TLB for that VMID. */
-        flush_tlb();
-        v->arch.default_cache = false;
-    }
-}
-
 static void do_cp15_32(struct cpu_user_regs *regs,
                        union hsr hsr)
 {
@@ -1374,89 +1349,6 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
-
-/* Passthru a 32-bit AArch32 register which is also 32-bit under AArch64 */
-#define CP32_PASSTHRU32(R...) do {              \
-    if ( cp32.read )                            \
-        *r = READ_SYSREG32(R);                  \
-    else                                        \
-        WRITE_SYSREG32(*r, R);                  \
-} while(0)
-
-/*
- * Passthru a 32-bit AArch32 register which is 64-bit under AArch64.
- * Updates the lower 32-bits and clears the upper bits.
- */
-#define CP32_PASSTHRU64(R...) do {              \
-    if ( cp32.read )                            \
-        *r = (uint32_t)READ_SYSREG64(R);        \
-    else                                        \
-        WRITE_SYSREG64((uint64_t)*r, R);        \
-} while(0)
-
-/*
- * Passthru a 32-bit AArch32 register which is 64-bit under AArch64.
- * Updates either the HI ([63:32]) or LO ([31:0]) 32-bits preserving
- * the other half.
- */
-#ifdef CONFIG_ARM_64
-#define CP32_PASSTHRU64_HI(R...) do {                   \
-    if ( cp32.read )                                    \
-        *r = (uint32_t)(READ_SYSREG64(R) >> 32);        \
-    else                                                \
-    {                                                   \
-        uint64_t t = READ_SYSREG64(R) & 0xffffffffUL;   \
-        t |= ((uint64_t)(*r)) << 32;                    \
-        WRITE_SYSREG64(t, R);                           \
-    }                                                   \
-} while(0)
-#define CP32_PASSTHRU64_LO(R...) do {                           \
-    if ( cp32.read )                                            \
-        *r = (uint32_t)(READ_SYSREG64(R) & 0xffffffff);         \
-    else                                                        \
-    {                                                           \
-        uint64_t t = READ_SYSREG64(R) & 0xffffffff00000000UL;   \
-        t |= *r;                                                \
-        WRITE_SYSREG64(t, R);                                   \
-    }                                                           \
-} while(0)
-#endif
-
-    /* HCR.TVM */
-    case HSR_CPREG32(SCTLR):
-        CP32_PASSTHRU32(SCTLR_EL1);
-        update_sctlr(v, *r);
-        break;
-    case HSR_CPREG32(TTBR0_32):   CP32_PASSTHRU64(TTBR0_EL1);      break;
-    case HSR_CPREG32(TTBR1_32):   CP32_PASSTHRU64(TTBR1_EL1);      break;
-    case HSR_CPREG32(TTBCR):      CP32_PASSTHRU32(TCR_EL1);        break;
-    case HSR_CPREG32(DACR):       CP32_PASSTHRU32(DACR32_EL2);     break;
-    case HSR_CPREG32(DFSR):       CP32_PASSTHRU32(ESR_EL1);        break;
-    case HSR_CPREG32(IFSR):       CP32_PASSTHRU32(IFSR32_EL2);     break;
-    case HSR_CPREG32(ADFSR):      CP32_PASSTHRU32(AFSR0_EL1);      break;
-    case HSR_CPREG32(AIFSR):      CP32_PASSTHRU32(AFSR1_EL1);      break;
-    case HSR_CPREG32(CONTEXTIDR): CP32_PASSTHRU32(CONTEXTIDR_EL1); break;
-
-#ifdef CONFIG_ARM_64
-    case HSR_CPREG32(DFAR):       CP32_PASSTHRU64_LO(FAR_EL1);     break;
-    case HSR_CPREG32(IFAR):       CP32_PASSTHRU64_HI(FAR_EL1);     break;
-    case HSR_CPREG32(MAIR0):      CP32_PASSTHRU64_LO(MAIR_EL1);    break;
-    case HSR_CPREG32(MAIR1):      CP32_PASSTHRU64_HI(MAIR_EL1);    break;
-    case HSR_CPREG32(AMAIR0):     CP32_PASSTHRU64_LO(AMAIR_EL1);   break;
-    case HSR_CPREG32(AMAIR1):     CP32_PASSTHRU64_HI(AMAIR_EL1);   break;
-#else
-    case HSR_CPREG32(DFAR):       CP32_PASSTHRU32(DFAR);           break;
-    case HSR_CPREG32(IFAR):       CP32_PASSTHRU32(IFAR);           break;
-    case HSR_CPREG32(MAIR0):      CP32_PASSTHRU32(MAIR0);          break;
-    case HSR_CPREG32(MAIR1):      CP32_PASSTHRU32(MAIR1);          break;
-    case HSR_CPREG32(AMAIR0):     CP32_PASSTHRU32(AMAIR0);         break;
-    case HSR_CPREG32(AMAIR1):     CP32_PASSTHRU32(AMAIR1);         break;
-#endif
-
-#undef CP32_PASSTHRU32
-#undef CP32_PASSTHRU64
-#undef CP32_PASSTHRU64_LO
-#undef CP32_PASSTHRU64_HI
     default:
         printk("%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
                cp32.read ? "mrc" : "mcr",
@@ -1470,9 +1362,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
                        union hsr hsr)
 {
     struct hsr_cp64 cp64 = hsr.cp64;
-    uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1);
-    uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2);
-    uint64_t r;
 
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -1490,26 +1379,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
             domain_crash_synchronous();
         }
         break;
-
-#define CP64_PASSTHRU(R...) do {                                  \
-    if ( cp64.read )                                            \
-    {                                                           \
-        r = READ_SYSREG64(R);                                   \
-        *r1 = r & 0xffffffffUL;                                 \
-        *r2 = r >> 32;                                          \
-    }                                                           \
-    else                                                        \
-    {                                                           \
-        r = (*r1) | (((uint64_t)(*r2))<<32);                    \
-        WRITE_SYSREG64(r, R);                                   \
-    }                                                           \
-} while(0)
-
-    case HSR_CPREG64(TTBR0): CP64_PASSTHRU(TTBR0_EL1); break;
-    case HSR_CPREG64(TTBR1): CP64_PASSTHRU(TTBR1_EL1); break;
-
-#undef CP64_PASSTHRU
-
     default:
         printk("%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
                cp64.read ? "mrrc" : "mcrr",
@@ -1524,8 +1393,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
                       union hsr hsr)
 {
     struct hsr_sysreg sysreg = hsr.sysreg;
-    register_t *x = select_user_reg(regs, sysreg.reg);
-    struct vcpu *v = current;
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
@@ -1538,31 +1405,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
             domain_crash_synchronous();
         }
         break;
-
-#define SYSREG_PASSTHRU(R...) do {              \
-    if ( sysreg.read )                          \
-        *x = READ_SYSREG(R);                    \
-    else                                        \
-        WRITE_SYSREG(*x, R);                    \
-} while(0)
-
-    case HSR_SYSREG_SCTLR_EL1:
-        SYSREG_PASSTHRU(SCTLR_EL1);
-        update_sctlr(v, *x);
-        break;
-    case HSR_SYSREG_TTBR0_EL1:      SYSREG_PASSTHRU(TTBR0_EL1);      break;
-    case HSR_SYSREG_TTBR1_EL1:      SYSREG_PASSTHRU(TTBR1_EL1);      break;
-    case HSR_SYSREG_TCR_EL1:        SYSREG_PASSTHRU(TCR_EL1);        break;
-    case HSR_SYSREG_ESR_EL1:        SYSREG_PASSTHRU(ESR_EL1);        break;
-    case HSR_SYSREG_FAR_EL1:        SYSREG_PASSTHRU(FAR_EL1);        break;
-    case HSR_SYSREG_AFSR0_EL1:      SYSREG_PASSTHRU(AFSR0_EL1);      break;
-    case HSR_SYSREG_AFSR1_EL1:      SYSREG_PASSTHRU(AFSR1_EL1);      break;
-    case HSR_SYSREG_MAIR_EL1:       SYSREG_PASSTHRU(MAIR_EL1);       break;
-    case HSR_SYSREG_AMAIR_EL1:      SYSREG_PASSTHRU(AMAIR_EL1);      break;
-    case HSR_SYSREG_CONTEXTIDR_EL1: SYSREG_PASSTHRU(CONTEXTIDR_EL1); break;
-
-#undef SYSREG_PASSTHRU
-
     default:
         printk("%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
                sysreg.read ? "mrs" : "msr",
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index af8c64b..bc20a15 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -257,8 +257,6 @@ struct arch_vcpu
     uint64_t event_mask;
     uint64_t lr_mask;
 
-    bool_t default_cache;
-
     struct {
         /*
          * SGIs and PPIs are per-VCPU, SPIs are domain global and in
-- 
1.7.10.4