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

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

[Ian Campbell]

Jan/Ian/Keir -- the final patch involves tools changes and a new domctl,
which is why you are copied (although the domctl is marked as arm
specific you might have opinions on it).

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.

This has survived 20000 bootloops on arm32 and 9700 (on going) on arm64.
(I've been seeing a sporadic issue on arm64 but I believe it is
unrelated, although I also cannot reproduce at the moment).

As well as being more correct (and secure!) this strategy is IMHO
simpler than the existing HCR.DC based thing and I'd like to push it to
4.4.

I'm aware that this is the third attempt to get this right and the
second one requiring a freeze exception :-(

Ian.

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

A follow up patch which fix this (yet) another way (third time is the charm!)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain.c           |    7 --
 xen/arch/arm/traps.c            |  163 +--------------------------------------
 xen/arch/arm/vtimer.c           |    6 +-
 xen/include/asm-arm/cpregs.h    |    4 -
 xen/include/asm-arm/domain.h    |    2 -
 xen/include/asm-arm/processor.h |    2 +-
 xen/include/asm-arm/sysregs.h   |   19 +----
 7 files changed, 9 insertions(+), 194 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 ea77cb8..377a1e3 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,13 +1393,11 @@ 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 )
     {
-    case HSR_SYSREG_CNTP_CTL_EL0:
-    case HSR_SYSREG_CNTP_TVAL_EL0:
+    case CNTP_CTL_EL0:
+    case CNTP_TVAL_EL0:
         if ( !vtimer_emulate(regs, hsr) )
         {
             dprintk(XENLOG_ERR,
@@ -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/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index e325f78..433ad55 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -240,18 +240,18 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
-    case HSR_SYSREG_CNTP_CTL_EL0:
+    case CNTP_CTL_EL0:
         vtimer_cntp_ctl(regs, &r, sysreg.read);
         if ( sysreg.read )
             *x = r;
         return 1;
-    case HSR_SYSREG_CNTP_TVAL_EL0:
+    case CNTP_TVAL_EL0:
         vtimer_cntp_tval(regs, &r, sysreg.read);
         if ( sysreg.read )
             *x = r;
         return 1;
 
-    case HSR_SYSREG_CNTPCT_EL0:
+    case HSR_CPREG64(CNTPCT):
         return vtimer_cntpct(regs, x, sysreg.read);
 
     default:
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 508467a..f0f1d53 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -121,8 +121,6 @@
 #define TTBR0           p15,0,c2        /* Translation Table Base Reg. 0 */
 #define TTBR1           p15,1,c2        /* Translation Table Base Reg. 1 */
 #define HTTBR           p15,4,c2        /* Hyp. Translation Table Base Register */
-#define TTBR0_32        p15,0,c2,c0,0   /* 32-bit access to TTBR0 */
-#define TTBR1_32        p15,0,c2,c0,1   /* 32-bit access to TTBR1 */
 #define HTCR            p15,4,c2,c0,2   /* Hyp. Translation Control Register */
 #define VTCR            p15,4,c2,c1,2   /* Virtualization Translation Control Register */
 #define VTTBR           p15,6,c2        /* Virtualization Translation Table Base Register */
@@ -262,9 +260,7 @@
 #define CPACR_EL1               CPACR
 #define CSSELR_EL1              CSSELR
 #define DACR32_EL2              DACR
-#define ESR_EL1                 DFSR
 #define ESR_EL2                 HSR
-#define FAR_EL1                 HIFAR
 #define FAR_EL2                 HIFAR
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
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
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 06e638f..dfe807d 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -342,7 +342,7 @@ union hsr {
 #define HSR_SYSREG_OP0_SHIFT (20)
 #define HSR_SYSREG_OP1_MASK (0x0001c000)
 #define HSR_SYSREG_OP1_SHIFT (14)
-#define HSR_SYSREG_CRN_MASK (0x00003c00)
+#define HSR_SYSREG_CRN_MASK (0x00003800)
 #define HSR_SYSREG_CRN_SHIFT (10)
 #define HSR_SYSREG_CRM_MASK (0x0000001e)
 #define HSR_SYSREG_CRM_SHIFT (1)
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index 0cee0e9..48ad07e 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -40,23 +40,8 @@
     ((__HSR_SYSREG_##crm) << HSR_SYSREG_CRM_SHIFT) | \
     ((__HSR_SYSREG_##op2) << HSR_SYSREG_OP2_SHIFT)
 
-#define HSR_SYSREG_SCTLR_EL1      HSR_SYSREG(3,0,c1, c0,0)
-#define HSR_SYSREG_TTBR0_EL1      HSR_SYSREG(3,0,c2, c0,0)
-#define HSR_SYSREG_TTBR1_EL1      HSR_SYSREG(3,0,c2, c0,1)
-#define HSR_SYSREG_TCR_EL1        HSR_SYSREG(3,0,c2, c0,2)
-#define HSR_SYSREG_AFSR0_EL1      HSR_SYSREG(3,0,c5, c1,0)
-#define HSR_SYSREG_AFSR1_EL1      HSR_SYSREG(3,0,c5, c1,1)
-#define HSR_SYSREG_ESR_EL1        HSR_SYSREG(3,0,c5, c2,0)
-#define HSR_SYSREG_FAR_EL1        HSR_SYSREG(3,0,c6, c0,0)
-#define HSR_SYSREG_MAIR_EL1       HSR_SYSREG(3,0,c10,c2,0)
-#define HSR_SYSREG_AMAIR_EL1      HSR_SYSREG(3,0,c10,c3,0)
-#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
-
-#define HSR_SYSREG_CNTPCT_EL0     HSR_SYSREG(3,3,c14,c0,0)
-#define HSR_SYSREG_CNTP_CTL_EL0   HSR_SYSREG(3,3,c14,c2,1)
-#define HSR_SYSREG_CNTP_TVAL_EL0  HSR_SYSREG(3,3,c14,c2,0)
-
-
+#define CNTP_CTL_EL0  HSR_SYSREG(3,3,c14,c2,1)
+#define CNTP_TVAL_EL0 HSR_SYSREG(3,3,c14,c2,0)
 #endif
 
 #endif
-- 
1.7.10.4

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

We need to clean all caches in order to catch both pages dirtied by the domain
builder and those which have been scrubbed but not yet flushed. Separating the
two and flushing scrubbed pages at scrub time and only builder-dirtied pages
here would require tracking the dirtiness state in the guest's p2m, perhaps
via a new p2m type.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: jbeulich@suse.com
Cc: keir@xen.org
Cc: ian.jackson@eu.citrix.com
---
 tools/libxc/xc_domain.c     |    8 ++++++
 tools/libxc/xenctrl.h       |    1 +
 tools/libxl/libxl_create.c  |    3 ++
 xen/arch/arm/domctl.c       |   12 ++++++++
 xen/arch/arm/p2m.c          |   64 +++++++++++++++++++++++++++++++++++++------
 xen/include/public/domctl.h |    9 ++++++
 6 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index c2fdd74..e6fa4ff 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -48,6 +48,14 @@ int xc_domain_create(xc_interface *xch,
     return 0;
 }
 
+int xc_domain_cacheflush(xc_interface *xch, uint32_t domid)
+{
+    DECLARE_DOMCTL;
+    domctl.cmd = XEN_DOMCTL_cacheflush;
+    domctl.domain = (domid_t)domid;
+    domctl.u.cacheflush.start_mfn = 0;
+    return do_domctl(xch, &domctl);
+}
 
 int xc_domain_pause(xc_interface *xch,
                     uint32_t domid)
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 13f816b..43dae5c 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -453,6 +453,7 @@ 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);
 
 
 /* Functions to produce a dump of a given domain
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a604cd8..55c86f0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1364,7 +1364,10 @@ static void domain_create_cb(libxl__egc *egc,
     STATE_AO_GC(cdcs->dcs.ao);
 
     if (!rc)
+    {
         *cdcs->domid_out = domid;
+        xc_domain_cacheflush(CTX->xch, domid);
+    }
 
     libxl__ao_complete(egc, ao, rc);
 }
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 546e86b..9e3b37d 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -11,12 +11,24 @@
 #include <xen/sched.h>
 #include <xen/hypercall.h>
 #include <public/domctl.h>
+#include <xen/guest_access.h>
+
+extern long p2m_cache_flush(struct domain *d, xen_pfn_t *start_mfn);
 
 long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
                     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
     switch ( domctl->cmd )
     {
+    case XEN_DOMCTL_cacheflush:
+    {
+        long rc = p2m_cache_flush(d, &domctl->u.cacheflush.start_mfn);
+        if ( __copy_to_guest(u_domctl, domctl, 1) )
+            rc = -EFAULT;
+
+        return rc;
+    }
+
     default:
         return subarch_do_domctl(domctl, d, u_domctl);
     }
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a61edeb..18bd500 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -228,15 +228,26 @@ enum p2m_operation {
     ALLOCATE,
     REMOVE,
     RELINQUISH,
+    CACHEFLUSH,
 };
 
+static void do_one_cacheflush(paddr_t mfn)
+{
+    void *v = map_domain_page(mfn);
+
+    flush_xen_dcache_va_range(v, PAGE_SIZE);
+
+    unmap_domain_page(v);
+}
+
 static int apply_p2m_changes(struct domain *d,
                      enum p2m_operation op,
                      paddr_t start_gpaddr,
                      paddr_t end_gpaddr,
                      paddr_t maddr,
                      int mattr,
-                     p2m_type_t t)
+                     p2m_type_t t,
+                     xen_pfn_t *last_mfn)
 {
     int rc;
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -381,18 +392,42 @@ static int apply_p2m_changes(struct domain *d,
                     count++;
                 }
                 break;
+            case CACHEFLUSH:
+                {
+                    if ( !pte.p2m.valid || !p2m_is_ram(pte.p2m.type) )
+                    {
+                        count++;
+                        break;
+                    }
+
+                    count += 0x10;
+
+                    do_one_cacheflush(pte.p2m.base);
+                }
+                break;
         }
 
+        if ( last_mfn )
+            *last_mfn = addr >> PAGE_SHIFT;
+
         /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
-        if ( op == RELINQUISH && count >= 0x2000 )
+        switch ( op )
         {
-            if ( hypercall_preempt_check() )
+        case RELINQUISH:
+        case CACHEFLUSH:
+            if (count >= 0x2000 && hypercall_preempt_check() )
             {
                 p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
                 rc = -EAGAIN;
                 goto out;
             }
             count = 0;
+            break;
+        case INSERT:
+        case ALLOCATE:
+        case REMOVE:
+            /* No preemption */
+            break;
         }
 
         /* Got the next page */
@@ -438,7 +473,7 @@ 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, p2m_ram_rw, NULL);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -447,7 +482,7 @@ int map_mmio_regions(struct domain *d,
                      paddr_t maddr)
 {
     return apply_p2m_changes(d, INSERT, start_gaddr, end_gaddr,
-                             maddr, MATTR_DEV, p2m_mmio_direct);
+                             maddr, MATTR_DEV, p2m_mmio_direct, NULL);
 }
 
 int guest_physmap_add_entry(struct domain *d,
@@ -459,7 +494,7 @@ 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, t, NULL);
 }
 
 void guest_physmap_remove_page(struct domain *d,
@@ -469,7 +504,7 @@ 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, p2m_invalid, NULL);
 }
 
 int p2m_alloc_table(struct domain *d)
@@ -621,7 +656,20 @@ 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, p2m_invalid, NULL);
+}
+
+long p2m_cache_flush(struct domain *d, xen_pfn_t *start_mfn)
+{
+    struct p2m_domain *p2m = &d->arch.p2m;
+
+    *start_mfn = MAX(*start_mfn, p2m->next_gfn_to_relinquish);
+
+    return apply_p2m_changes(d, CACHEFLUSH,
+                             pfn_to_paddr(*start_mfn),
+                             pfn_to_paddr(p2m->max_mapped_gfn),
+                             pfn_to_paddr(INVALID_MFN),
+                             MATTR_MEM, p2m_invalid, start_mfn);
 }
 
 unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 91f01fa..d7b22c3 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -885,6 +885,13 @@ 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);
 
