[Xen-devel,1/4] Revert "xen: arm: force guest memory accesses to cacheable when MMU is disabled"

Message ID 1390997486-3986-1-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell Jan. 29, 2014, 12:11 p.m.
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(-)

Comments

Julien Grall Jan. 29, 2014, 1:22 p.m. | #1
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.
Ian Campbell Jan. 29, 2014, 2:18 p.m. | #2
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

Patch

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