* [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
@ 2014-01-07 16:02 Ian Campbell
2014-01-07 16:06 ` Ian Campbell
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ian Campbell @ 2014-01-07 16:02 UTC (permalink / raw)
To: xen-devel
Cc: Ian Campbell, julien.grall, tim, george.dunlap,
stefano.stabellini
On ARM guest OSes are started with MMU and Caches disables (as they are on
native) however caching is enabled in the domain running the builder and
therefore we must ensure cache consistency.
The existing solution to this problem (a0035ecc0d82 "tools: libxc: flush data
cache after loading images into guest memory") is to flush the caches after
loading the various blobs into guest RAM. However this approach has two short
comings:
- The cache flush primitives available to userspace on arm32 are not
sufficient for our needs.
- There is a race between the cache flush and the unmap of the guest page
where the processor might speculatively dirty the cache line again.
(of these the second is the more fundamental)
This patch makes use of the the hardware functionality to force all accesses
made from guest mode to be cached (the HCR.DC == default cached bit). This
means that we don't need to worry about the domain builder's writes being
cached because the guests "uncached" accesses will actually be cached.
Unfortunately the use of HCR.DC is incompatible with the guest enabling its
MMU (SCTLR.M bit). Therefore we must trap accesses to the SCTLR so that we can
detect when this happens and disable HCR.DC. This is done with the HCR.TVM
(trap virtual memory controls) bit which also causes various other registers
to be trapped, all of which can be passed straight through to the underlying
register. Once the guest has enabled its MMU we no longer need to trap so
there is no ongoing overhead. In my tests Linux makes about half a dozen
accesses to these registers before the MMU is enabled, I would expect other
OSes to behave similarly (the sequence of writes needed to setup the MMU is
pretty obvious).
Apart from this unfortunate need to trap these accesses this approach is
incompatible with guests which attempt to do DMA operations with their MMU
disabled. In practice this means guests with passthrough which we do not yet
support. Since a typical guest (including dom0) does not access devices which
require DMA until after it is fully up and running with paging enabled the
main risk is to in-guest firmware which does DMA i.e. running EFI in a guest,
with a disk passed through and booting from that disk. Since we know that dom0
is not using any such firmware and we do not support device passthrough to
guests yet we can live with this restriction. Once passthrough is implemented
this will need to be revisited.
The patch includes a couple of seemingly unrelated but necessary changes:
- HSR_SYSREG_CRN_MASK was incorrectly defined, which happened to be benign
with the existing set of system register we handled, but broke with the new
ones introduced here.
- The defines used to decode the HSR system register fields were named the
same as the register. This breaks the accessor macros. This had gone
unnoticed because the handling of the existing trapped registers did not
require accessing the underlying hardware register. Rename those constants
with an HSR_SYSREG prefix (in line with HSR_CP32/64 for 32-bit registers).
This patch has survived thousands of boot loops on a Midway system.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
My preferred solution here would be for the tools to use an uncached mapping
of guest memory when building the guest, which requires adding a new privmcd
ioctl (a relatively straightforward patch) and plumbing a "cached" flag
through the libxc foreign mapping interfaces (a twisty maze of passage, all
alike). IMO the libxc side of this patch was not looking suitable for a 4.4
freeze exception, since it was quite large (because we have 4 or more mapping
interfaces in libxc, some of which call back into others).
So I propose this version for 4.4. The uncached mapping solution should be
revisited for a future release.
At the risk of appearing to be going mad:
<speaking hat="submitter">
This bug results in memory corruption bugs in the guest, which mostly manifest
as a failure to boot the guest (subsequent bad behaviour is possible but, I
think, unlikely) the frequency of failures is perhaps 1/10 times. This would
not constitute an awesome release.
Although the patch is large most of it is repetative and mechanical (made
explicit through the use of macros in many cases). The biggest risk is that
one of the registers is not passed through correctly (i.e. the wrong size or
target registers). The ones which Linux uses have been tested and appear to
function OK. The others might be buggy but this is mitigated through the use
of the same set of macros.
I think the chance of the patch having a bug wrt my understanding of the
hardware behaviour is pretty low. WRT there being bugs in my understanding of
the hardware documentation, I would say middle to low, but I have discussed it
with some folks at ARM and they didn't call me an idiot (in fact pretty much
the same thing has been proposed for KVM).
Overall I think the benefits outweigh the risks.
One thing I'm not sure about is reverting the previous fix in a0035ecc0d82.
It's reasonably recent so reverting it takes us back to a pretty well
understood state in the libraries. The functionality is harmless if
incomplete. I think given the first argument I would lean towards reverting.
</speaking>
<speaking hat="stand in release manager">
OK.
</speaking>
Obviously if you think I'm being to easy on myself please say so.
Actually, if you think my judgement is right I'd appreciate being told so too.
---
xen/arch/arm/domain.c | 5 ++
xen/arch/arm/domain_build.c | 1 +
xen/arch/arm/traps.c | 153 ++++++++++++++++++++++++++++++++++++++-
xen/arch/arm/vtimer.c | 6 +-
xen/include/asm-arm/cpregs.h | 4 +
xen/include/asm-arm/processor.h | 2 +-
xen/include/asm-arm/sysregs.h | 19 ++++-
7 files changed, 182 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 124cccf..104d228 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
else
hcr |= HCR_RW;
+ if ( n->arch.sctlr & SCTLR_M )
+ hcr &= ~(HCR_TVM|HCR_DC);
+ else
+ hcr |= (HCR_TVM|HCR_DC);
+
WRITE_SYSREG(hcr, HCR_EL2);
isb();
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 47b781b..bb31db8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1026,6 +1026,7 @@ int construct_dom0(struct domain *d)
else
WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
#endif
+ WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_DC | HCR_TVM, HCR_EL2);
/*
* kernel_load will determine the placement of the initrd & fdt in
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7c5ab19..d00bba3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1279,6 +1279,23 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
regs->pc += hsr.len ? 4 : 2;
}
+static void update_sctlr(uint32_t v)
+{
+ /*
+ * 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 ( v & SCTLR_M )
+ WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
+}
+
static void do_cp15_32(struct cpu_user_regs *regs,
union hsr hsr)
{
@@ -1338,6 +1355,89 @@ 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(*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",
@@ -1351,6 +1451,9 @@ 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) )
{
@@ -1368,6 +1471,26 @@ 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",
@@ -1382,11 +1505,12 @@ 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);
switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
{
- case CNTP_CTL_EL0:
- case CNTP_TVAL_EL0:
+ case HSR_SYSREG_CNTP_CTL_EL0:
+ case HSR_SYSREG_CNTP_TVAL_EL0:
if ( !vtimer_emulate(regs, hsr) )
{
dprintk(XENLOG_ERR,
@@ -1394,6 +1518,31 @@ 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(*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/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 433ad55..e325f78 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 CNTP_CTL_EL0:
+ case HSR_SYSREG_CNTP_CTL_EL0:
vtimer_cntp_ctl(regs, &r, sysreg.read);
if ( sysreg.read )
*x = r;
return 1;
- case CNTP_TVAL_EL0:
+ case HSR_SYSREG_CNTP_TVAL_EL0:
vtimer_cntp_tval(regs, &r, sysreg.read);
if ( sysreg.read )
*x = r;
return 1;
- case HSR_CPREG64(CNTPCT):
+ case HSR_SYSREG_CNTPCT_EL0:
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 f0f1d53..508467a 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -121,6 +121,8 @@
#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 */
@@ -260,7 +262,9 @@
#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/processor.h b/xen/include/asm-arm/processor.h
index dfe807d..06e638f 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 (0x00003800)
+#define HSR_SYSREG_CRN_MASK (0x00003c00)
#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 48ad07e..0cee0e9 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -40,8 +40,23 @@
((__HSR_SYSREG_##crm) << HSR_SYSREG_CRM_SHIFT) | \
((__HSR_SYSREG_##op2) << HSR_SYSREG_OP2_SHIFT)
-#define CNTP_CTL_EL0 HSR_SYSREG(3,3,c14,c2,1)
-#define CNTP_TVAL_EL0 HSR_SYSREG(3,3,c14,c2,0)
+#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)
+
+
#endif
#endif
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-07 16:02 [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled Ian Campbell
@ 2014-01-07 16:06 ` Ian Campbell
2014-01-07 16:59 ` Julien Grall
2014-01-07 20:39 ` Stefano Stabellini
2 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2014-01-07 16:06 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, tim, george.dunlap, stefano.stabellini
On Tue, 2014-01-07 at 16:02 +0000, Ian Campbell wrote:
> One thing I'm not sure about is reverting the previous fix in
> a0035ecc0d82.
> It's reasonably recent so reverting it takes us back to a pretty well
> understood state in the libraries. The functionality is harmless if
> incomplete. I think given the first argument I would lean towards
> reverting.
If the answer here is "revert" then that would be the following. I'd
probably rewrite "A previous patch" into a reference to the actual
applied patch as I committed it.
-------
>From b33eb6a3d5082d3497b8ef0872c0bbd89fb74af1 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 7 Jan 2014 15:52:29 +0000
Subject: [PATCH] Revert "tools: libxc: flush data cache after loading images
into guest memory"
This reverts commit a0035ecc0d82c1d4dcd5e429e2fcc3192d89747a.
Even with this fix there is a period between the flush and the unmap where
processor may speculate data into the cache. The solution is to map this
region uncached or to use the HCR.DC bit to mark all guest accesses cached. A
previous patch has arranged to do the latter.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxc/xc_dom_armzimageloader.c | 1 -
tools/libxc/xc_dom_binloader.c | 1 -
tools/libxc/xc_dom_core.c | 2 --
tools/libxc/xc_linux_osdep.c | 41 ----------------------------------
tools/libxc/xc_minios.c | 12 ----------
tools/libxc/xc_netbsd.c | 14 ------------
tools/libxc/xc_private.c | 5 -----
tools/libxc/xc_private.h | 3 ---
tools/libxc/xc_solaris.c | 14 ------------
tools/libxc/xenctrl_osdep_ENOSYS.c | 9 --------
tools/libxc/xenctrlosdep.h | 1 -
11 files changed, 103 deletions(-)
diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c
index 508f74b..e6516a1 100644
--- a/tools/libxc/xc_dom_armzimageloader.c
+++ b/tools/libxc/xc_dom_armzimageloader.c
@@ -229,7 +229,6 @@ static int xc_dom_load_zimage_kernel(struct xc_dom_image *dom)
__func__, dom->kernel_size, dom->kernel_blob, dst);
memcpy(dst, dom->kernel_blob, dom->kernel_size);
- xc_cache_flush(dom->xch, dst, dom->kernel_size);
return 0;
}
diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
index aa0463c..e1de5b5 100644
--- a/tools/libxc/xc_dom_binloader.c
+++ b/tools/libxc/xc_dom_binloader.c
@@ -301,7 +301,6 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom)
memcpy(dest, image + skip, text_size);
memset(dest + text_size, 0, bss_size);
- xc_cache_flush(dom->xch, dest, text_size+bss_size);
return 0;
}
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index d46ac22..77a4e64 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -978,7 +978,6 @@ int xc_dom_build_image(struct xc_dom_image *dom)
}
else
memcpy(ramdiskmap, dom->ramdisk_blob, dom->ramdisk_size);
- xc_cache_flush(dom->xch, ramdiskmap, ramdisklen);
}
/* load devicetree */
@@ -998,7 +997,6 @@ int xc_dom_build_image(struct xc_dom_image *dom)
goto err;
}
memcpy(devicetreemap, dom->devicetree_blob, dom->devicetree_size);
- xc_cache_flush(dom->xch, devicetreemap, dom->devicetree_size);
}
/* allocate other pages */
diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index e219b24..73860a2 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -30,7 +30,6 @@
#include <sys/mman.h>
#include <sys/ioctl.h>
-#include <sys/syscall.h>
#include <xen/memory.h>
#include <xen/sys/evtchn.h>
@@ -417,44 +416,6 @@ static void *linux_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handle
return ret;
}
-static void linux_privcmd_cache_flush(xc_interface *xch,
- const void *ptr, size_t nr)
-{
-#if defined(__arm__)
- unsigned long start = (unsigned long)ptr;
- unsigned long end = start + nr;
- /* cacheflush(unsigned long start, unsigned long end, int flags) */
- int rc = syscall(__ARM_NR_cacheflush, start, end, 0);
- if ( rc < 0 )
- PERROR("cache flush operation failed: %d\n", errno);
-#elif defined(__aarch64__)
- unsigned long start = (unsigned long)ptr;
- unsigned long end = start + nr;
- unsigned long p, ctr;
- int stride;
-
- /* Flush cache using direct DC CVAC instructions. This is
- * available to EL0 when SCTLR_EL1.UCI is set, which Linux does.
- *
- * Bits 19:16 of CTR_EL0 are log2 of the minimum dcache line size
- * in words, which we use as our stride length. This is readable
- * with SCTLR_EL1.UCT is set, which Linux does.
- */
- asm volatile ("mrs %0, ctr_el0" : "=r" (ctr));
-
- stride = 4 * (1 << ((ctr & 0xf0000UL) >> 16));
-
- for ( p = start ; p < end ; p += stride )
- asm volatile ("dc cvac, %0" : : "r" (p));
- asm volatile ("dsb sy");
-#elif defined(__i386__) || defined(__x86_64__)
- /* No need for cache maintenance on x86 */
-#else
- PERROR("No cache flush operation defined for architecture");
- abort();
-#endif
-}
-
static struct xc_osdep_ops linux_privcmd_ops = {
.open = &linux_privcmd_open,
.close = &linux_privcmd_close,
@@ -469,8 +430,6 @@ static struct xc_osdep_ops linux_privcmd_ops = {
.map_foreign_bulk = &linux_privcmd_map_foreign_bulk,
.map_foreign_range = &linux_privcmd_map_foreign_range,
.map_foreign_ranges = &linux_privcmd_map_foreign_ranges,
-
- .cache_flush = &linux_privcmd_cache_flush,
},
};
diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
index 5026b2b..dec4d73 100644
--- a/tools/libxc/xc_minios.c
+++ b/tools/libxc/xc_minios.c
@@ -181,16 +181,6 @@ static void *minios_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handl
return ret;
}
-static void minios_privcmd_cache_flush(xc_interface *xch,
- const void *ptr, size_t nr)
-{
-#if defined(__i386__) || defined(__x86_64__)
- /* No need for cache maintenance on x86 */
-#else
- printf("No cache flush operation defined for architecture");
- BUG();
-#endif
-}
static struct xc_osdep_ops minios_privcmd_ops = {
.open = &minios_privcmd_open,
@@ -206,8 +196,6 @@ static struct xc_osdep_ops minios_privcmd_ops = {
.map_foreign_bulk = &minios_privcmd_map_foreign_bulk,
.map_foreign_range = &minios_privcmd_map_foreign_range,
.map_foreign_ranges = &minios_privcmd_map_foreign_ranges,
-
- .cache_flush = &minios_privcmd_cache_flush,
},
};
diff --git a/tools/libxc/xc_netbsd.c b/tools/libxc/xc_netbsd.c
index 0143305..8a90ef3 100644
--- a/tools/libxc/xc_netbsd.c
+++ b/tools/libxc/xc_netbsd.c
@@ -22,7 +22,6 @@
#include <xen/sys/evtchn.h>
#include <unistd.h>
-#include <stdlib.h>
#include <fcntl.h>
#include <malloc.h>
#include <sys/mman.h>
@@ -208,17 +207,6 @@ mmap_failed:
return NULL;
}
-static void netbsd_privcmd_cache_flush(xc_interface *xch,
- const void *ptr, size_t nr)
-{
-#if defined(__i386__) || defined(__x86_64__)
- /* No need for cache maintenance on x86 */
-#else
- PERROR("No cache flush operation defined for architecture");
- abort();
-#endif
-}
-
static struct xc_osdep_ops netbsd_privcmd_ops = {
.open = &netbsd_privcmd_open,
.close = &netbsd_privcmd_close,
@@ -233,8 +221,6 @@ static struct xc_osdep_ops netbsd_privcmd_ops = {
.map_foreign_bulk = &xc_map_foreign_bulk_compat,
.map_foreign_range = &netbsd_privcmd_map_foreign_range,
.map_foreign_ranges = &netbsd_privcmd_map_foreign_ranges,
-
- .cache_flush = &netbsd_privcmd_cache_flush,
},
};
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 3ccee2b..838fd21 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -249,11 +249,6 @@ int do_xen_hypercall(xc_interface *xch, privcmd_hypercall_t *hypercall)
return xch->ops->u.privcmd.hypercall(xch, xch->ops_handle, hypercall);
}
-void xc_cache_flush(xc_interface *xch, const void *p, size_t n)
-{
- xch->ops->u.privcmd.cache_flush(xch, p, n);
-}
-
xc_evtchn *xc_evtchn_open(xentoollog_logger *logger,
unsigned open_flags)
{
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 50a0aa7..92271c9 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -304,9 +304,6 @@ void bitmap_byte_to_64(uint64_t *lp, const uint8_t *bp, int nbits);
/* Optionally flush file to disk and discard page cache */
void discard_file_cache(xc_interface *xch, int fd, int flush);
-/* Flush data cache */
-void xc_cache_flush(xc_interface *xch, const void *p, size_t n);
-
#define MAX_MMU_UPDATES 1024
struct xc_mmu {
mmu_update_t updates[MAX_MMU_UPDATES];
diff --git a/tools/libxc/xc_solaris.c b/tools/libxc/xc_solaris.c
index edffab1..7257a54 100644
--- a/tools/libxc/xc_solaris.c
+++ b/tools/libxc/xc_solaris.c
@@ -23,7 +23,6 @@
#include <xen/memory.h>
#include <xen/sys/evtchn.h>
#include <unistd.h>
-#include <stdlib.h>
#include <fcntl.h>
#include <malloc.h>
@@ -179,17 +178,6 @@ mmap_failed:
return NULL;
}
-static void solaris_privcmd_cache_flush(xc_interface *xch,
- const void *ptr, size_t nr)
-{
-#if defined(__i386__) || defined(__x86_64__)
- /* No need for cache maintenance on x86 */
-#else
- PERROR("No cache flush operation defined for architecture");
- abort();
-#endif
-}
-
static struct xc_osdep_ops solaris_privcmd_ops = {
.open = &solaris_privcmd_open,
.close = &solaris_privcmd_close,
@@ -204,8 +192,6 @@ static struct xc_osdep_ops solaris_privcmd_ops = {
.map_foreign_bulk = &xc_map_foreign_bulk_compat,
.map_foreign_range = &solaris_privcmd_map_foreign_range,
.map_foreign_ranges = &solaris_privcmd_map_foreign_ranges,
-
- .cache_flush = &solaris_privcmd_cache_flush,
},
};
diff --git a/tools/libxc/xenctrl_osdep_ENOSYS.c b/tools/libxc/xenctrl_osdep_ENOSYS.c
index d911b10..4821342 100644
--- a/tools/libxc/xenctrl_osdep_ENOSYS.c
+++ b/tools/libxc/xenctrl_osdep_ENOSYS.c
@@ -63,13 +63,6 @@ static void *ENOSYS_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handl
return MAP_FAILED;
}
-static void ENOSYS_privcmd_cache_flush(xc_interface *xch, const void *p, size_t n)
-{
- unsigned long start = (unsigned long)p;
- unsigned long end = start + n;
- IPRINTF(xch, "ENOSYS_privcmd: cache_flush: %#lx-%#lx\n", start, end);
-}
-
static struct xc_osdep_ops ENOSYS_privcmd_ops =
{
.open = &ENOSYS_privcmd_open,
@@ -81,8 +74,6 @@ static struct xc_osdep_ops ENOSYS_privcmd_ops =
.map_foreign_bulk = &ENOSYS_privcmd_map_foreign_bulk,
.map_foreign_range = &ENOSYS_privcmd_map_foreign_range,
.map_foreign_ranges = &ENOSYS_privcmd_map_foreign_ranges,
-
- .cache_flush = &ENOSYS_privcmd_cache_flush,
}
};
diff --git a/tools/libxc/xenctrlosdep.h b/tools/libxc/xenctrlosdep.h
index 6c9a005..e610a24 100644
--- a/tools/libxc/xenctrlosdep.h
+++ b/tools/libxc/xenctrlosdep.h
@@ -89,7 +89,6 @@ struct xc_osdep_ops
void *(*map_foreign_ranges)(xc_interface *xch, xc_osdep_handle h, uint32_t dom, size_t size, int prot,
size_t chunksize, privcmd_mmap_entry_t entries[],
int nentries);
- void (*cache_flush)(xc_interface *xch, const void *p, size_t n);
} privcmd;
struct {
int (*fd)(xc_evtchn *xce, xc_osdep_handle h);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-07 16:02 [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled Ian Campbell
2014-01-07 16:06 ` Ian Campbell
@ 2014-01-07 16:59 ` Julien Grall
2014-01-08 10:02 ` Ian Campbell
2014-01-07 20:39 ` Stefano Stabellini
2 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2014-01-07 16:59 UTC (permalink / raw)
To: Ian Campbell; +Cc: stefano.stabellini, tim, george.dunlap, xen-devel
On 01/07/2014 04:02 PM, Ian Campbell wrote:
.>
> One thing I'm not sure about is reverting the previous fix in a0035ecc0d82.
> It's reasonably recent so reverting it takes us back to a pretty well
> understood state in the libraries. The functionality is harmless if
> incomplete. I think given the first argument I would lean towards reverting.
I would also prefer reverting the previous patch.
>
> Obviously if you think I'm being to easy on myself please say so.
Without this patch, we will likely crash a guest in production, that
it's not acceptable for a release.
>
> Actually, if you think my judgement is right I'd appreciate being told so too.
> ---
> xen/arch/arm/domain.c | 5 ++
> xen/arch/arm/domain_build.c | 1 +
> xen/arch/arm/traps.c | 153 ++++++++++++++++++++++++++++++++++++++-
> xen/arch/arm/vtimer.c | 6 +-
> xen/include/asm-arm/cpregs.h | 4 +
> xen/include/asm-arm/processor.h | 2 +-
> xen/include/asm-arm/sysregs.h | 19 ++++-
> 7 files changed, 182 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 124cccf..104d228 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> else
> hcr |= HCR_RW;
>
> + if ( n->arch.sctlr & SCTLR_M )
> + hcr &= ~(HCR_TVM|HCR_DC);
> + else
> + hcr |= (HCR_TVM|HCR_DC);
> +
> WRITE_SYSREG(hcr, HCR_EL2);
> isb();
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 47b781b..bb31db8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1026,6 +1026,7 @@ int construct_dom0(struct domain *d)
> else
> WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
> #endif
> + WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_DC | HCR_TVM, HCR_EL2);
Is it useful? As I understand, we will at least context switch one time
before booting dom0.
If we need it, perhaps the better place to setup it is init_traps?
>
> /*
> * kernel_load will determine the placement of the initrd & fdt in
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 7c5ab19..d00bba3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1279,6 +1279,23 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
> regs->pc += hsr.len ? 4 : 2;
> }
>
> +static void update_sctlr(uint32_t v)
> +{
> + /*
> + * 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 ( v & SCTLR_M )
> + WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
Even if it's unlikely, can we handle the case where the guest disable
the case?
Also from ARM ARM B3.2.1, a TLB flush by VMID is required if HCR_DC is
disabled and the VMID is not changed.
--
Julien Grall
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-07 16:02 [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled Ian Campbell
2014-01-07 16:06 ` Ian Campbell
2014-01-07 16:59 ` Julien Grall
@ 2014-01-07 20:39 ` Stefano Stabellini
2014-01-08 10:04 ` Ian Campbell
2 siblings, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2014-01-07 20:39 UTC (permalink / raw)
To: Ian Campbell
Cc: stefano.stabellini, tim, george.dunlap, julien.grall, xen-devel
On Tue, 7 Jan 2014, Ian Campbell wrote:
> On ARM guest OSes are started with MMU and Caches disables (as they are on
> native) however caching is enabled in the domain running the builder and
> therefore we must ensure cache consistency.
>
> The existing solution to this problem (a0035ecc0d82 "tools: libxc: flush data
> cache after loading images into guest memory") is to flush the caches after
> loading the various blobs into guest RAM. However this approach has two short
> comings:
>
> - The cache flush primitives available to userspace on arm32 are not
> sufficient for our needs.
> - There is a race between the cache flush and the unmap of the guest page
> where the processor might speculatively dirty the cache line again.
>
> (of these the second is the more fundamental)
>
> This patch makes use of the the hardware functionality to force all accesses
> made from guest mode to be cached (the HCR.DC == default cached bit). This
> means that we don't need to worry about the domain builder's writes being
> cached because the guests "uncached" accesses will actually be cached.
>
> Unfortunately the use of HCR.DC is incompatible with the guest enabling its
> MMU (SCTLR.M bit). Therefore we must trap accesses to the SCTLR so that we can
> detect when this happens and disable HCR.DC. This is done with the HCR.TVM
> (trap virtual memory controls) bit which also causes various other registers
> to be trapped, all of which can be passed straight through to the underlying
> register. Once the guest has enabled its MMU we no longer need to trap so
> there is no ongoing overhead. In my tests Linux makes about half a dozen
> accesses to these registers before the MMU is enabled, I would expect other
> OSes to behave similarly (the sequence of writes needed to setup the MMU is
> pretty obvious).
>
> Apart from this unfortunate need to trap these accesses this approach is
> incompatible with guests which attempt to do DMA operations with their MMU
> disabled. In practice this means guests with passthrough which we do not yet
> support. Since a typical guest (including dom0) does not access devices which
> require DMA until after it is fully up and running with paging enabled the
> main risk is to in-guest firmware which does DMA i.e. running EFI in a guest,
> with a disk passed through and booting from that disk. Since we know that dom0
> is not using any such firmware and we do not support device passthrough to
> guests yet we can live with this restriction. Once passthrough is implemented
> this will need to be revisited.
>
> The patch includes a couple of seemingly unrelated but necessary changes:
>
> - HSR_SYSREG_CRN_MASK was incorrectly defined, which happened to be benign
> with the existing set of system register we handled, but broke with the new
> ones introduced here.
> - The defines used to decode the HSR system register fields were named the
> same as the register. This breaks the accessor macros. This had gone
> unnoticed because the handling of the existing trapped registers did not
> require accessing the underlying hardware register. Rename those constants
> with an HSR_SYSREG prefix (in line with HSR_CP32/64 for 32-bit registers).
>
> This patch has survived thousands of boot loops on a Midway system.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> My preferred solution here would be for the tools to use an uncached mapping
> of guest memory when building the guest, which requires adding a new privmcd
> ioctl (a relatively straightforward patch) and plumbing a "cached" flag
> through the libxc foreign mapping interfaces (a twisty maze of passage, all
> alike). IMO the libxc side of this patch was not looking suitable for a 4.4
> freeze exception, since it was quite large (because we have 4 or more mapping
> interfaces in libxc, some of which call back into others).
>
> So I propose this version for 4.4. The uncached mapping solution should be
> revisited for a future release.
>
> At the risk of appearing to be going mad:
>
> <speaking hat="submitter">
>
> This bug results in memory corruption bugs in the guest, which mostly manifest
> as a failure to boot the guest (subsequent bad behaviour is possible but, I
> think, unlikely) the frequency of failures is perhaps 1/10 times. This would
> not constitute an awesome release.
>
> Although the patch is large most of it is repetative and mechanical (made
> explicit through the use of macros in many cases). The biggest risk is that
> one of the registers is not passed through correctly (i.e. the wrong size or
> target registers). The ones which Linux uses have been tested and appear to
> function OK. The others might be buggy but this is mitigated through the use
> of the same set of macros.
>
> I think the chance of the patch having a bug wrt my understanding of the
> hardware behaviour is pretty low. WRT there being bugs in my understanding of
> the hardware documentation, I would say middle to low, but I have discussed it
> with some folks at ARM and they didn't call me an idiot (in fact pretty much
> the same thing has been proposed for KVM).
>
> Overall I think the benefits outweigh the risks.
>
> One thing I'm not sure about is reverting the previous fix in a0035ecc0d82.
> It's reasonably recent so reverting it takes us back to a pretty well
> understood state in the libraries. The functionality is harmless if
> incomplete. I think given the first argument I would lean towards reverting.
>
> </speaking>
>
> <speaking hat="stand in release manager">
>
> OK.
>
> </speaking>
>
> Obviously if you think I'm being to easy on myself please say so.
>
> Actually, if you think my judgement is right I'd appreciate being told so too.
> ---
> xen/arch/arm/domain.c | 5 ++
> xen/arch/arm/domain_build.c | 1 +
> xen/arch/arm/traps.c | 153 ++++++++++++++++++++++++++++++++++++++-
> xen/arch/arm/vtimer.c | 6 +-
> xen/include/asm-arm/cpregs.h | 4 +
> xen/include/asm-arm/processor.h | 2 +-
> xen/include/asm-arm/sysregs.h | 19 ++++-
> 7 files changed, 182 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 124cccf..104d228 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> else
> hcr |= HCR_RW;
>
> + if ( n->arch.sctlr & SCTLR_M )
> + hcr &= ~(HCR_TVM|HCR_DC);
> + else
> + hcr |= (HCR_TVM|HCR_DC);
> +
> WRITE_SYSREG(hcr, HCR_EL2);
> isb();
Is this actually needed? Shouldn't HCR be already correctly updated by
update_sctlr?
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 47b781b..bb31db8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1026,6 +1026,7 @@ int construct_dom0(struct domain *d)
> else
> WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
> #endif
> + WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_DC | HCR_TVM, HCR_EL2);
>
> /*
> * kernel_load will determine the placement of the initrd & fdt in
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 7c5ab19..d00bba3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1279,6 +1279,23 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
> regs->pc += hsr.len ? 4 : 2;
> }
>
> +static void update_sctlr(uint32_t v)
> +{
> + /*
> + * 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 ( v & SCTLR_M )
> + WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
> +}
> +
> static void do_cp15_32(struct cpu_user_regs *regs,
> union hsr hsr)
> {
> @@ -1338,6 +1355,89 @@ 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)
Can/Should CP32_PASSTHRU64_LO be used instead of this?
> +/*
> + * 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(*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",
> @@ -1351,6 +1451,9 @@ 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) )
> {
> @@ -1368,6 +1471,26 @@ 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; \
it doesn't look like r, r1 and r2 are used anywhere
> + } \
> + 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",
> @@ -1382,11 +1505,12 @@ 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);
>
> switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
> {
> - case CNTP_CTL_EL0:
> - case CNTP_TVAL_EL0:
> + case HSR_SYSREG_CNTP_CTL_EL0:
> + case HSR_SYSREG_CNTP_TVAL_EL0:
> if ( !vtimer_emulate(regs, hsr) )
> {
> dprintk(XENLOG_ERR,
> @@ -1394,6 +1518,31 @@ 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(*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/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 433ad55..e325f78 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 CNTP_CTL_EL0:
> + case HSR_SYSREG_CNTP_CTL_EL0:
> vtimer_cntp_ctl(regs, &r, sysreg.read);
> if ( sysreg.read )
> *x = r;
> return 1;
> - case CNTP_TVAL_EL0:
> + case HSR_SYSREG_CNTP_TVAL_EL0:
> vtimer_cntp_tval(regs, &r, sysreg.read);
> if ( sysreg.read )
> *x = r;
> return 1;
>
> - case HSR_CPREG64(CNTPCT):
> + case HSR_SYSREG_CNTPCT_EL0:
> 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 f0f1d53..508467a 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -121,6 +121,8 @@
> #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 */
> @@ -260,7 +262,9 @@
> #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/processor.h b/xen/include/asm-arm/processor.h
> index dfe807d..06e638f 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 (0x00003800)
> +#define HSR_SYSREG_CRN_MASK (0x00003c00)
> #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 48ad07e..0cee0e9 100644
> --- a/xen/include/asm-arm/sysregs.h
> +++ b/xen/include/asm-arm/sysregs.h
> @@ -40,8 +40,23 @@
> ((__HSR_SYSREG_##crm) << HSR_SYSREG_CRM_SHIFT) | \
> ((__HSR_SYSREG_##op2) << HSR_SYSREG_OP2_SHIFT)
>
> -#define CNTP_CTL_EL0 HSR_SYSREG(3,3,c14,c2,1)
> -#define CNTP_TVAL_EL0 HSR_SYSREG(3,3,c14,c2,0)
> +#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)
> +
> +
> #endif
>
> #endif
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-07 16:59 ` Julien Grall
@ 2014-01-08 10:02 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2014-01-08 10:02 UTC (permalink / raw)
To: Julien Grall; +Cc: stefano.stabellini, tim, george.dunlap, xen-devel
On Tue, 2014-01-07 at 16:59 +0000, Julien Grall wrote:
> On 01/07/2014 04:02 PM, Ian Campbell wrote:
> .>
> > One thing I'm not sure about is reverting the previous fix in a0035ecc0d82.
> > It's reasonably recent so reverting it takes us back to a pretty well
> > understood state in the libraries. The functionality is harmless if
> > incomplete. I think given the first argument I would lean towards reverting.
>
> I would also prefer reverting the previous patch.
>
> >
> > Obviously if you think I'm being to easy on myself please say so.
>
> Without this patch, we will likely crash a guest in production, that
> it's not acceptable for a release.
>
> >
> > Actually, if you think my judgement is right I'd appreciate being told so too.
> > ---
> > xen/arch/arm/domain.c | 5 ++
> > xen/arch/arm/domain_build.c | 1 +
> > xen/arch/arm/traps.c | 153 ++++++++++++++++++++++++++++++++++++++-
> > xen/arch/arm/vtimer.c | 6 +-
> > xen/include/asm-arm/cpregs.h | 4 +
> > xen/include/asm-arm/processor.h | 2 +-
> > xen/include/asm-arm/sysregs.h | 19 ++++-
> > 7 files changed, 182 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 124cccf..104d228 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> > else
> > hcr |= HCR_RW;
> >
> > + if ( n->arch.sctlr & SCTLR_M )
> > + hcr &= ~(HCR_TVM|HCR_DC);
> > + else
> > + hcr |= (HCR_TVM|HCR_DC);
> > +
> > WRITE_SYSREG(hcr, HCR_EL2);
> > isb();
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 47b781b..bb31db8 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1026,6 +1026,7 @@ int construct_dom0(struct domain *d)
> > else
> > WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
> > #endif
> > + WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_DC | HCR_TVM, HCR_EL2);
>
> Is it useful? As I understand, we will at least context switch one time
> before booting dom0.
For some reason I was thinking that the initial context became d0v0 when
actually it becomes the first idle vcpu. Unlike HCR_RW etc I don't think
anything in the dom0 build code relies on this bit being correct, so I
think you are right that it can be removed.
> If we need it, perhaps the better place to setup it is init_traps?
This may not be quite true today, but: I think it would be good for
init_traps to setup the global/constant HCR settings and leave the
per-VM ones to more per-VM locations.
>
> >
> > /*
> > * kernel_load will determine the placement of the initrd & fdt in
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 7c5ab19..d00bba3 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1279,6 +1279,23 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
> > regs->pc += hsr.len ? 4 : 2;
> > }
> >
> > +static void update_sctlr(uint32_t v)
> > +{
> > + /*
> > + * 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 ( v & SCTLR_M )
> > + WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
>
> Even if it's unlikely, can we handle the case where the guest disable
> the case?
No, we disable the traps so we would never find out that the guest had
done so (at least, not without waiting for a context switch). We don't
really mind this shortcoming though because we expect guests to enable
caches early on and keep them on (in some sense this is part of our ABI)
but in any case if the guest were to disable its MMU it would have to
have taken care of making the caches consistent itself already (e.g. it
is required on native).
This does make me think that the code in ctxt switch is wrong though,
since if the guest does set SCTLR.M then we will enable HCR.DC at that
point, without necessarily doing everything which would be needed. I
think I'll add a per-vcpu flag which indicates whether DC should be set
and clear it in this function.
> Also from ARM ARM B3.2.1, a TLB flush by VMID is required if HCR_DC is
> disabled and the VMID is not changed.
Oh yes, well spotted, will add that.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-07 20:39 ` Stefano Stabellini
@ 2014-01-08 10:04 ` Ian Campbell
2014-01-08 12:43 ` Stefano Stabellini
0 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2014-01-08 10:04 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, tim, george.dunlap, xen-devel
On Tue, 2014-01-07 at 20:39 +0000, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 124cccf..104d228 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> > else
> > hcr |= HCR_RW;
> >
> > + if ( n->arch.sctlr & SCTLR_M )
> > + hcr &= ~(HCR_TVM|HCR_DC);
> > + else
> > + hcr |= (HCR_TVM|HCR_DC);
> > +
> > WRITE_SYSREG(hcr, HCR_EL2);
> > isb();
>
> Is this actually needed? Shouldn't HCR be already correctly updated by
> update_sctlr?
Not if we are switching back and forth between two guests which are in
different states.
> > +/*
> > + * 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)
>
> Can/Should CP32_PASSTHRU64_LO be used instead of this?
LO preserves the upper 32-bits which this macro deliberately does not.
Now, an AArch32 guest on an AArch64 hypervisor should never have
anything else in the top bits of a register which it sees as 32-bit, so
using LO would work, but I think having it be explicit like this is
better.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-08 10:04 ` Ian Campbell
@ 2014-01-08 12:43 ` Stefano Stabellini
2014-01-08 13:08 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2014-01-08 12:43 UTC (permalink / raw)
To: Ian Campbell
Cc: xen-devel, tim, george.dunlap, julien.grall, Stefano Stabellini
On Wed, 8 Jan 2014, Ian Campbell wrote:
> On Tue, 2014-01-07 at 20:39 +0000, Stefano Stabellini wrote:
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > index 124cccf..104d228 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> > > else
> > > hcr |= HCR_RW;
> > >
> > > + if ( n->arch.sctlr & SCTLR_M )
> > > + hcr &= ~(HCR_TVM|HCR_DC);
> > > + else
> > > + hcr |= (HCR_TVM|HCR_DC);
> > > +
> > > WRITE_SYSREG(hcr, HCR_EL2);
> > > isb();
> >
> > Is this actually needed? Shouldn't HCR be already correctly updated by
> > update_sctlr?
>
> Not if we are switching back and forth between two guests which are in
> different states.
I didn't realize that HCR is not properly saved and restored during
context switch. Maybe it is worth doing that instead? If nothing else
for simplicity?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-08 12:43 ` Stefano Stabellini
@ 2014-01-08 13:08 ` Ian Campbell
2014-01-08 13:49 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2014-01-08 13:08 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, tim, george.dunlap, xen-devel
On Wed, 2014-01-08 at 12:43 +0000, Stefano Stabellini wrote:
> On Wed, 8 Jan 2014, Ian Campbell wrote:
> > On Tue, 2014-01-07 at 20:39 +0000, Stefano Stabellini wrote:
> > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > > index 124cccf..104d228 100644
> > > > --- a/xen/arch/arm/domain.c
> > > > +++ b/xen/arch/arm/domain.c
> > > > @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> > > > else
> > > > hcr |= HCR_RW;
> > > >
> > > > + if ( n->arch.sctlr & SCTLR_M )
> > > > + hcr &= ~(HCR_TVM|HCR_DC);
> > > > + else
> > > > + hcr |= (HCR_TVM|HCR_DC);
> > > > +
> > > > WRITE_SYSREG(hcr, HCR_EL2);
> > > > isb();
> > >
> > > Is this actually needed? Shouldn't HCR be already correctly updated by
> > > update_sctlr?
> >
> > Not if we are switching back and forth between two guests which are in
> > different states.
>
> I didn't realize that HCR is not properly saved and restored during
> context switch. Maybe it is worth doing that instead? If nothing else
> for simplicity?
Yes. We ended up here because originally the HCR content was completely
static.
I'm a little concerned about making this change for 4.4, mostly because
I'd need to reliably track down the right places to initialise
v->arch.hcr and the correct contents.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled
2014-01-08 13:08 ` Ian Campbell
@ 2014-01-08 13:49 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2014-01-08 13:49 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, tim, george.dunlap, xen-devel
On Wed, 2014-01-08 at 13:08 +0000, Ian Campbell wrote:
> I'm a little concerned about making this change for 4.4, mostly because
> I'd need to reliably track down the right places to initialise
> v->arch.hcr and the correct contents.
Stefano and I discussed this IRL. Changing how we handle HCR would
involve checking that places like gic_inject and the
XEN_DOMCTL_set_address_size handling did the right thing. In principal
its easy but it's a little too subtle for us to be comfortable in making
such a change now. So I'm going to stick with the original approach
here.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-01-08 13:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-07 16:02 [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled Ian Campbell
2014-01-07 16:06 ` Ian Campbell
2014-01-07 16:59 ` Julien Grall
2014-01-08 10:02 ` Ian Campbell
2014-01-07 20:39 ` Stefano Stabellini
2014-01-08 10:04 ` Ian Campbell
2014-01-08 12:43 ` Stefano Stabellini
2014-01-08 13:08 ` Ian Campbell
2014-01-08 13:49 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.