+struct xen_domctl_cacheflush {
+    /* Updated for progress */
+    xen_pfn_t start_mfn;
+};
+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 +961,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 +1020,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];
-- 
1.7.10.4

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

[Jan Beulich]

>>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -48,6 +48,14 @@ int xc_domain_create(xc_interface *xch,
>      return 0;
>  }
>  
> +int xc_domain_cacheflush(xc_interface *xch, uint32_t domid)
> +{
> +    DECLARE_DOMCTL;
> +    domctl.cmd = XEN_DOMCTL_cacheflush;
> +    domctl.domain = (domid_t)domid;

Why can't the function parameter be domid_t right away?

> +    case XEN_DOMCTL_cacheflush:
> +    {
> +        long rc = p2m_cache_flush(d, &domctl->u.cacheflush.start_mfn);
> +        if ( __copy_to_guest(u_domctl, domctl, 1) )

While you certainly say so in the public header change, I think you
recall that we pretty recently changed another hypercall to not be
the only inconsistent one modifying the input structure in order to
handle hypercall preemption.

Further - who's responsible for initiating the resume after a
preemption? p2m_cache_flush() returning -EAGAIN isn't being
handled here, and also not in libxc (which would be awkward
anyway).

> +static void do_one_cacheflush(paddr_t mfn)
> +{
> +    void *v = map_domain_page(mfn);
> +
> +    flush_xen_dcache_va_range(v, PAGE_SIZE);
> +
> +    unmap_domain_page(v);
> +}

Sort of odd that you have to map a page in order to flush cache
(which I very much hope is physically indexed, or else this
operation wouldn't have the intended effect anyway). Can this
not be done based on the machine address?

>          /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
> -        if ( op == RELINQUISH && count >= 0x2000 )
> +        switch ( op )
>          {
> -            if ( hypercall_preempt_check() )
> +        case RELINQUISH:
> +        case CACHEFLUSH:
> +            if (count >= 0x2000 && hypercall_preempt_check() )
>              {
>                  p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
>                  rc = -EAGAIN;
>                  goto out;
>              }
>              count = 0;
> +            break;
> +        case INSERT:
> +        case ALLOCATE:
> +        case REMOVE:
> +            /* No preemption */
> +            break;
>          }

Unrelated to the patch here, but don't you have a problem if you
don't preempt _at all_ here for certain operation types? Or is a
limit on the number of iterations being enforced elsewhere for
those?

Jan

Re: [PATCH 2/4] xen: arm: rename create_p2m_entries to apply_p2m_changes

[Julien Grall]

On 29/01/14 12:11, Ian Campbell wrote:
> 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>

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

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

-- 
Julien Grall

Re: [PATCH 3/4] xen: arm: rename p2m next_gfn_to_relinquish to lowest_mapped_gfn

[Julien Grall]

On 29/01/14 12:11, Ian Campbell wrote:
> 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>
Acked-by: Julien Grall <julien.grall@linaro.org>

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

-- 
Julien Grall

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

[Julien Grall]

On 29/01/14 12:11, 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.

By reverting this patch, you also revert some insteresting fix:
   - Fixing HSR_SYSREG_CRN_MASK
   - Use of HSR_SYSREG_

I think this both changes should be kept.

Otherwise, I would move this patch at the end of the series if we need 
to bisect the code.

-- 
Julien Grall

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

[Julien Grall]

Hi Ian,

Thanks for the patch series.

On 29/01/14 12:10, Ian Campbell wrote:
> Jan/Ian/Keir -- the final patch involves tools changes and a new domctl,
> which is why you are copied (although the domctl is marked as arm
> specific you might have opinions on it).
>
> 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).

Enabling the cache earlier wouldn't change the issue :). The main 
problem is the page table attributes. Using Write-Through (with cached 
enabled/disabled) can randomly crash guest.

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 29 Jan 2014, Jan Beulich wrote:
> > +static void do_one_cacheflush(paddr_t mfn)
> > +{
> > +    void *v = map_domain_page(mfn);
> > +
> > +    flush_xen_dcache_va_range(v, PAGE_SIZE);
> > +
> > +    unmap_domain_page(v);
> > +}
> 
> Sort of odd that you have to map a page in order to flush cache
> (which I very much hope is physically indexed, or else this
> operation wouldn't have the intended effect anyway). Can this
> not be done based on the machine address?

Unfortunately no. I asked for a similar change when Ian sent the RFC
because I was concerned about performances, but it turns out is not
possible. A pity.

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

[Ian Campbell]

On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
> >>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/tools/libxc/xc_domain.c
> > +++ b/tools/libxc/xc_domain.c
> > @@ -48,6 +48,14 @@ int xc_domain_create(xc_interface *xch,
> >      return 0;
> >  }
> >  
> > +int xc_domain_cacheflush(xc_interface *xch, uint32_t domid)
> > +{
> > +    DECLARE_DOMCTL;
> > +    domctl.cmd = XEN_DOMCTL_cacheflush;
> > +    domctl.domain = (domid_t)domid;
> 
> Why can't the function parameter be domid_t right away?

It seemed that the vast majority of the current libxc functions were
using uint32_t for whatever reason.

> 
> > +    case XEN_DOMCTL_cacheflush:
> > +    {
> > +        long rc = p2m_cache_flush(d, &domctl->u.cacheflush.start_mfn);
> > +        if ( __copy_to_guest(u_domctl, domctl, 1) )
> 
> While you certainly say so in the public header change, I think you
> recall that we pretty recently changed another hypercall to not be
> the only inconsistent one modifying the input structure in order to
> handle hypercall preemption.

That was a XNEMEM though IIRC -- is the same requirement also true of
domctl's?

How/where would you recommend saving the progress here?

> 
> Further - who's responsible for initiating the resume after a
> preemption? p2m_cache_flush() returning -EAGAIN isn't being
> handled here, and also not in libxc (which would be awkward
> anyway).

I've once again fallen into the trap of thinking the common domctl code
would do it for me.

> 
> > +static void do_one_cacheflush(paddr_t mfn)
> > +{
> > +    void *v = map_domain_page(mfn);
> > +
> > +    flush_xen_dcache_va_range(v, PAGE_SIZE);
> > +
> > +    unmap_domain_page(v);
> > +}
> 
> Sort of odd that you have to map a page in order to flush cache
> (which I very much hope is physically indexed, or else this
> operation wouldn't have the intended effect anyway). Can this
> not be done based on the machine address?

Sadly not, yes it is very annoying.

Yes, the cache is required to be physically indexed from armv7 onwards.

> 
> >          /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
> > -        if ( op == RELINQUISH && count >= 0x2000 )
> > +        switch ( op )
> >          {
> > -            if ( hypercall_preempt_check() )
> > +        case RELINQUISH:
> > +        case CACHEFLUSH:
> > +            if (count >= 0x2000 && hypercall_preempt_check() )
> >              {
> >                  p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
> >                  rc = -EAGAIN;
> >                  goto out;
> >              }
> >              count = 0;
> > +            break;
> > +        case INSERT:
> > +        case ALLOCATE:
> > +        case REMOVE:
> > +            /* No preemption */
> > +            break;
> >          }
> 
> Unrelated to the patch here, but don't you have a problem if you
> don't preempt _at all_ here for certain operation types? Or is a
> limit on the number of iterations being enforced elsewhere for
> those?

Good question.

The tools/guest accessible paths here are through
guest_physmap_add/remove_page. I think the only paths which are exposed
that pass a non-zero order are XENMEM_populate_physmap and
XENMEM_exchange, bot of which restrict the maximum order.

I don't think those guest_physmap_* are preemptible on x86 either?

It's possible that we should nevertheless handle preemption on those
code paths as well, but I don't think it is critical right now (*or at
least not critical enough to warrant an freeze exception for 4.4).

Ian.

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

[Ian Campbell]

On Wed, 2014-01-29 at 13:22 +0000, Julien Grall wrote:
> 
> On 29/01/14 12:11, 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.
> 
> By reverting this patch, you also revert some insteresting fix:
>    - Fixing HSR_SYSREG_CRN_MASK
>    - Use of HSR_SYSREG_
> 
> I think this both changes should be kept.

Good, point. The issues are not actual problems with the main change
reverted, but we should keep them anyway since they are correct.

> Otherwise, I would move this patch at the end of the series if we need 
> to bisect the code.

Yes, for some reason I had thought this one needed to come first, but I
think I was wrong.

Ian

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

[Jan Beulich]

>>> On 29.01.14 at 15:15, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
>> >>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > +    case XEN_DOMCTL_cacheflush:
>> > +    {
>> > +        long rc = p2m_cache_flush(d, &domctl->u.cacheflush.start_mfn);
>> > +        if ( __copy_to_guest(u_domctl, domctl, 1) )
>> 
>> While you certainly say so in the public header change, I think you
>> recall that we pretty recently changed another hypercall to not be
>> the only inconsistent one modifying the input structure in order to
>> handle hypercall preemption.
> 
> That was a XNEMEM though IIRC -- is the same requirement also true of
> domctl's?

Not necessarily - I was just trying to point out the issue to
avoid needing to fix it later on.

> How/where would you recommend saving the progress here?

Depending on the nature, a per-domain or per-vCPU field that
gets acted upon before issuing any new, similar operation. I.e.
something along the lines of x86's old_guest_table. It's ugly, I
know. But with exposing domctls to semi-trusted guests in
mind, you may use state modifiable by the caller here only if
tampering with that state isn't going to harm the whole system
(if the guest being started is affected in the case here that
obviously wouldn't be a problem).

>> >          /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
>> > -        if ( op == RELINQUISH && count >= 0x2000 )
>> > +        switch ( op )
>> >          {
>> > -            if ( hypercall_preempt_check() )
>> > +        case RELINQUISH:
>> > +        case CACHEFLUSH:
>> > +            if (count >= 0x2000 && hypercall_preempt_check() )
>> >              {
>> >                  p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
>> >                  rc = -EAGAIN;
>> >                  goto out;
>> >              }
>> >              count = 0;
>> > +            break;
>> > +        case INSERT:
>> > +        case ALLOCATE:
>> > +        case REMOVE:
>> > +            /* No preemption */
>> > +            break;
>> >          }
>> 
>> Unrelated to the patch here, but don't you have a problem if you
>> don't preempt _at all_ here for certain operation types? Or is a
>> limit on the number of iterations being enforced elsewhere for
>> those?
> 
> Good question.
> 
> The tools/guest accessible paths here are through
> guest_physmap_add/remove_page. I think the only paths which are exposed
> that pass a non-zero order are XENMEM_populate_physmap and
> XENMEM_exchange, bot of which restrict the maximum order.
> 
> I don't think those guest_physmap_* are preemptible on x86 either?

They aren't, but they have a strict upper limit of at most dealing
with a 1Gb page at a time. If that's similar for ARM, I don't see
an immediate issue.

> It's possible that we should nevertheless handle preemption on those
> code paths as well, but I don't think it is critical right now (*or at
> least not critical enough to warrant an freeze exception for 4.4).

Indeed. Of course the 1Gb limit mentioned above, while perhaps
acceptable to process without preemption right now, is still pretty
high for achieving really good responsiveness, so we may need to
do something about that going forward.

Jan

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

[Ian Campbell]

On Wed, 2014-01-29 at 15:01 +0000, Jan Beulich wrote:
> >>> On 29.01.14 at 15:15, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
> >> >>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > +    case XEN_DOMCTL_cacheflush:
> >> > +    {
> >> > +        long rc = p2m_cache_flush(d, &domctl->u.cacheflush.start_mfn);
> >> > +        if ( __copy_to_guest(u_domctl, domctl, 1) )
> >> 
> >> While you certainly say so in the public header change, I think you
> >> recall that we pretty recently changed another hypercall to not be
> >> the only inconsistent one modifying the input structure in order to
> >> handle hypercall preemption.
> > 
> > That was a XNEMEM though IIRC -- is the same requirement also true of
> > domctl's?
> 
> Not necessarily - I was just trying to point out the issue to
> avoid needing to fix it later on.

OK, but you do think it should be fixed "transparently" rather than made
an explicit part of the API?

> > How/where would you recommend saving the progress here?
> 
> Depending on the nature, a per-domain or per-vCPU field that
> gets acted upon before issuing any new, similar operation. I.e.
> something along the lines of x86's old_guest_table. It's ugly, I
> know. But with exposing domctls to semi-trusted guests in
> mind, you may use state modifiable by the caller here only if
> tampering with that state isn't going to harm the whole system
> (if the guest being started is affected in the case here that
> obviously wouldn't be a problem).

Hrm, thanks for raising this -- it made me realise that we cannot
necessarily rely on the disaggregated domain builder to even issue this
call at all.

That would be OK from the point of view of not flushing the things which
the builder touched (as you say, it can only harm the domain it is
building). But it is not ok from the PoV of flushing scrubbed data from
the cache, ensuring that the scrubbed bytes reach RAM (i.e. it can leak
old data).

So I think I need an approach which flushes the scrubbed pages as it
does the scrubbing (this makes a certain logical sense anyway) and have
the toolstack issue hypercalls to flush the stuff it has written. (the
first approach to this issue tried to do this but used a system call
provided by Linux which didn't have the quite correct semantics, but
using a version of this hypercall with a range should work).

Before I get too deep into that, do you think that
        struct xen_domctl_cacheflush {
            /* start_mfn is updated for progress over preemption. */
            xen_pfn_t start_mfn;
            xen_pfn_t end_mfn;
        };
        
is acceptable or do you want me to try and find a way to do preemption
without the write back?

The blobs written by the toolstack aren't likely to be >1GB in size, so
rejecting over large ranges would be a potential option, but it's not
totally satisfactory.

> >> >          /* Preempt every 2MiB (mapped) or 32 MiB (unmapped) - arbitrary */
> >> > -        if ( op == RELINQUISH && count >= 0x2000 )
> >> > +        switch ( op )
> >> >          {
> >> > -            if ( hypercall_preempt_check() )
> >> > +        case RELINQUISH:
> >> > +        case CACHEFLUSH:
> >> > +            if (count >= 0x2000 && hypercall_preempt_check() )
> >> >              {
> >> >                  p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT;
> >> >                  rc = -EAGAIN;
> >> >                  goto out;
> >> >              }
> >> >              count = 0;
> >> > +            break;
> >> > +        case INSERT:
> >> > +        case ALLOCATE:
> >> > +        case REMOVE:
> >> > +            /* No preemption */
> >> > +            break;
> >> >          }
> >> 
> >> Unrelated to the patch here, but don't you have a problem if you
> >> don't preempt _at all_ here for certain operation types? Or is a
> >> limit on the number of iterations being enforced elsewhere for
> >> those?
> > 
> > Good question.
> > 
> > The tools/guest accessible paths here are through
> > guest_physmap_add/remove_page. I think the only paths which are exposed
> > that pass a non-zero order are XENMEM_populate_physmap and
> > XENMEM_exchange, bot of which restrict the maximum order.
> > 
> > I don't think those guest_physmap_* are preemptible on x86 either?
> 
> They aren't, but they have a strict upper limit of at most dealing
> with a 1Gb page at a time. If that's similar for ARM, I don't see
> an immediate issue.

Same on ARM (through common code using MAX_ORDER == 20)

> > It's possible that we should nevertheless handle preemption on those
> > code paths as well, but I don't think it is critical right now (*or at
> > least not critical enough to warrant an freeze exception for 4.4).
> 
> Indeed. Of course the 1Gb limit mentioned above, while perhaps
> acceptable to process without preemption right now, is still pretty
> high for achieving really good responsiveness, so we may need to
> do something about that going forward.

Right. I won't worry about it now though.

> 
> Jan
> 

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

[Jan Beulich]

>>> On 29.01.14 at 17:35, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-01-29 at 15:01 +0000, Jan Beulich wrote:
>> >>> On 29.01.14 at 15:15, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
>> >> >>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
>> >> > +    case XEN_DOMCTL_cacheflush:
>> >> > +    {
>> >> > +        long rc = p2m_cache_flush(d, &domctl->u.cacheflush.start_mfn);
>> >> > +        if ( __copy_to_guest(u_domctl, domctl, 1) )
>> >> 
>> >> While you certainly say so in the public header change, I think you
>> >> recall that we pretty recently changed another hypercall to not be
>> >> the only inconsistent one modifying the input structure in order to
>> >> handle hypercall preemption.
>> > 
>> > That was a XNEMEM though IIRC -- is the same requirement also true of
>> > domctl's?
>> 
>> Not necessarily - I was just trying to point out the issue to
>> avoid needing to fix it later on.
> 
> OK, but you do think it should be fixed "transparently" rather than made
> an explicit part of the API?

I'd prefer if it would be done that way. Otherwise we'd need to
mentally store that we're allowing exceptions for domctls, but not
for other hypercalls.

> Before I get too deep into that, do you think that
>         struct xen_domctl_cacheflush {
>             /* start_mfn is updated for progress over preemption. */
>             xen_pfn_t start_mfn;
>             xen_pfn_t end_mfn;
>         };
>         
> is acceptable or do you want me to try and find a way to do preemption
> without the write back?

As per above - if possible, I'd prefer you avoiding the write back.

> The blobs written by the toolstack aren't likely to be >1GB in size, so
> rejecting over large ranges would be a potential option, but it's not
> totally satisfactory.

Agreed, but good enough for now (considering the various other
cases that already have similar "issues").

Jan

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 4/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
>>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
> > > +int xc_domain_cacheflush(xc_interface *xch, uint32_t domid)
> > > +    domctl.domain = (domid_t)domid;
> > 
> > Why can't the function parameter be domid_t right away?
> 
> It seemed that the vast majority of the current libxc functions were
> using uint32_t for whatever reason.

What's the point of the cast, though ?

Ian.

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

[Ian Campbell]

On Thu, 2014-01-30 at 11:26 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 4/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> > On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
> >>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
> > > > +int xc_domain_cacheflush(xc_interface *xch, uint32_t domid)
> > > > +    domctl.domain = (domid_t)domid;
> > > 
> > > Why can't the function parameter be domid_t right away?
> > 
> > It seemed that the vast majority of the current libxc functions were
> > using uint32_t for whatever reason.
> 
> What's the point of the cast, though ?

Apparently all the cool kids in this file are doing it and I followed
suite ;-)

domid_t is a uint16_t, I kind of expect gcc to warn about an assignment
of a uint32_t to a uint16_t to generate some sort of warning, but
apparently not...

Ian.

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

[Ian Campbell]

On Thu, 2014-01-30 at 12:24 +0000, Ian Campbell wrote:
> On Thu, 2014-01-30 at 11:26 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH 4/4] xen/arm: clean and invalidate all guest caches by VMID after domain build."):
> > > On Wed, 2014-01-29 at 13:00 +0000, Jan Beulich wrote:
> > >>> On 29.01.14 at 13:11, Ian Campbell <ian.campbell@citrix.com> wrote:
> > > > > +int xc_domain_cacheflush(xc_interface *xch, uint32_t domid)
> > > > > +    domctl.domain = (domid_t)domid;
> > > > 
> > > > Why can't the function parameter be domid_t right away?
> > > 
> > > It seemed that the vast majority of the current libxc functions were
> > > using uint32_t for whatever reason.
> > 
> > What's the point of the cast, though ?
> 
> Apparently all the cool kids in this file are doing it and I followed
> suite ;-)
> 
> domid_t is a uint16_t, I kind of expect gcc to warn about an assignment
> of a uint32_t to a uint16_t to generate some sort of warning, but
> apparently not...

Just for completeness: -Wconversion is the option to make it do this.