* [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations
@ 2018-12-14 11:58 Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 1/5] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM Julien Grall
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Julien Grall @ 2018-12-14 11:58 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Roger Pau Monné
Hi all,
This is version 3 of the series to implement set/way. For more details see
patch #4.
A branch with the code is available at:
https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
branch cacheflush/v3
Cheers,
Julien Grall (5):
xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by
HCR_EL2.TVM
xen/arm: vsysreg: Add wrapper to handle sysreg access trapped by
HCR_EL2.TVM
xen/arm: p2m: Add support for preemption in p2m_cache_flush_range
xen/arm: Implement Set/Way operations
xen/arm: Track page accessed between batch of Set/Way operations
xen/arch/arm/arm64/vsysreg.c | 75 +++++++++++++++++++
xen/arch/arm/domain.c | 14 ++++
xen/arch/arm/domctl.c | 8 +-
xen/arch/arm/p2m.c | 156 +++++++++++++++++++++++++++++++++++++--
xen/arch/arm/traps.c | 25 ++++++-
xen/arch/arm/vcpreg.c | 171 +++++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/domain.c | 4 +
xen/common/domain.c | 5 +-
xen/include/asm-arm/cpregs.h | 1 +
xen/include/asm-arm/domain.h | 8 ++
xen/include/asm-arm/p2m.h | 26 ++++++-
xen/include/xen/domain.h | 2 +
12 files changed, 486 insertions(+), 9 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH for-4.12 v3 1/5] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM
2018-12-14 11:58 [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations Julien Grall
@ 2018-12-14 11:58 ` Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 2/5] xen/arm: vsysreg: Add wrapper to handle sysreg " Julien Grall
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2018-12-14 11:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini
A follow-up patch will require to emulate some accesses to some
co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes
to the virtual memory control registers will be trapped to the hypervisor.
This patch adds the infrastructure to passthrough the access to host
registers. For convenience a bunch of helpers have been added to
generate the different helpers.
Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v3:
- Add Stefano's reviewed-by
Changes in v2:
- Add missing include vreg.h
- Fixup mask TMV_REG32_COMBINED
- Update comments
---
xen/arch/arm/vcpreg.c | 149 +++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/cpregs.h | 1 +
2 files changed, 150 insertions(+)
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 7b783e4bcc..550c25ec3f 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -23,8 +23,129 @@
#include <asm/current.h>
#include <asm/regs.h>
#include <asm/traps.h>
+#include <asm/vreg.h>
#include <asm/vtimer.h>
+/*
+ * Macros to help generating helpers for registers trapped when
+ * HCR_EL2.TVM is set.
+ *
+ * Note that it only traps NS write access from EL1.
+ *
+ * - TVM_REG() should not be used outside of the macros. It is there to
+ * help defining TVM_REG32() and TVM_REG64()
+ * - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
+ * resp. generate helper accessing 32-bit and 64-bit register. "regname"
+ * is the Arm32 name and "xreg" the Arm64 name.
+ * - TVM_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
+ * pair of register sharing the same Arm64 register, but are 2 distinct
+ * Arm32 registers. "lowreg" and "hireg" contains the name for on Arm32
+ * registers, "xreg" contains the name for the combined register on Arm64.
+ * The definition of "lowreg" and "higreg" match the Armv8 specification,
+ * this means "lowreg" is an alias to xreg[31:0] and "high" is an alias to
+ * xreg[63:32].
+ *
+ */
+
+/* The name is passed from the upper macro to workaround macro expansion. */
+#define TVM_REG(sz, func, reg...) \
+static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
+{ \
+ GUEST_BUG_ON(read); \
+ WRITE_SYSREG##sz(*r, reg); \
+ \
+ return true; \
+}
+
+#define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
+#define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
+
+#ifdef CONFIG_ARM_32
+#define TVM_REG32_COMBINED(lowreg, hireg, xreg) \
+ /* Use TVM_REG directly to workaround macro expansion. */ \
+ TVM_REG(32, vreg_emulate_##lowreg, lowreg) \
+ TVM_REG(32, vreg_emulate_##hireg, hireg)
+
+#else /* CONFIG_ARM_64 */
+#define TVM_REG32_COMBINED(lowreg, hireg, xreg) \
+static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r, \
+ bool read, bool hi) \
+{ \
+ register_t reg = READ_SYSREG(xreg); \
+ \
+ GUEST_BUG_ON(read); \
+ if ( hi ) /* reg[63:32] is AArch32 register hireg */ \
+ { \
+ reg &= GENMASK(31, 0); \
+ reg |= ((uint64_t)*r) << 32; \
+ } \
+ else /* reg[31:0] is AArch32 register lowreg. */ \
+ { \
+ reg &= GENMASK(63, 32); \
+ reg |= *r; \
+ } \
+ WRITE_SYSREG(reg, xreg); \
+ \
+ return true; \
+} \
+ \
+static bool vreg_emulate_##lowreg(struct cpu_user_regs *regs, uint32_t *r, \
+ bool read) \
+{ \
+ return vreg_emulate_##xreg(regs, r, read, false); \
+} \
+ \
+static bool vreg_emulate_##hireg(struct cpu_user_regs *regs, uint32_t *r, \
+ bool read) \
+{ \
+ return vreg_emulate_##xreg(regs, r, read, true); \
+}
+#endif
+
+/* Defining helpers for emulating co-processor registers. */
+TVM_REG32(SCTLR, SCTLR_EL1)
+/*
+ * AArch32 provides two way to access TTBR* depending on the access
+ * size, whilst AArch64 provides one way.
+ *
+ * When using AArch32, for simplicity, use the same access size as the
+ * guest.
+ */
+#ifdef CONFIG_ARM_32
+TVM_REG32(TTBR0_32, TTBR0_32)
+TVM_REG32(TTBR1_32, TTBR1_32)
+#else
+TVM_REG32(TTBR0_32, TTBR0_EL1)
+TVM_REG32(TTBR1_32, TTBR1_EL1)
+#endif
+TVM_REG64(TTBR0, TTBR0_EL1)
+TVM_REG64(TTBR1, TTBR1_EL1)
+/* AArch32 registers TTBCR and TTBCR2 share AArch64 register TCR_EL1. */
+TVM_REG32_COMBINED(TTBCR, TTBCR2, TCR_EL1)
+TVM_REG32(DACR, DACR32_EL2)
+TVM_REG32(DFSR, ESR_EL1)
+TVM_REG32(IFSR, IFSR32_EL2)
+/* AArch32 registers DFAR and IFAR shares AArch64 register FAR_EL1. */
+TVM_REG32_COMBINED(DFAR, IFAR, FAR_EL1)
+TVM_REG32(ADFSR, AFSR0_EL1)
+TVM_REG32(AIFSR, AFSR1_EL1)
+/* AArch32 registers MAIR0 and MAIR1 share AArch64 register MAIR_EL1. */
+TVM_REG32_COMBINED(MAIR0, MAIR1, MAIR_EL1)
+/* AArch32 registers AMAIR0 and AMAIR1 share AArch64 register AMAIR_EL1. */
+TVM_REG32_COMBINED(AMAIR0, AMAIR1, AMAIR_EL1)
+TVM_REG32(CONTEXTIDR, CONTEXTIDR_EL1)
+
+/* Macro to generate easily case for co-processor emulation. */
+#define GENERATE_CASE(reg, sz) \
+ case HSR_CPREG##sz(reg): \
+ { \
+ bool res; \
+ \
+ res = vreg_emulate_cp##sz(regs, hsr, vreg_emulate_##reg); \
+ ASSERT(res); \
+ break; \
+ }
+
void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
{
const struct hsr_cp32 cp32 = hsr.cp32;
@@ -65,6 +186,31 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
break;
/*
+ * HCR_EL2.TVM
+ *
+ * ARMv8 (DDI 0487D.a): Table D1-38
+ */
+ GENERATE_CASE(SCTLR, 32)
+ GENERATE_CASE(TTBR0_32, 32)
+ GENERATE_CASE(TTBR1_32, 32)
+ GENERATE_CASE(TTBCR, 32)
+ GENERATE_CASE(TTBCR2, 32)
+ GENERATE_CASE(DACR, 32)
+ GENERATE_CASE(DFSR, 32)
+ GENERATE_CASE(IFSR, 32)
+ GENERATE_CASE(DFAR, 32)
+ GENERATE_CASE(IFAR, 32)
+ GENERATE_CASE(ADFSR, 32)
+ GENERATE_CASE(AIFSR, 32)
+ /* AKA PRRR */
+ GENERATE_CASE(MAIR0, 32)
+ /* AKA NMRR */
+ GENERATE_CASE(MAIR1, 32)
+ GENERATE_CASE(AMAIR0, 32)
+ GENERATE_CASE(AMAIR1, 32)
+ GENERATE_CASE(CONTEXTIDR, 32)
+
+ /*
* MDCR_EL2.TPM
*
* ARMv7 (DDI 0406C.b): B1.14.17
@@ -193,6 +339,9 @@ void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr)
return inject_undef_exception(regs, hsr);
break;
+ GENERATE_CASE(TTBR0, 64)
+ GENERATE_CASE(TTBR1, 64)
+
/*
* CPTR_EL2.T{0..9,12..13}
*
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 97a3c6f1c1..8fd344146e 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -140,6 +140,7 @@
/* CP15 CR2: Translation Table Base and Control Registers */
#define TTBCR p15,0,c2,c0,2 /* Translation Table Base Control Register */
+#define TTBCR2 p15,0,c2,c0,3 /* Translation Table Base Control Register 2 */
#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 */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH for-4.12 v3 2/5] xen/arm: vsysreg: Add wrapper to handle sysreg access trapped by HCR_EL2.TVM
2018-12-14 11:58 [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 1/5] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM Julien Grall
@ 2018-12-14 11:58 ` Julien Grall
2018-12-14 21:14 ` Stefano Stabellini
2018-12-14 11:58 ` [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range Julien Grall
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2018-12-14 11:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini
A follow-up patch will require to emulate some accesses to system
registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes to the
virtual memory control registers will be trapped to the hypervisor.
This patch adds the infrastructure to passthrough the access to the host
registers.
Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
- Add missing include vreg.h
- Update documentation reference to the lastest one
---
xen/arch/arm/arm64/vsysreg.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 6e60824572..16ac9c344a 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -21,8 +21,49 @@
#include <asm/current.h>
#include <asm/regs.h>
#include <asm/traps.h>
+#include <asm/vreg.h>
#include <asm/vtimer.h>
+/*
+ * Macro to help generating helpers for registers trapped when
+ * HCR_EL2.TVM is set.
+ *
+ * Note that it only traps NS write access from EL1.
+ */
+#define TVM_REG(reg) \
+static bool vreg_emulate_##reg(struct cpu_user_regs *regs, \
+ uint64_t *r, bool read) \
+{ \
+ GUEST_BUG_ON(read); \
+ WRITE_SYSREG64(*r, reg); \
+ \
+ return true; \
+}
+
+/* Defining helpers for emulating sysreg registers. */
+TVM_REG(SCTLR_EL1)
+TVM_REG(TTBR0_EL1)
+TVM_REG(TTBR1_EL1)
+TVM_REG(TCR_EL1)
+TVM_REG(ESR_EL1)
+TVM_REG(FAR_EL1)
+TVM_REG(AFSR0_EL1)
+TVM_REG(AFSR1_EL1)
+TVM_REG(MAIR_EL1)
+TVM_REG(AMAIR_EL1)
+TVM_REG(CONTEXTIDR_EL1)
+
+/* Macro to generate easily case for co-processor emulation */
+#define GENERATE_CASE(reg) \
+ case HSR_SYSREG_##reg: \
+ { \
+ bool res; \
+ \
+ res = vreg_emulate_sysreg64(regs, hsr, vreg_emulate_##reg); \
+ ASSERT(res); \
+ break; \
+ }
+
void do_sysreg(struct cpu_user_regs *regs,
const union hsr hsr)
{
@@ -44,6 +85,23 @@ void do_sysreg(struct cpu_user_regs *regs,
break;
/*
+ * HCR_EL2.TVM
+ *
+ * ARMv8 (DDI 0487D.a): Table D1-38
+ */
+ GENERATE_CASE(SCTLR_EL1)
+ GENERATE_CASE(TTBR0_EL1)
+ GENERATE_CASE(TTBR1_EL1)
+ GENERATE_CASE(TCR_EL1)
+ GENERATE_CASE(ESR_EL1)
+ GENERATE_CASE(FAR_EL1)
+ GENERATE_CASE(AFSR0_EL1)
+ GENERATE_CASE(AFSR1_EL1)
+ GENERATE_CASE(MAIR_EL1)
+ GENERATE_CASE(AMAIR_EL1)
+ GENERATE_CASE(CONTEXTIDR_EL1)
+
+ /*
* MDCR_EL2.TDRA
*
* ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range
2018-12-14 11:58 [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 1/5] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 2/5] xen/arm: vsysreg: Add wrapper to handle sysreg " Julien Grall
@ 2018-12-14 11:58 ` Julien Grall
2018-12-14 21:27 ` Stefano Stabellini
2018-12-14 11:58 ` [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of " Julien Grall
4 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2018-12-14 11:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini
p2m_cache_flush_range does not yet support preemption, this may be an
issue as cleaning the cache can take a long time. While the current
caller (XEN_DOMCTL_cacheflush) does not stricly require preemption, this
will be necessary for new caller in a follow-up patch.
The preemption implemented is quite simple, a counter is incremented by:
- 1 on region skipped
- 10 for each page requiring a flush
When the counter reach 512 or above, we will check if preemption is
needed. If not, the counter will be reset to 0. If yes, the function
will stop, update start (to allow resuming later on) and return
-ERESTART. This allows the caller to decide how the preemption will be
done.
For now, XEN_DOMCTL_cacheflush will continue to ignore the preemption.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
- Patch added
---
xen/arch/arm/domctl.c | 8 +++++++-
xen/arch/arm/p2m.c | 35 ++++++++++++++++++++++++++++++++---
xen/include/asm-arm/p2m.h | 4 +++-
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 20691528a6..9da88b8c64 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -54,6 +54,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
{
gfn_t s = _gfn(domctl->u.cacheflush.start_pfn);
gfn_t e = gfn_add(s, domctl->u.cacheflush.nr_pfns);
+ int rc;
if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
return -EINVAL;
@@ -61,7 +62,12 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
if ( gfn_x(e) < gfn_x(s) )
return -EINVAL;
- return p2m_cache_flush_range(d, s, e);
+ /* XXX: Handle preemption */
+ do
+ rc = p2m_cache_flush_range(d, &s, e);
+ while ( rc == -ERESTART );
+
+ return rc;
}
case XEN_DOMCTL_bind_pt_irq:
{
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 17e2523fc1..5639e4b64c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1524,13 +1524,17 @@ int relinquish_p2m_mapping(struct domain *d)
return rc;
}
-int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
+int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
{
struct p2m_domain *p2m = p2m_get_hostp2m(d);
gfn_t next_block_gfn;
+ gfn_t start = *pstart;
mfn_t mfn = INVALID_MFN;
p2m_type_t t;
unsigned int order;
+ int rc = 0;
+ /* Counter for preemption */
+ unsigned long count = 0;
/*
* The operation cache flush will invalidate the RAM assigned to the
@@ -1547,6 +1551,25 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
while ( gfn_x(start) < gfn_x(end) )
{
+ /*
+ * Cleaning the cache for the P2M may take a long time. So we
+ * need to be able to preempt. We will arbitrarily preempt every
+ * time count reach 512 or above.
+ *
+ * The count will be incremented by:
+ * - 1 on region skipped
+ * - 10 for each page requiring a flush
+ */
+ if ( count >= 512 )
+ {
+ if ( softirq_pending(smp_processor_id()) )
+ {
+ rc = -ERESTART;
+ break;
+ }
+ count = 0;
+ }
+
/*
* We want to flush page by page as:
* - it may not be possible to map the full block (can be up to 1GB)
@@ -1568,22 +1591,28 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
{
+ count++;
start = next_block_gfn;
continue;
}
}
+ count += 10;
+
flush_page_to_ram(mfn_x(mfn), false);
start = gfn_add(start, 1);
mfn = mfn_add(mfn, 1);
}
- invalidate_icache();
+ if ( rc != -ERESTART )
+ invalidate_icache();
p2m_read_unlock(p2m);
- return 0;
+ *pstart = start;
+
+ return rc;
}
mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 7c1d930b1d..a633e27cc9 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -232,8 +232,10 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
/*
* Clean & invalidate caches corresponding to a region [start,end) of guest
* address space.
+ *
+ * start will get updated if the function is preempted.
*/
-int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end);
+int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
/*
* Map a region in the guest p2m with a specific p2m type.
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations
2018-12-14 11:58 [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations Julien Grall
` (2 preceding siblings ...)
2018-12-14 11:58 ` [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range Julien Grall
@ 2018-12-14 11:58 ` Julien Grall
2018-12-14 21:22 ` Stefano Stabellini
2018-12-14 21:31 ` Andrew Cooper
2018-12-14 11:58 ` [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of " Julien Grall
4 siblings, 2 replies; 15+ messages in thread
From: Julien Grall @ 2018-12-14 11:58 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini
Set/Way operations are used to perform maintenance on a given cache.
At the moment, Set/Way operations are not trapped and therefore a guest
OS will directly act on the local cache. However, a vCPU may migrate to
another pCPU in the middle of the processor. This will result to have
cache with stall data (Set/Way are not propagated) potentially causing
crash. This may be the cause of heisenbug noticed in Osstest [1].
Furthermore, Set/Way operations are not available on system cache. This
means that OS, such as Linux 32-bit, relying on those operations to
fully clean the cache before disabling MMU may break because data may
sits in system caches and not in RAM.
For more details about Set/Way, see the talk "The Art of Virtualizing
Cache Maintenance" given at Xen Summit 2018 [2].
In the context of Xen, we need to trap Set/Way operations and emulate
them. From the Arm Arm (B1.14.4 in DDI 046C.c), Set/Way operations are
difficult to virtualized. So we can assume that a guest OS using them will
suffer the consequence (i.e slowness) until developer removes all the usage
of Set/Way.
As the software is not allowed to infer the Set/Way to Physical Address
mapping, Xen will need to go through the guest P2M and clean &
invalidate all the entries mapped.
Because Set/Way happen in batch (a loop on all Set/Way of a cache), Xen
would need to go through the P2M for every instructions. This is quite
expensive and would severely impact the guest OS. The implementation is
re-using the KVM policy to limit the number of flush:
- If we trap a Set/Way operations, we enable VM trapping (i.e
HVC_EL2.TVM) to detect cache being turned on/off, and do a full
clean.
- We clean the caches when turning on and off
- Once the caches are enabled, we stop trapping VM instructions
[1] https://lists.xenproject.org/archives/html/xen-devel/2017-09/msg03191.html
[2] https://fr.slideshare.net/xen_com_mgr/virtualizing-cache
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
- Fix emulation for Set/Way cache flush arm64 sysreg
- Add support for preemption
- Check cache status on every VM traps in Arm64
- Remove spurious change
---
xen/arch/arm/arm64/vsysreg.c | 17 ++++++++
xen/arch/arm/p2m.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/traps.c | 25 +++++++++++-
xen/arch/arm/vcpreg.c | 22 +++++++++++
xen/include/asm-arm/domain.h | 8 ++++
xen/include/asm-arm/p2m.h | 20 ++++++++++
6 files changed, 183 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 16ac9c344a..8a85507d9d 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -34,9 +34,14 @@
static bool vreg_emulate_##reg(struct cpu_user_regs *regs, \
uint64_t *r, bool read) \
{ \
+ struct vcpu *v = current; \
+ bool cache_enabled = vcpu_has_cache_enabled(v); \
+ \
GUEST_BUG_ON(read); \
WRITE_SYSREG64(*r, reg); \
\
+ p2m_toggle_cache(v, cache_enabled); \
+ \
return true; \
}
@@ -85,6 +90,18 @@ void do_sysreg(struct cpu_user_regs *regs,
break;
/*
+ * HCR_EL2.TSW
+ *
+ * ARMv8 (DDI 0487B.b): Table D1-42
+ */
+ case HSR_SYSREG_DCISW:
+ case HSR_SYSREG_DCCSW:
+ case HSR_SYSREG_DCCISW:
+ if ( !hsr.sysreg.read )
+ p2m_set_way_flush(current);
+ break;
+
+ /*
* HCR_EL2.TVM
*
* ARMv8 (DDI 0487D.a): Table D1-38
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5639e4b64c..125d858d02 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -3,6 +3,7 @@
#include <xen/iocap.h>
#include <xen/lib.h>
#include <xen/sched.h>
+#include <xen/softirq.h>
#include <asm/event.h>
#include <asm/flushtlb.h>
@@ -1615,6 +1616,97 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
return rc;
}
+/*
+ * Clean & invalidate RAM associated to the guest vCPU.
+ *
+ * The function can only work with the current vCPU and should be called
+ * with IRQ enabled as the vCPU could get preempted.
+ */
+void p2m_flush_vm(struct vcpu *v)
+{
+ int rc;
+ gfn_t start = _gfn(0);
+
+ ASSERT(v == current);
+ ASSERT(local_irq_is_enabled());
+ ASSERT(v->arch.need_flush_to_ram);
+
+ do
+ {
+ rc = p2m_cache_flush_range(v->domain, &start, _gfn(ULONG_MAX));
+ if ( rc == -ERESTART )
+ do_softirq();
+ } while ( rc == -ERESTART );
+
+ if ( rc != 0 )
+ gprintk(XENLOG_WARNING,
+ "P2M has not been correctly cleaned (rc = %d)\n",
+ rc);
+
+ v->arch.need_flush_to_ram = false;
+}
+
+/*
+ * See note at ARMv7 ARM B1.14.4 (DDI 0406C.c) (TL;DR: S/W ops are not
+ * easily virtualized).
+ *
+ * Main problems:
+ * - S/W ops are local to a CPU (not broadcast)
+ * - We have line migration behind our back (speculation)
+ * - System caches don't support S/W at all (damn!)
+ *
+ * In the face of the above, the best we can do is to try and convert
+ * S/W ops to VA ops. Because the guest is not allowed to infer the S/W
+ * to PA mapping, it can only use S/W to nuke the whole cache, which is
+ * rather a good thing for us.
+ *
+ * Also, it is only used when turning caches on/off ("The expected
+ * usage of the cache maintenance instructions that operate by set/way
+ * is associated with the powerdown and powerup of caches, if this is
+ * required by the implementation.").
+ *
+ * We use the following policy:
+ * - If we trap a S/W operation, we enabled VM trapping to detect
+ * caches being turned on/off, and do a full clean.
+ *
+ * - We flush the caches on both caches being turned on and off.
+ *
+ * - Once the caches are enabled, we stop trapping VM ops.
+ */
+void p2m_set_way_flush(struct vcpu *v)
+{
+ /* This function can only work with the current vCPU. */
+ ASSERT(v == current);
+
+ if ( !(v->arch.hcr_el2 & HCR_TVM) )
+ {
+ v->arch.need_flush_to_ram = true;
+ vcpu_hcr_set_flags(v, HCR_TVM);
+ }
+}
+
+void p2m_toggle_cache(struct vcpu *v, bool was_enabled)
+{
+ bool now_enabled = vcpu_has_cache_enabled(v);
+
+ /* This function can only work with the current vCPU. */
+ ASSERT(v == current);
+
+ /*
+ * If switching the MMU+caches on, need to invalidate the caches.
+ * If switching it off, need to clean the caches.
+ * Clean + invalidate does the trick always.
+ */
+ if ( was_enabled != now_enabled )
+ {
+ v->arch.need_flush_to_ram = true;
+ }
+
+ /* Caches are now on, stop trapping VM ops (until a S/W op) */
+ if ( now_enabled )
+ vcpu_hcr_clear_flags(v, HCR_TVM);
+}
+
mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
{
return p2m_lookup(d, gfn, NULL);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 02665cc7b4..221c762ada 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -97,7 +97,7 @@ register_t get_default_hcr_flags(void)
{
return (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
(vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
- HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
+ HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB|HCR_TSW);
}
static enum {
@@ -2258,10 +2258,33 @@ static void check_for_pcpu_work(void)
}
}
+/*
+ * Process pending work for the vCPU. Any call should be fast or
+ * implement preemption.
+ */
+static void check_for_vcpu_work(void)
+{
+ struct vcpu *v = current;
+
+ if ( likely(!v->arch.need_flush_to_ram) )
+ return;
+
+ /*
+ * Give a chance for the pCPU to process work before handling the vCPU
+ * pending work.
+ */
+ check_for_pcpu_work();
+
+ local_irq_enable();
+ p2m_flush_vm(v);
+ local_irq_disable();
+}
+
void leave_hypervisor_tail(void)
{
local_irq_disable();
+ check_for_vcpu_work();
check_for_pcpu_work();
vgic_sync_to_lrs();
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 550c25ec3f..cdc91cdf5b 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -51,9 +51,14 @@
#define TVM_REG(sz, func, reg...) \
static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
{ \
+ struct vcpu *v = current; \
+ bool cache_enabled = vcpu_has_cache_enabled(v); \
+ \
GUEST_BUG_ON(read); \
WRITE_SYSREG##sz(*r, reg); \
\
+ p2m_toggle_cache(v, cache_enabled); \
+ \
return true; \
}
@@ -71,6 +76,8 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r, \
bool read, bool hi) \
{ \
+ struct vcpu *v = current; \
+ bool cache_enabled = vcpu_has_cache_enabled(v); \
register_t reg = READ_SYSREG(xreg); \
\
GUEST_BUG_ON(read); \
@@ -86,6 +93,8 @@ static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r, \
} \
WRITE_SYSREG(reg, xreg); \
\
+ p2m_toggle_cache(v, cache_enabled); \
+ \
return true; \
} \
\
@@ -186,6 +195,19 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
break;
/*
+ * HCR_EL2.TSW
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.6
+ * ARMv8 (DDI 0487B.b): Table D1-42
+ */
+ case HSR_CPREG32(DCISW):
+ case HSR_CPREG32(DCCSW):
+ case HSR_CPREG32(DCCISW):
+ if ( !cp32.read )
+ p2m_set_way_flush(current);
+ break;
+
+ /*
* HCR_EL2.TVM
*
* ARMv8 (DDI 0487D.a): Table D1-38
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 175de44927..f16b973e0d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -202,6 +202,14 @@ struct arch_vcpu
struct vtimer phys_timer;
struct vtimer virt_timer;
bool vtimer_initialized;
+
+ /*
+ * The full P2M may require some cleaning (e.g when emulation
+ * set/way). As the action can take a long time, it requires
+ * preemption. So this is deferred until we return to the guest.
+ */
+ bool need_flush_to_ram;
+
} __cacheline_aligned;
void vcpu_show_execution_state(struct vcpu *);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index a633e27cc9..79abcb5a63 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -6,6 +6,8 @@
#include <xen/rwlock.h>
#include <xen/mem_access.h>
+#include <asm/current.h>
+
#define paddr_bits PADDR_BITS
/* Holds the bit size of IPAs in p2m tables. */
@@ -237,6 +239,12 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
*/
int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
+void p2m_set_way_flush(struct vcpu *v);
+
+void p2m_toggle_cache(struct vcpu *v, bool was_enabled);
+
+void p2m_flush_vm(struct vcpu *v);
+
/*
* Map a region in the guest p2m with a specific p2m type.
* The memory attributes will be derived from the p2m type.
@@ -364,6 +372,18 @@ static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
return -EOPNOTSUPP;
}
+/*
+ * A vCPU has cache enabled only when the MMU is enabled and data cache
+ * is enabled.
+ */
+static inline bool vcpu_has_cache_enabled(struct vcpu *v)
+{
+ /* Only works with the current vCPU */
+ ASSERT(current == v);
+
+ return (READ_SYSREG32(SCTLR_EL1) & (SCTLR_C|SCTLR_M)) == (SCTLR_C|SCTLR_M);
+}
+
#endif /* _XEN_P2M_H */
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of Set/Way operations
2018-12-14 11:58 [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations Julien Grall
` (3 preceding siblings ...)
2018-12-14 11:58 ` [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations Julien Grall
@ 2018-12-14 11:58 ` Julien Grall
2018-12-14 21:37 ` Stefano Stabellini
4 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2018-12-14 11:58 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Roger Pau Monné
At the moment, the implementation of Set/Way operations will go through
all the entries of the guest P2M and flush them. However, this is very
expensive and may render unusable a guest OS using them.
For instance, Linux 32-bit will use Set/Way operations during secondary
CPU bring-up. As the implementation is really expensive, it may be possible
to hit the CPU bring-up timeout.
To limit the Set/Way impact, we track what pages has been of the guest
has been accessed between batch of Set/Way operations. This is done
using bit[0] (aka valid bit) of the P2M entry.
This patch adds a new per-arch helper is introduced to perform actions just
before the guest is first unpaused. This will be used to invalidate the
P2M to track access from the start of the guest.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
While we can spread d->creation_finished all over the code, the per-arch
helper to perform actions just before the guest is first unpaused can
bring a lot of benefit for both architecture. For instance, on Arm, the
flush to the instruction cache could be delayed until the domain is
first run. This would improve greatly the performance of creating guest.
I am still doing the benchmark whether having a command line option is
worth it. I will provide numbers as soon as I have them.
Changes in v3:
- Add Jan reviewed-by for non-ARM pieces
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/arm/domain.c | 14 ++++++++++++++
xen/arch/arm/p2m.c | 29 +++++++++++++++++++++++++++--
xen/arch/x86/domain.c | 4 ++++
xen/common/domain.c | 5 ++++-
xen/include/asm-arm/p2m.h | 2 ++
xen/include/xen/domain.h | 2 ++
6 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 1d926dcb29..41f101746e 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -767,6 +767,20 @@ int arch_domain_soft_reset(struct domain *d)
return -ENOSYS;
}
+void arch_domain_creation_finished(struct domain *d)
+{
+ /*
+ * To avoid flushing the whole guest RAM on the first Set/Way, we
+ * invalidate the P2M to track what has been accessed.
+ *
+ * This is only turned when IOMMU is not used or the page-table are
+ * not shared because bit[0] (e.g valid bit) unset will result
+ * IOMMU fault that could be not fixed-up.
+ */
+ if ( !iommu_use_hap_pt(d) )
+ p2m_invalidate_root(p2m_get_hostp2m(d));
+}
+
static int is_guest_pv32_psr(uint32_t psr)
{
switch (psr & PSR_MODE_MASK)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 125d858d02..347028c325 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1079,6 +1079,22 @@ static void p2m_invalidate_table(struct p2m_domain *p2m, mfn_t mfn)
}
/*
+ * Invalidate all entries in the root page-tables. This is
+ * useful to get fault on entry and do an action.
+ */
+void p2m_invalidate_root(struct p2m_domain *p2m)
+{
+ unsigned int i;
+
+ p2m_write_lock(p2m);
+
+ for ( i = 0; i < P2M_ROOT_LEVEL; i++ )
+ p2m_invalidate_table(p2m, page_to_mfn(p2m->root + i));
+
+ p2m_write_unlock(p2m);
+}
+
+/*
* Resolve any translation fault due to change in the p2m. This
* includes break-before-make and valid bit cleared.
*/
@@ -1587,10 +1603,12 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
*/
if ( gfn_eq(start, next_block_gfn) )
{
- mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
+ bool valid;
+
+ mfn = p2m_get_entry(p2m, start, &t, NULL, &order, &valid);
next_block_gfn = gfn_next_boundary(start, order);
- if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
+ if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) || !valid )
{
count++;
start = next_block_gfn;
@@ -1624,6 +1642,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
*/
void p2m_flush_vm(struct vcpu *v)
{
+ struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
int rc;
gfn_t start = _gfn(0);
@@ -1643,6 +1662,12 @@ void p2m_flush_vm(struct vcpu *v)
"P2M has not been correctly cleaned (rc = %d)\n",
rc);
+ /*
+ * Invalidate the p2m to track which page was modified by the guest
+ * between call of p2m_flush_vm().
+ */
+ p2m_invalidate_root(p2m);
+
v->arch.need_flush_to_ram = false;
}
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f0e0cdbb0e..3729887d00 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -762,6 +762,10 @@ int arch_domain_soft_reset(struct domain *d)
return ret;
}
+void arch_domain_creation_finished(struct domain *d)
+{
+}
+
/*
* These are the masks of CR4 bits (subject to hardware availability) which a
* PV guest may not legitimiately attempt to modify.
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 78cc5249e8..c623daec56 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1116,8 +1116,11 @@ int domain_unpause_by_systemcontroller(struct domain *d)
* Creation is considered finished when the controller reference count
* first drops to 0.
*/
- if ( new == 0 )
+ if ( new == 0 && !d->creation_finished )
+ {
d->creation_finished = true;
+ arch_domain_creation_finished(d);
+ }
domain_unpause(d);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 79abcb5a63..01cd3ee4b5 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -231,6 +231,8 @@ int p2m_set_entry(struct p2m_domain *p2m,
bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
+void p2m_invalidate_root(struct p2m_domain *p2m);
+
/*
* Clean & invalidate caches corresponding to a region [start,end) of guest
* address space.
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 33e41486cb..d1bfc82f57 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -70,6 +70,8 @@ void arch_domain_unpause(struct domain *d);
int arch_domain_soft_reset(struct domain *d);
+void arch_domain_creation_finished(struct domain *d);
+
void arch_p2m_set_access_required(struct domain *d, bool access_required);
int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 2/5] xen/arm: vsysreg: Add wrapper to handle sysreg access trapped by HCR_EL2.TVM
2018-12-14 11:58 ` [PATCH for-4.12 v3 2/5] xen/arm: vsysreg: Add wrapper to handle sysreg " Julien Grall
@ 2018-12-14 21:14 ` Stefano Stabellini
0 siblings, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2018-12-14 21:14 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, sstabellini
On Fri, 14 Dec 2018, Julien Grall wrote:
> A follow-up patch will require to emulate some accesses to system
> registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes to the
> virtual memory control registers will be trapped to the hypervisor.
>
> This patch adds the infrastructure to passthrough the access to the host
> registers.
>
> Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2:
> - Add missing include vreg.h
> - Update documentation reference to the lastest one
> ---
> xen/arch/arm/arm64/vsysreg.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index 6e60824572..16ac9c344a 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -21,8 +21,49 @@
> #include <asm/current.h>
> #include <asm/regs.h>
> #include <asm/traps.h>
> +#include <asm/vreg.h>
> #include <asm/vtimer.h>
>
> +/*
> + * Macro to help generating helpers for registers trapped when
> + * HCR_EL2.TVM is set.
> + *
> + * Note that it only traps NS write access from EL1.
> + */
> +#define TVM_REG(reg) \
> +static bool vreg_emulate_##reg(struct cpu_user_regs *regs, \
> + uint64_t *r, bool read) \
> +{ \
> + GUEST_BUG_ON(read); \
> + WRITE_SYSREG64(*r, reg); \
> + \
> + return true; \
> +}
> +
> +/* Defining helpers for emulating sysreg registers. */
> +TVM_REG(SCTLR_EL1)
> +TVM_REG(TTBR0_EL1)
> +TVM_REG(TTBR1_EL1)
> +TVM_REG(TCR_EL1)
> +TVM_REG(ESR_EL1)
> +TVM_REG(FAR_EL1)
> +TVM_REG(AFSR0_EL1)
> +TVM_REG(AFSR1_EL1)
> +TVM_REG(MAIR_EL1)
> +TVM_REG(AMAIR_EL1)
> +TVM_REG(CONTEXTIDR_EL1)
> +
> +/* Macro to generate easily case for co-processor emulation */
> +#define GENERATE_CASE(reg) \
> + case HSR_SYSREG_##reg: \
> + { \
> + bool res; \
> + \
> + res = vreg_emulate_sysreg64(regs, hsr, vreg_emulate_##reg); \
> + ASSERT(res); \
> + break; \
> + }
> +
> void do_sysreg(struct cpu_user_regs *regs,
> const union hsr hsr)
> {
> @@ -44,6 +85,23 @@ void do_sysreg(struct cpu_user_regs *regs,
> break;
>
> /*
> + * HCR_EL2.TVM
> + *
> + * ARMv8 (DDI 0487D.a): Table D1-38
> + */
> + GENERATE_CASE(SCTLR_EL1)
> + GENERATE_CASE(TTBR0_EL1)
> + GENERATE_CASE(TTBR1_EL1)
> + GENERATE_CASE(TCR_EL1)
> + GENERATE_CASE(ESR_EL1)
> + GENERATE_CASE(FAR_EL1)
> + GENERATE_CASE(AFSR0_EL1)
> + GENERATE_CASE(AFSR1_EL1)
> + GENERATE_CASE(MAIR_EL1)
> + GENERATE_CASE(AMAIR_EL1)
> + GENERATE_CASE(CONTEXTIDR_EL1)
> +
> + /*
> * MDCR_EL2.TDRA
> *
> * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations
2018-12-14 11:58 ` [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations Julien Grall
@ 2018-12-14 21:22 ` Stefano Stabellini
2018-12-17 10:17 ` Julien Grall
2018-12-14 21:31 ` Andrew Cooper
1 sibling, 1 reply; 15+ messages in thread
From: Stefano Stabellini @ 2018-12-14 21:22 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, sstabellini
On Fri, 14 Dec 2018, Julien Grall wrote:
> Set/Way operations are used to perform maintenance on a given cache.
> At the moment, Set/Way operations are not trapped and therefore a guest
> OS will directly act on the local cache. However, a vCPU may migrate to
> another pCPU in the middle of the processor. This will result to have
> cache with stall data (Set/Way are not propagated) potentially causing
> crash. This may be the cause of heisenbug noticed in Osstest [1].
>
> Furthermore, Set/Way operations are not available on system cache. This
> means that OS, such as Linux 32-bit, relying on those operations to
> fully clean the cache before disabling MMU may break because data may
> sits in system caches and not in RAM.
>
> For more details about Set/Way, see the talk "The Art of Virtualizing
> Cache Maintenance" given at Xen Summit 2018 [2].
>
> In the context of Xen, we need to trap Set/Way operations and emulate
> them. From the Arm Arm (B1.14.4 in DDI 046C.c), Set/Way operations are
> difficult to virtualized. So we can assume that a guest OS using them will
> suffer the consequence (i.e slowness) until developer removes all the usage
> of Set/Way.
>
> As the software is not allowed to infer the Set/Way to Physical Address
> mapping, Xen will need to go through the guest P2M and clean &
> invalidate all the entries mapped.
>
> Because Set/Way happen in batch (a loop on all Set/Way of a cache), Xen
> would need to go through the P2M for every instructions. This is quite
> expensive and would severely impact the guest OS. The implementation is
> re-using the KVM policy to limit the number of flush:
> - If we trap a Set/Way operations, we enable VM trapping (i.e
> HVC_EL2.TVM) to detect cache being turned on/off, and do a full
> clean.
> - We clean the caches when turning on and off
> - Once the caches are enabled, we stop trapping VM instructions
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2017-09/msg03191.html
> [2] https://fr.slideshare.net/xen_com_mgr/virtualizing-cache
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
> Changes in v2:
> - Fix emulation for Set/Way cache flush arm64 sysreg
> - Add support for preemption
> - Check cache status on every VM traps in Arm64
> - Remove spurious change
> ---
> xen/arch/arm/arm64/vsysreg.c | 17 ++++++++
> xen/arch/arm/p2m.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/traps.c | 25 +++++++++++-
> xen/arch/arm/vcpreg.c | 22 +++++++++++
> xen/include/asm-arm/domain.h | 8 ++++
> xen/include/asm-arm/p2m.h | 20 ++++++++++
> 6 files changed, 183 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index 16ac9c344a..8a85507d9d 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -34,9 +34,14 @@
> static bool vreg_emulate_##reg(struct cpu_user_regs *regs, \
> uint64_t *r, bool read) \
> { \
> + struct vcpu *v = current; \
> + bool cache_enabled = vcpu_has_cache_enabled(v); \
> + \
> GUEST_BUG_ON(read); \
> WRITE_SYSREG64(*r, reg); \
> \
> + p2m_toggle_cache(v, cache_enabled); \
> + \
> return true; \
> }
>
> @@ -85,6 +90,18 @@ void do_sysreg(struct cpu_user_regs *regs,
> break;
>
> /*
> + * HCR_EL2.TSW
> + *
> + * ARMv8 (DDI 0487B.b): Table D1-42
> + */
> + case HSR_SYSREG_DCISW:
> + case HSR_SYSREG_DCCSW:
> + case HSR_SYSREG_DCCISW:
> + if ( !hsr.sysreg.read )
> + p2m_set_way_flush(current);
> + break;
> +
> + /*
> * HCR_EL2.TVM
> *
> * ARMv8 (DDI 0487D.a): Table D1-38
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 5639e4b64c..125d858d02 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -3,6 +3,7 @@
> #include <xen/iocap.h>
> #include <xen/lib.h>
> #include <xen/sched.h>
> +#include <xen/softirq.h>
>
> #include <asm/event.h>
> #include <asm/flushtlb.h>
> @@ -1615,6 +1616,97 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
> return rc;
> }
>
> +/*
> + * Clean & invalidate RAM associated to the guest vCPU.
> + *
> + * The function can only work with the current vCPU and should be called
> + * with IRQ enabled as the vCPU could get preempted.
> + */
> +void p2m_flush_vm(struct vcpu *v)
> +{
> + int rc;
> + gfn_t start = _gfn(0);
> +
> + ASSERT(v == current);
> + ASSERT(local_irq_is_enabled());
> + ASSERT(v->arch.need_flush_to_ram);
> +
> + do
> + {
> + rc = p2m_cache_flush_range(v->domain, &start, _gfn(ULONG_MAX));
> + if ( rc == -ERESTART )
> + do_softirq();
> + } while ( rc == -ERESTART );
> +
> + if ( rc != 0 )
> + gprintk(XENLOG_WARNING,
> + "P2M has not been correctly cleaned (rc = %d)\n",
> + rc);
> +
> + v->arch.need_flush_to_ram = false;
> +}
> +
> +/*
> + * See note at ARMv7 ARM B1.14.4 (DDI 0406C.c) (TL;DR: S/W ops are not
> + * easily virtualized).
> + *
> + * Main problems:
> + * - S/W ops are local to a CPU (not broadcast)
> + * - We have line migration behind our back (speculation)
> + * - System caches don't support S/W at all (damn!)
> + *
> + * In the face of the above, the best we can do is to try and convert
> + * S/W ops to VA ops. Because the guest is not allowed to infer the S/W
> + * to PA mapping, it can only use S/W to nuke the whole cache, which is
> + * rather a good thing for us.
> + *
> + * Also, it is only used when turning caches on/off ("The expected
> + * usage of the cache maintenance instructions that operate by set/way
> + * is associated with the powerdown and powerup of caches, if this is
> + * required by the implementation.").
> + *
> + * We use the following policy:
> + * - If we trap a S/W operation, we enabled VM trapping to detect
> + * caches being turned on/off, and do a full clean.
> + *
> + * - We flush the caches on both caches being turned on and off.
> + *
> + * - Once the caches are enabled, we stop trapping VM ops.
> + */
> +void p2m_set_way_flush(struct vcpu *v)
> +{
> + /* This function can only work with the current vCPU. */
> + ASSERT(v == current);
> +
> + if ( !(v->arch.hcr_el2 & HCR_TVM) )
> + {
> + v->arch.need_flush_to_ram = true;
> + vcpu_hcr_set_flags(v, HCR_TVM);
> + }
> +}
> +
> +void p2m_toggle_cache(struct vcpu *v, bool was_enabled)
> +{
> + bool now_enabled = vcpu_has_cache_enabled(v);
> +
> + /* This function can only work with the current vCPU. */
> + ASSERT(v == current);
> +
> + /*
> + * If switching the MMU+caches on, need to invalidate the caches.
> + * If switching it off, need to clean the caches.
> + * Clean + invalidate does the trick always.
> + */
> + if ( was_enabled != now_enabled )
> + {
> + v->arch.need_flush_to_ram = true;
> + }
NIT: no need for brakets
> + /* Caches are now on, stop trapping VM ops (until a S/W op) */
> + if ( now_enabled )
> + vcpu_hcr_clear_flags(v, HCR_TVM);
> +}
> +
> mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
> {
> return p2m_lookup(d, gfn, NULL);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 02665cc7b4..221c762ada 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -97,7 +97,7 @@ register_t get_default_hcr_flags(void)
> {
> return (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
> - HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
> + HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB|HCR_TSW);
> }
>
> static enum {
> @@ -2258,10 +2258,33 @@ static void check_for_pcpu_work(void)
> }
> }
>
> +/*
> + * Process pending work for the vCPU. Any call should be fast or
> + * implement preemption.
> + */
> +static void check_for_vcpu_work(void)
> +{
> + struct vcpu *v = current;
> +
> + if ( likely(!v->arch.need_flush_to_ram) )
> + return;
> +
> + /*
> + * Give a chance for the pCPU to process work before handling the vCPU
> + * pending work.
> + */
> + check_for_pcpu_work();
> +
> + local_irq_enable();
> + p2m_flush_vm(v);
> + local_irq_disable();
> +}
> +
> void leave_hypervisor_tail(void)
> {
> local_irq_disable();
>
> + check_for_vcpu_work();
> check_for_pcpu_work();
>
> vgic_sync_to_lrs();
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 550c25ec3f..cdc91cdf5b 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -51,9 +51,14 @@
> #define TVM_REG(sz, func, reg...) \
> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
> { \
> + struct vcpu *v = current; \
> + bool cache_enabled = vcpu_has_cache_enabled(v); \
> + \
> GUEST_BUG_ON(read); \
> WRITE_SYSREG##sz(*r, reg); \
> \
> + p2m_toggle_cache(v, cache_enabled); \
> + \
> return true; \
> }
>
> @@ -71,6 +76,8 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
> static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r, \
> bool read, bool hi) \
> { \
> + struct vcpu *v = current; \
> + bool cache_enabled = vcpu_has_cache_enabled(v); \
> register_t reg = READ_SYSREG(xreg); \
> \
> GUEST_BUG_ON(read); \
> @@ -86,6 +93,8 @@ static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r, \
> } \
> WRITE_SYSREG(reg, xreg); \
> \
> + p2m_toggle_cache(v, cache_enabled); \
> + \
> return true; \
> } \
> \
> @@ -186,6 +195,19 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
> break;
>
> /*
> + * HCR_EL2.TSW
> + *
> + * ARMv7 (DDI 0406C.b): B1.14.6
> + * ARMv8 (DDI 0487B.b): Table D1-42
> + */
> + case HSR_CPREG32(DCISW):
> + case HSR_CPREG32(DCCSW):
> + case HSR_CPREG32(DCCISW):
> + if ( !cp32.read )
> + p2m_set_way_flush(current);
> + break;
> +
> + /*
> * HCR_EL2.TVM
> *
> * ARMv8 (DDI 0487D.a): Table D1-38
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 175de44927..f16b973e0d 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -202,6 +202,14 @@ struct arch_vcpu
> struct vtimer phys_timer;
> struct vtimer virt_timer;
> bool vtimer_initialized;
> +
> + /*
> + * The full P2M may require some cleaning (e.g when emulation
> + * set/way). As the action can take a long time, it requires
> + * preemption. So this is deferred until we return to the guest.
Please replace the last sentence of this comment with:
"It is deferred until we return to guest, where we can more easily check
for softirqs and preempt the vcpu safely."
> + */
> + bool need_flush_to_ram;
> +
> } __cacheline_aligned;
>
> void vcpu_show_execution_state(struct vcpu *);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index a633e27cc9..79abcb5a63 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -6,6 +6,8 @@
> #include <xen/rwlock.h>
> #include <xen/mem_access.h>
>
> +#include <asm/current.h>
> +
> #define paddr_bits PADDR_BITS
>
> /* Holds the bit size of IPAs in p2m tables. */
> @@ -237,6 +239,12 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
> */
> int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
>
> +void p2m_set_way_flush(struct vcpu *v);
> +
> +void p2m_toggle_cache(struct vcpu *v, bool was_enabled);
> +
> +void p2m_flush_vm(struct vcpu *v);
> +
> /*
> * Map a region in the guest p2m with a specific p2m type.
> * The memory attributes will be derived from the p2m type.
> @@ -364,6 +372,18 @@ static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
> return -EOPNOTSUPP;
> }
>
> +/*
> + * A vCPU has cache enabled only when the MMU is enabled and data cache
> + * is enabled.
> + */
> +static inline bool vcpu_has_cache_enabled(struct vcpu *v)
> +{
> + /* Only works with the current vCPU */
> + ASSERT(current == v);
> +
> + return (READ_SYSREG32(SCTLR_EL1) & (SCTLR_C|SCTLR_M)) == (SCTLR_C|SCTLR_M);
Line > 80
> +}
> +
> #endif /* _XEN_P2M_H */
>
> /*
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range
2018-12-14 11:58 ` [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range Julien Grall
@ 2018-12-14 21:27 ` Stefano Stabellini
2018-12-18 17:47 ` Julien Grall
0 siblings, 1 reply; 15+ messages in thread
From: Stefano Stabellini @ 2018-12-14 21:27 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, sstabellini
On Fri, 14 Dec 2018, Julien Grall wrote:
> p2m_cache_flush_range does not yet support preemption, this may be an
> issue as cleaning the cache can take a long time. While the current
> caller (XEN_DOMCTL_cacheflush) does not stricly require preemption, this
> will be necessary for new caller in a follow-up patch.
>
> The preemption implemented is quite simple, a counter is incremented by:
> - 1 on region skipped
> - 10 for each page requiring a flush
>
> When the counter reach 512 or above, we will check if preemption is
> needed. If not, the counter will be reset to 0. If yes, the function
> will stop, update start (to allow resuming later on) and return
> -ERESTART. This allows the caller to decide how the preemption will be
> done.
>
> For now, XEN_DOMCTL_cacheflush will continue to ignore the preemption.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
> Changes in v2:
> - Patch added
> ---
> xen/arch/arm/domctl.c | 8 +++++++-
> xen/arch/arm/p2m.c | 35 ++++++++++++++++++++++++++++++++---
> xen/include/asm-arm/p2m.h | 4 +++-
> 3 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 20691528a6..9da88b8c64 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -54,6 +54,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> {
> gfn_t s = _gfn(domctl->u.cacheflush.start_pfn);
> gfn_t e = gfn_add(s, domctl->u.cacheflush.nr_pfns);
> + int rc;
>
> if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
> return -EINVAL;
> @@ -61,7 +62,12 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> if ( gfn_x(e) < gfn_x(s) )
> return -EINVAL;
>
> - return p2m_cache_flush_range(d, s, e);
> + /* XXX: Handle preemption */
> + do
> + rc = p2m_cache_flush_range(d, &s, e);
> + while ( rc == -ERESTART );
> +
> + return rc;
> }
> case XEN_DOMCTL_bind_pt_irq:
> {
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 17e2523fc1..5639e4b64c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1524,13 +1524,17 @@ int relinquish_p2m_mapping(struct domain *d)
> return rc;
> }
>
> -int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
> +int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
> {
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> gfn_t next_block_gfn;
> + gfn_t start = *pstart;
> mfn_t mfn = INVALID_MFN;
> p2m_type_t t;
> unsigned int order;
> + int rc = 0;
> + /* Counter for preemption */
> + unsigned long count = 0;
No need for unsigned long, count could be unsigned int or even unsigned
short.
With this change:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> /*
> * The operation cache flush will invalidate the RAM assigned to the
> @@ -1547,6 +1551,25 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>
> while ( gfn_x(start) < gfn_x(end) )
> {
> + /*
> + * Cleaning the cache for the P2M may take a long time. So we
> + * need to be able to preempt. We will arbitrarily preempt every
> + * time count reach 512 or above.
> + *
> + * The count will be incremented by:
> + * - 1 on region skipped
> + * - 10 for each page requiring a flush
> + */
> + if ( count >= 512 )
> + {
> + if ( softirq_pending(smp_processor_id()) )
> + {
> + rc = -ERESTART;
> + break;
> + }
> + count = 0;
> + }
> +
> /*
> * We want to flush page by page as:
> * - it may not be possible to map the full block (can be up to 1GB)
> @@ -1568,22 +1591,28 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>
> if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
> {
> + count++;
> start = next_block_gfn;
> continue;
> }
> }
>
> + count += 10;
> +
> flush_page_to_ram(mfn_x(mfn), false);
>
> start = gfn_add(start, 1);
> mfn = mfn_add(mfn, 1);
> }
>
> - invalidate_icache();
> + if ( rc != -ERESTART )
> + invalidate_icache();
>
> p2m_read_unlock(p2m);
>
> - return 0;
> + *pstart = start;
> +
> + return rc;
> }
>
> mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 7c1d930b1d..a633e27cc9 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -232,8 +232,10 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
> /*
> * Clean & invalidate caches corresponding to a region [start,end) of guest
> * address space.
> + *
> + * start will get updated if the function is preempted.
> */
> -int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end);
> +int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
>
> /*
> * Map a region in the guest p2m with a specific p2m type.
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations
2018-12-14 11:58 ` [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations Julien Grall
2018-12-14 21:22 ` Stefano Stabellini
@ 2018-12-14 21:31 ` Andrew Cooper
2018-12-18 17:58 ` Julien Grall
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2018-12-14 21:31 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
On 14/12/2018 03:58, Julien Grall wrote:
> Set/Way operations are used to perform maintenance on a given cache.
> At the moment, Set/Way operations are not trapped and therefore a guest
> OS will directly act on the local cache. However, a vCPU may migrate to
> another pCPU in the middle of the processor. This will result to have
> cache with stall data (Set/Way are not propagated) potentially causing
s/stall/stale/ ?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of Set/Way operations
2018-12-14 11:58 ` [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of " Julien Grall
@ 2018-12-14 21:37 ` Stefano Stabellini
0 siblings, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2018-12-14 21:37 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel,
Roger Pau Monné
On Fri, 14 Dec 2018, Julien Grall wrote:
> At the moment, the implementation of Set/Way operations will go through
> all the entries of the guest P2M and flush them. However, this is very
> expensive and may render unusable a guest OS using them.
>
> For instance, Linux 32-bit will use Set/Way operations during secondary
> CPU bring-up. As the implementation is really expensive, it may be possible
> to hit the CPU bring-up timeout.
>
> To limit the Set/Way impact, we track what pages has been of the guest
> has been accessed between batch of Set/Way operations. This is done
> using bit[0] (aka valid bit) of the P2M entry.
>
> This patch adds a new per-arch helper is introduced to perform actions just
> before the guest is first unpaused. This will be used to invalidate the
> P2M to track access from the start of the guest.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> While we can spread d->creation_finished all over the code, the per-arch
> helper to perform actions just before the guest is first unpaused can
> bring a lot of benefit for both architecture. For instance, on Arm, the
> flush to the instruction cache could be delayed until the domain is
> first run. This would improve greatly the performance of creating guest.
>
> I am still doing the benchmark whether having a command line option is
> worth it. I will provide numbers as soon as I have them.
>
> Changes in v3:
> - Add Jan reviewed-by for non-ARM pieces
>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> xen/arch/arm/domain.c | 14 ++++++++++++++
> xen/arch/arm/p2m.c | 29 +++++++++++++++++++++++++++--
> xen/arch/x86/domain.c | 4 ++++
> xen/common/domain.c | 5 ++++-
> xen/include/asm-arm/p2m.h | 2 ++
> xen/include/xen/domain.h | 2 ++
> 6 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 1d926dcb29..41f101746e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -767,6 +767,20 @@ int arch_domain_soft_reset(struct domain *d)
> return -ENOSYS;
> }
>
> +void arch_domain_creation_finished(struct domain *d)
> +{
> + /*
> + * To avoid flushing the whole guest RAM on the first Set/Way, we
> + * invalidate the P2M to track what has been accessed.
> + *
> + * This is only turned when IOMMU is not used or the page-table are
> + * not shared because bit[0] (e.g valid bit) unset will result
> + * IOMMU fault that could be not fixed-up.
> + */
> + if ( !iommu_use_hap_pt(d) )
> + p2m_invalidate_root(p2m_get_hostp2m(d));
> +}
> +
> static int is_guest_pv32_psr(uint32_t psr)
> {
> switch (psr & PSR_MODE_MASK)
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 125d858d02..347028c325 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1079,6 +1079,22 @@ static void p2m_invalidate_table(struct p2m_domain *p2m, mfn_t mfn)
> }
>
> /*
> + * Invalidate all entries in the root page-tables. This is
> + * useful to get fault on entry and do an action.
> + */
> +void p2m_invalidate_root(struct p2m_domain *p2m)
> +{
> + unsigned int i;
> +
> + p2m_write_lock(p2m);
> +
> + for ( i = 0; i < P2M_ROOT_LEVEL; i++ )
> + p2m_invalidate_table(p2m, page_to_mfn(p2m->root + i));
> +
> + p2m_write_unlock(p2m);
> +}
> +
> +/*
> * Resolve any translation fault due to change in the p2m. This
> * includes break-before-make and valid bit cleared.
> */
> @@ -1587,10 +1603,12 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
> */
> if ( gfn_eq(start, next_block_gfn) )
> {
> - mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
> + bool valid;
> +
> + mfn = p2m_get_entry(p2m, start, &t, NULL, &order, &valid);
> next_block_gfn = gfn_next_boundary(start, order);
>
> - if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
> + if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) || !valid )
> {
> count++;
> start = next_block_gfn;
> @@ -1624,6 +1642,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
> */
> void p2m_flush_vm(struct vcpu *v)
> {
> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> int rc;
> gfn_t start = _gfn(0);
>
> @@ -1643,6 +1662,12 @@ void p2m_flush_vm(struct vcpu *v)
> "P2M has not been correctly cleaned (rc = %d)\n",
> rc);
>
> + /*
> + * Invalidate the p2m to track which page was modified by the guest
> + * between call of p2m_flush_vm().
> + */
> + p2m_invalidate_root(p2m);
> +
> v->arch.need_flush_to_ram = false;
> }
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f0e0cdbb0e..3729887d00 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -762,6 +762,10 @@ int arch_domain_soft_reset(struct domain *d)
> return ret;
> }
>
> +void arch_domain_creation_finished(struct domain *d)
> +{
> +}
> +
> /*
> * These are the masks of CR4 bits (subject to hardware availability) which a
> * PV guest may not legitimiately attempt to modify.
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 78cc5249e8..c623daec56 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1116,8 +1116,11 @@ int domain_unpause_by_systemcontroller(struct domain *d)
> * Creation is considered finished when the controller reference count
> * first drops to 0.
> */
> - if ( new == 0 )
> + if ( new == 0 && !d->creation_finished )
> + {
> d->creation_finished = true;
> + arch_domain_creation_finished(d);
> + }
>
> domain_unpause(d);
>
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 79abcb5a63..01cd3ee4b5 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -231,6 +231,8 @@ int p2m_set_entry(struct p2m_domain *p2m,
>
> bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
>
> +void p2m_invalidate_root(struct p2m_domain *p2m);
> +
> /*
> * Clean & invalidate caches corresponding to a region [start,end) of guest
> * address space.
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 33e41486cb..d1bfc82f57 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -70,6 +70,8 @@ void arch_domain_unpause(struct domain *d);
>
> int arch_domain_soft_reset(struct domain *d);
>
> +void arch_domain_creation_finished(struct domain *d);
> +
> void arch_p2m_set_access_required(struct domain *d, bool access_required);
>
> int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations
2018-12-14 21:22 ` Stefano Stabellini
@ 2018-12-17 10:17 ` Julien Grall
2018-12-17 17:31 ` Stefano Stabellini
0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2018-12-17 10:17 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
Hi,
On 14/12/2018 21:22, Stefano Stabellini wrote:
> On Fri, 14 Dec 2018, Julien Grall wrote:
>> +
>> + /*
>> + * The full P2M may require some cleaning (e.g when emulation
>> + * set/way). As the action can take a long time, it requires
>> + * preemption. So this is deferred until we return to the guest.
>
> Please replace the last sentence of this comment with:
>
> "It is deferred until we return to guest, where we can more easily check
> for softirqs and preempt the vcpu safely."
Ok.
>
>> + */
>> + bool need_flush_to_ram;
>> +
>> } __cacheline_aligned;
>>
>> void vcpu_show_execution_state(struct vcpu *);
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index a633e27cc9..79abcb5a63 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -6,6 +6,8 @@
>> #include <xen/rwlock.h>
>> #include <xen/mem_access.h>
>>
>> +#include <asm/current.h>
>> +
>> #define paddr_bits PADDR_BITS
>>
>> /* Holds the bit size of IPAs in p2m tables. */
>> @@ -237,6 +239,12 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
>> */
>> int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
>>
>> +void p2m_set_way_flush(struct vcpu *v);
>> +
>> +void p2m_toggle_cache(struct vcpu *v, bool was_enabled);
>> +
>> +void p2m_flush_vm(struct vcpu *v);
>> +
>> /*
>> * Map a region in the guest p2m with a specific p2m type.
>> * The memory attributes will be derived from the p2m type.
>> @@ -364,6 +372,18 @@ static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
>> return -EOPNOTSUPP;
>> }
>>
>> +/*
>> + * A vCPU has cache enabled only when the MMU is enabled and data cache
>> + * is enabled.
>> + */
>> +static inline bool vcpu_has_cache_enabled(struct vcpu *v)
>> +{
>> + /* Only works with the current vCPU */
>> + ASSERT(current == v);
>> +
>> + return (READ_SYSREG32(SCTLR_EL1) & (SCTLR_C|SCTLR_M)) == (SCTLR_C|SCTLR_M);
>
> Line > 80
No, it is 79 characters (not counting \n). Why do you think it is more than 80
characters?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations
2018-12-17 10:17 ` Julien Grall
@ 2018-12-17 17:31 ` Stefano Stabellini
0 siblings, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2018-12-17 17:31 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini
On Mon, 17 Dec 2018, Julien Grall wrote:
> Hi,
>
> On 14/12/2018 21:22, Stefano Stabellini wrote:
> > On Fri, 14 Dec 2018, Julien Grall wrote:
> > > +
> > > + /*
> > > + * The full P2M may require some cleaning (e.g when emulation
> > > + * set/way). As the action can take a long time, it requires
> > > + * preemption. So this is deferred until we return to the guest.
> >
> > Please replace the last sentence of this comment with:
> >
> > "It is deferred until we return to guest, where we can more easily check
> > for softirqs and preempt the vcpu safely."
>
> Ok.
>
> >
> > > + */
> > > + bool need_flush_to_ram;
> > > +
> > > } __cacheline_aligned;
> > > void vcpu_show_execution_state(struct vcpu *);
> > > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > > index a633e27cc9..79abcb5a63 100644
> > > --- a/xen/include/asm-arm/p2m.h
> > > +++ b/xen/include/asm-arm/p2m.h
> > > @@ -6,6 +6,8 @@
> > > #include <xen/rwlock.h>
> > > #include <xen/mem_access.h>
> > > +#include <asm/current.h>
> > > +
> > > #define paddr_bits PADDR_BITS
> > > /* Holds the bit size of IPAs in p2m tables. */
> > > @@ -237,6 +239,12 @@ bool p2m_resolve_translation_fault(struct domain *d,
> > > gfn_t gfn);
> > > */
> > > int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
> > > +void p2m_set_way_flush(struct vcpu *v);
> > > +
> > > +void p2m_toggle_cache(struct vcpu *v, bool was_enabled);
> > > +
> > > +void p2m_flush_vm(struct vcpu *v);
> > > +
> > > /*
> > > * Map a region in the guest p2m with a specific p2m type.
> > > * The memory attributes will be derived from the p2m type.
> > > @@ -364,6 +372,18 @@ static inline int set_foreign_p2m_entry(struct domain
> > > *d, unsigned long gfn,
> > > return -EOPNOTSUPP;
> > > }
> > > +/*
> > > + * A vCPU has cache enabled only when the MMU is enabled and data cache
> > > + * is enabled.
> > > + */
> > > +static inline bool vcpu_has_cache_enabled(struct vcpu *v)
> > > +{
> > > + /* Only works with the current vCPU */
> > > + ASSERT(current == v);
> > > +
> > > + return (READ_SYSREG32(SCTLR_EL1) & (SCTLR_C|SCTLR_M)) ==
> > > (SCTLR_C|SCTLR_M);
> >
> > Line > 80
>
> No, it is 79 characters (not counting \n). Why do you think it is more than 80
> characters?
Weird. I must have miscounted in my reply email, where '>', '+' and tabs
increase the line count.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range
2018-12-14 21:27 ` Stefano Stabellini
@ 2018-12-18 17:47 ` Julien Grall
0 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2018-12-18 17:47 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
Hi Stefano,
On 12/14/18 9:27 PM, Stefano Stabellini wrote:
> On Fri, 14 Dec 2018, Julien Grall wrote:
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 17e2523fc1..5639e4b64c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1524,13 +1524,17 @@ int relinquish_p2m_mapping(struct domain *d)
>> return rc;
>> }
>>
>> -int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>> +int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
>> {
>> struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> gfn_t next_block_gfn;
>> + gfn_t start = *pstart;
>> mfn_t mfn = INVALID_MFN;
>> p2m_type_t t;
>> unsigned int order;
>> + int rc = 0;
>> + /* Counter for preemption */
>> + unsigned long count = 0;
>
> No need for unsigned long, count could be unsigned int or even unsigned
> short.
>
> With this change:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
I will use unsigned short and commit it.
Thank you for the review!
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations
2018-12-14 21:31 ` Andrew Cooper
@ 2018-12-18 17:58 ` Julien Grall
0 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2018-12-18 17:58 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: sstabellini
Hi Andrew,
On 12/14/18 9:31 PM, Andrew Cooper wrote:
> On 14/12/2018 03:58, Julien Grall wrote:
>> Set/Way operations are used to perform maintenance on a given cache.
>> At the moment, Set/Way operations are not trapped and therefore a guest
>> OS will directly act on the local cache. However, a vCPU may migrate to
>> another pCPU in the middle of the processor. This will result to have
>> cache with stall data (Set/Way are not propagated) potentially causing
>
> s/stall/stale/ ?
Yes. I tend to confuse the both a lot.
Cheers,
>
> ~Andrew
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-12-18 17:58 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14 11:58 [PATCH for-4.12 v3 0/5] xen/arm: Implement Set/Way operations Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 1/5] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 2/5] xen/arm: vsysreg: Add wrapper to handle sysreg " Julien Grall
2018-12-14 21:14 ` Stefano Stabellini
2018-12-14 11:58 ` [PATCH for-4.12 v3 3/5] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range Julien Grall
2018-12-14 21:27 ` Stefano Stabellini
2018-12-18 17:47 ` Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 4/5] xen/arm: Implement Set/Way operations Julien Grall
2018-12-14 21:22 ` Stefano Stabellini
2018-12-17 10:17 ` Julien Grall
2018-12-17 17:31 ` Stefano Stabellini
2018-12-14 21:31 ` Andrew Cooper
2018-12-18 17:58 ` Julien Grall
2018-12-14 11:58 ` [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of " Julien Grall
2018-12-14 21:37 ` Stefano Stabellini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).