Stefano Stabellini
2012-Nov-16 15:42 UTC
[PATCH v3] xen/arm: flush D-cache and I-cache when appropriate
- invalidate tlb after setting WXN
- flush D-cache and I-cache after relocation;
- invalidate D-cache after writing to smp_up_cpu;
- flush I-cache after changing HTTBR;
- flush I-cache and branch predictor after writing Xen text ptes.
Changes in v3:
- remove flush before HTTBR change;
- added more comments through the code;
- remove asm volatile to change HTTBR and replace it with C functions
calls and macros;
- add an isb at the beginning of flush_xen_text_tlb;
- remove isb after write_pte and before setting WXN;
- change flush_xen_dcache_va to take into account pointers that cross a
cacheline boundary.
Changes in v2:
- fix a wrong comment;
- add a comment to described why we need a DSB at the beginning of
write_pte;
- do not issue ISB within write_pte, call isb() afterwards whenever
appropriate;
- issue DSB after DCCMVAC in write_pte to make sure that the data flush
is completed before proceeding;
- make flush_xen_dcache_va take a void* as argument;
- introduce flush_xen_dcache_va_range.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
xen/arch/arm/head.S | 6 +++++
xen/arch/arm/mm.c | 21 +++++++++--------
xen/arch/arm/smpboot.c | 4 +++
xen/include/asm-arm/page.h | 53 +++++++++++++++++++++++++++++++++++++++++--
4 files changed, 71 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/head.S b/xen/arch/arm/head.S
index 3fe6412..25c4cfe 100644
--- a/xen/arch/arm/head.S
+++ b/xen/arch/arm/head.S
@@ -278,8 +278,12 @@ paging:
ldr r4, =boot_httbr /* VA of HTTBR value stashed by CPU 0 */
add r4, r4, r10 /* PA of it */
ldrd r4, r5, [r4] /* Actual value */
+ dsb
mcrr CP64(r4, r5, HTTBR)
+ dsb
+ isb
mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */
+ mcr CP32(r0, ICIALLU) /* Flush I-cache */
mcr CP32(r0, BPIALL) /* Flush branch predictor */
dsb /* Ensure completion of TLB+BP flush */
isb
@@ -292,6 +296,8 @@ paging:
teq r2, #0
bne 1b
dsb
+ mcr CP32(r0, DCCMVAC) /* flush D-Cache */
+ dsb
/* Here, the non-boot CPUs must wait again -- they''re now running on
* the boot CPU''s pagetables so it''s safe for the boot CPU
to
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index d0cd2c9..1680f4d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -244,14 +244,13 @@ void __init setup_pagetables(unsigned long
boot_phys_offset, paddr_t xen_paddr)
/* Change pagetables to the copy in the relocated Xen */
boot_httbr = (unsigned long) xen_pgtable + phys_offset;
- asm volatile (
- STORE_CP64(0, HTTBR) /* Change translation base */
- "dsb;" /* Ensure visibility of HTTBR
update */
- STORE_CP32(0, TLBIALLH) /* Flush hypervisor TLB */
- STORE_CP32(0, BPIALL) /* Flush branch predictor */
- "dsb;" /* Ensure completion of TLB+BP
flush */
- "isb;"
- : : "r" (boot_httbr) : "memory");
+ flush_xen_dcache_va(&boot_httbr);
+ flush_xen_dcache_va_range((void*)dest_va, _end - _start);
+ flush_xen_text_tlb();
+
+ WRITE_CP64(boot_httbr, HTTBR); /* Change translation base */
+ dsb(); /* Ensure visibility of HTTBR update */
+ flush_xen_text_tlb();
/* Undo the temporary map */
pte.bits = 0;
@@ -291,11 +290,12 @@ void __init setup_pagetables(unsigned long
boot_phys_offset, paddr_t xen_paddr)
>> PAGE_SHIFT);
pte.pt.table = 1;
write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
- /* Have changed a mapping used for .text. Flush everything for safety. */
- flush_xen_text_tlb();
+ /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */
/* From now on, no mapping may be both writable and executable. */
WRITE_CP32(READ_CP32(HSCTLR) | SCTLR_WXN, HSCTLR);
+ /* Flush everything after setting WXN bit. */
+ flush_xen_text_tlb();
}
/* MMU setup for secondary CPUS (which already have paging enabled) */
@@ -303,6 +303,7 @@ void __cpuinit mmu_init_secondary_cpu(void)
{
/* From now on, no mapping may be both writable and executable. */
WRITE_CP32(READ_CP32(HSCTLR) | SCTLR_WXN, HSCTLR);
+ flush_xen_text_tlb();
}
/* Create Xen''s mappings of memory.
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index c0750c0..1b52e22 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -105,6 +105,7 @@ make_cpus_ready(unsigned int max_cpus, unsigned long
boot_phys_offset)
/* Tell the next CPU to get ready */
/* TODO: handle boards where CPUIDs are not contiguous */
*gate = i;
+ flush_xen_dcache_va(gate);
asm volatile("dsb; isb; sev");
/* And wait for it to respond */
while ( ready_cpus < i )
@@ -201,6 +202,9 @@ int __cpu_up(unsigned int cpu)
/* Unblock the CPU. It should be waiting in the loop in head.S
* for an event to arrive when smp_up_cpu matches its cpuid. */
smp_up_cpu = cpu;
+ /* we need to make sure that the change to smp_up_cpu is visible to
+ * secondary cpus with D-cache off */
+ flush_xen_dcache_va(&smp_up_cpu);
asm volatile("dsb; isb; sev");
while ( !cpu_online(cpu) )
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 9511c45..3e77281 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -228,27 +228,74 @@ static inline lpae_t mfn_to_p2m_entry(unsigned long mfn,
unsigned int mattr)
return e;
}
-/* Write a pagetable entry */
+/* Write a pagetable entry.
+ *
+ * If the table entry is changing a text mapping, it is responsibility
+ * of the caller to issue an ISB after write_pte.
+ */
static inline void write_pte(lpae_t *p, lpae_t pte)
{
asm volatile (
+ /* Ensure any writes have completed with the old mappings. */
+ "dsb;"
/* Safely write the entry (STRD is atomic on CPUs that support LPAE) */
"strd %0, %H0, [%1];"
+ "dsb;"
/* Push this cacheline to the PoC so the rest of the system sees it. */
STORE_CP32(1, DCCMVAC)
+ /* Ensure that the data flush is completed before proceeding */
+ "dsb;"
: : "r" (pte.bits), "r" (p) : "memory");
}
+
+/* Function for flushing medium-sized areas.
+ * if ''range'' is large enough we might want to use
model-specific
+ * full-cache flushes. */
+static inline void flush_xen_dcache_va_range(void *p, unsigned long size)
+{
+ int cacheline_bytes = READ_CP32(CCSIDR);
+ void *end;
+ dsb(); /* So the CPU issues all writes to the range */
+ for ( end = p + size; p < end; p += cacheline_bytes )
+ WRITE_CP32((uint32_t) p, DCCMVAC);
+ dsb(); /* So we know the flushes happen before continuing */
+}
+
+
+/* Macro for flushing a single small item. The predicate is always
+ * compile-time constant so this will compile down to 3 instructions in
+ * the common case. Make sure to call it with the correct type of
+ * pointer! */
+#define flush_xen_dcache_va(p) do { \
+ int cacheline_bytes = READ_CP32(CCSIDR); \
+ typeof(p) _p = (p); \
+ if ( ((unsigned long)_p & ~(cacheline_bytes - 1)) != \
+ (((unsigned long)_p + (sizeof *_p)) & ~(cacheline_bytes - 1)) ) \
+ flush_xen_dcache_va_range(_p, sizeof *_p); \
+ else \
+ asm volatile ( \
+ "dsb;" /* Finish all earlier writes */
\
+ STORE_CP32(0, DCCMVAC) \
+ "dsb;" /* Finish flush before continuing */
\
+ : : "r" (_p), "m" (*_p));
\
+} while (0)
+
+
/*
* Flush all hypervisor mappings from the TLB and branch predictor.
- * This is needed after changing Xen code mappings.
+ * This is needed after changing Xen code mappings.
+ *
+ * The caller needs to issue the necessary DSB and D-cache flushes
+ * before calling flush_xen_text_tlb.
*/
static inline void flush_xen_text_tlb(void)
{
register unsigned long r0 asm ("r0");
asm volatile (
- "dsb;" /* Ensure visibility of PTE
writes */
+ "isb;" /* Ensure synchronization with
previous changes to text */
STORE_CP32(0, TLBIALLH) /* Flush hypervisor TLB */
+ STORE_CP32(0, ICIALLU) /* Flush I-cache */
STORE_CP32(0, BPIALL) /* Flush branch predictor */
"dsb;" /* Ensure completion of TLB+BP
flush */
"isb;"
--
1.7.2.5
Ian Campbell
2012-Nov-19 12:55 UTC
Re: [PATCH v3] xen/arm: flush D-cache and I-cache when appropriate
On Fri, 2012-11-16 at 15:42 +0000, Stefano Stabellini wrote:> - invalidate tlb after setting WXN > - flush D-cache and I-cache after relocation; > - invalidate D-cache after writing to smp_up_cpu; > - flush I-cache after changing HTTBR; > - flush I-cache and branch predictor after writing Xen text ptes. > > > Changes in v3: > - remove flush before HTTBR change; > - added more comments through the code; > - remove asm volatile to change HTTBR and replace it with C functions > calls and macros; > - add an isb at the beginning of flush_xen_text_tlb; > - remove isb after write_pte and before setting WXN; > - change flush_xen_dcache_va to take into account pointers that cross a > cacheline boundary. > > Changes in v2: > - fix a wrong comment; > - add a comment to described why we need a DSB at the beginning of > write_pte; > - do not issue ISB within write_pte, call isb() afterwards whenever > appropriate; > - issue DSB after DCCMVAC in write_pte to make sure that the data flush > is completed before proceeding; > - make flush_xen_dcache_va take a void* as argument; > - introduce flush_xen_dcache_va_range. > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>Acked-by: Ian Campbell <Ian.campbell@citrix.com> and applied, thanks,> --- > xen/arch/arm/head.S | 6 +++++ > xen/arch/arm/mm.c | 21 +++++++++-------- > xen/arch/arm/smpboot.c | 4 +++ > xen/include/asm-arm/page.h | 53 +++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 71 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/head.S b/xen/arch/arm/head.S > index 3fe6412..25c4cfe 100644 > --- a/xen/arch/arm/head.S > +++ b/xen/arch/arm/head.S > @@ -278,8 +278,12 @@ paging: > ldr r4, =boot_httbr /* VA of HTTBR value stashed by CPU 0 */ > add r4, r4, r10 /* PA of it */ > ldrd r4, r5, [r4] /* Actual value */ > + dsb > mcrr CP64(r4, r5, HTTBR) > + dsb > + isb > mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */ > + mcr CP32(r0, ICIALLU) /* Flush I-cache */ > mcr CP32(r0, BPIALL) /* Flush branch predictor */ > dsb /* Ensure completion of TLB+BP flush */ > isb > @@ -292,6 +296,8 @@ paging: > teq r2, #0 > bne 1b > dsb > + mcr CP32(r0, DCCMVAC) /* flush D-Cache */ > + dsb > > /* Here, the non-boot CPUs must wait again -- they''re now running on > * the boot CPU''s pagetables so it''s safe for the boot CPU to > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index d0cd2c9..1680f4d 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -244,14 +244,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) > > /* Change pagetables to the copy in the relocated Xen */ > boot_httbr = (unsigned long) xen_pgtable + phys_offset; > - asm volatile ( > - STORE_CP64(0, HTTBR) /* Change translation base */ > - "dsb;" /* Ensure visibility of HTTBR update */ > - STORE_CP32(0, TLBIALLH) /* Flush hypervisor TLB */ > - STORE_CP32(0, BPIALL) /* Flush branch predictor */ > - "dsb;" /* Ensure completion of TLB+BP flush */ > - "isb;" > - : : "r" (boot_httbr) : "memory"); > + flush_xen_dcache_va(&boot_httbr); > + flush_xen_dcache_va_range((void*)dest_va, _end - _start); > + flush_xen_text_tlb(); > + > + WRITE_CP64(boot_httbr, HTTBR); /* Change translation base */ > + dsb(); /* Ensure visibility of HTTBR update */ > + flush_xen_text_tlb(); > > /* Undo the temporary map */ > pte.bits = 0; > @@ -291,11 +290,12 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) > >> PAGE_SHIFT); > pte.pt.table = 1; > write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte); > - /* Have changed a mapping used for .text. Flush everything for safety. */ > - flush_xen_text_tlb(); > + /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */ > > /* From now on, no mapping may be both writable and executable. */ > WRITE_CP32(READ_CP32(HSCTLR) | SCTLR_WXN, HSCTLR); > + /* Flush everything after setting WXN bit. */ > + flush_xen_text_tlb(); > } > > /* MMU setup for secondary CPUS (which already have paging enabled) */ > @@ -303,6 +303,7 @@ void __cpuinit mmu_init_secondary_cpu(void) > { > /* From now on, no mapping may be both writable and executable. */ > WRITE_CP32(READ_CP32(HSCTLR) | SCTLR_WXN, HSCTLR); > + flush_xen_text_tlb(); > } > > /* Create Xen''s mappings of memory. > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index c0750c0..1b52e22 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -105,6 +105,7 @@ make_cpus_ready(unsigned int max_cpus, unsigned long boot_phys_offset) > /* Tell the next CPU to get ready */ > /* TODO: handle boards where CPUIDs are not contiguous */ > *gate = i; > + flush_xen_dcache_va(gate); > asm volatile("dsb; isb; sev"); > /* And wait for it to respond */ > while ( ready_cpus < i ) > @@ -201,6 +202,9 @@ int __cpu_up(unsigned int cpu) > /* Unblock the CPU. It should be waiting in the loop in head.S > * for an event to arrive when smp_up_cpu matches its cpuid. */ > smp_up_cpu = cpu; > + /* we need to make sure that the change to smp_up_cpu is visible to > + * secondary cpus with D-cache off */ > + flush_xen_dcache_va(&smp_up_cpu); > asm volatile("dsb; isb; sev"); > > while ( !cpu_online(cpu) ) > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > index 9511c45..3e77281 100644 > --- a/xen/include/asm-arm/page.h > +++ b/xen/include/asm-arm/page.h > @@ -228,27 +228,74 @@ static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr) > return e; > } > > -/* Write a pagetable entry */ > +/* Write a pagetable entry. > + * > + * If the table entry is changing a text mapping, it is responsibility > + * of the caller to issue an ISB after write_pte. > + */ > static inline void write_pte(lpae_t *p, lpae_t pte) > { > asm volatile ( > + /* Ensure any writes have completed with the old mappings. */ > + "dsb;" > /* Safely write the entry (STRD is atomic on CPUs that support LPAE) */ > "strd %0, %H0, [%1];" > + "dsb;" > /* Push this cacheline to the PoC so the rest of the system sees it. */ > STORE_CP32(1, DCCMVAC) > + /* Ensure that the data flush is completed before proceeding */ > + "dsb;" > : : "r" (pte.bits), "r" (p) : "memory"); > } > > + > +/* Function for flushing medium-sized areas. > + * if ''range'' is large enough we might want to use model-specific > + * full-cache flushes. */ > +static inline void flush_xen_dcache_va_range(void *p, unsigned long size) > +{ > + int cacheline_bytes = READ_CP32(CCSIDR); > + void *end; > + dsb(); /* So the CPU issues all writes to the range */ > + for ( end = p + size; p < end; p += cacheline_bytes ) > + WRITE_CP32((uint32_t) p, DCCMVAC); > + dsb(); /* So we know the flushes happen before continuing */ > +} > + > + > +/* Macro for flushing a single small item. The predicate is always > + * compile-time constant so this will compile down to 3 instructions in > + * the common case. Make sure to call it with the correct type of > + * pointer! */ > +#define flush_xen_dcache_va(p) do { \ > + int cacheline_bytes = READ_CP32(CCSIDR); \ > + typeof(p) _p = (p); \ > + if ( ((unsigned long)_p & ~(cacheline_bytes - 1)) != \ > + (((unsigned long)_p + (sizeof *_p)) & ~(cacheline_bytes - 1)) ) \ > + flush_xen_dcache_va_range(_p, sizeof *_p); \ > + else \ > + asm volatile ( \ > + "dsb;" /* Finish all earlier writes */ \ > + STORE_CP32(0, DCCMVAC) \ > + "dsb;" /* Finish flush before continuing */ \ > + : : "r" (_p), "m" (*_p)); \ > +} while (0) > + > + > /* > * Flush all hypervisor mappings from the TLB and branch predictor. > - * This is needed after changing Xen code mappings. > + * This is needed after changing Xen code mappings. > + * > + * The caller needs to issue the necessary DSB and D-cache flushes > + * before calling flush_xen_text_tlb. > */ > static inline void flush_xen_text_tlb(void) > { > register unsigned long r0 asm ("r0"); > asm volatile ( > - "dsb;" /* Ensure visibility of PTE writes */ > + "isb;" /* Ensure synchronization with previous changes to text */ > STORE_CP32(0, TLBIALLH) /* Flush hypervisor TLB */ > + STORE_CP32(0, ICIALLU) /* Flush I-cache */ > STORE_CP32(0, BPIALL) /* Flush branch predictor */ > "dsb;" /* Ensure completion of TLB+BP flush */ > "isb;"
Tim Deegan
2012-Nov-19 13:08 UTC
Re: [PATCH v3] xen/arm: flush D-cache and I-cache when appropriate
Sorry for the late review - I meant to get to these on Thursday but hadn''t time. At 12:55 +0000 on 19 Nov (1353329757), Ian Campbell wrote:> > +/* Macro for flushing a single small item. The predicate is always > > + * compile-time constant so this will compile down to 3 instructions in > > + * the common case.This is no longer true! The predicate now depends on the alignment of the pointer, so this will compile to quite a bit of code, including the memory barrier I was trying to avoid. If we can sensibly restrict this to naturally-aligned objects smaller than a cacheline, we should go back to the previous version of this macro. If not, I think we might as well just use flush_xen_dcache_va_range(_p, sizeof *_p) and be done with it. Tim.> > Make sure to call it with the correct type of > > + * pointer! */ > > +#define flush_xen_dcache_va(p) do { \ > > + int cacheline_bytes = READ_CP32(CCSIDR); \ > > + typeof(p) _p = (p); \ > > + if ( ((unsigned long)_p & ~(cacheline_bytes - 1)) != \ > > + (((unsigned long)_p + (sizeof *_p)) & ~(cacheline_bytes - 1)) ) \ > > + flush_xen_dcache_va_range(_p, sizeof *_p); \ > > + else \ > > + asm volatile ( \ > > + "dsb;" /* Finish all earlier writes */ \ > > + STORE_CP32(0, DCCMVAC) \ > > + "dsb;" /* Finish flush before continuing */ \ > > + : : "r" (_p), "m" (*_p)); \ > > +} while (0) > > + > > + > > /* > > * Flush all hypervisor mappings from the TLB and branch predictor. > > - * This is needed after changing Xen code mappings. > > + * This is needed after changing Xen code mappings. > > + * > > + * The caller needs to issue the necessary DSB and D-cache flushes > > + * before calling flush_xen_text_tlb. > > */ > > static inline void flush_xen_text_tlb(void) > > { > > register unsigned long r0 asm ("r0"); > > asm volatile ( > > - "dsb;" /* Ensure visibility of PTE writes */ > > + "isb;" /* Ensure synchronization with previous changes to text */ > > STORE_CP32(0, TLBIALLH) /* Flush hypervisor TLB */ > > + STORE_CP32(0, ICIALLU) /* Flush I-cache */ > > STORE_CP32(0, BPIALL) /* Flush branch predictor */ > > "dsb;" /* Ensure completion of TLB+BP flush */ > > "isb;" > >
Ian Campbell
2012-Nov-19 13:56 UTC
Re: [PATCH v3] xen/arm: flush D-cache and I-cache when appropriate
On Mon, 2012-11-19 at 13:08 +0000, Tim Deegan wrote:> Sorry for the late review - I meant to get to these on Thursday but > hadn''t time. > > At 12:55 +0000 on 19 Nov (1353329757), Ian Campbell wrote: > > > +/* Macro for flushing a single small item. The predicate is always > > > + * compile-time constant so this will compile down to 3 instructions in > > > + * the common case. > > This is no longer true! The predicate now depends on the alignment of > the pointer, so this will compile to quite a bit of code, including the > memory barrier I was trying to avoid.If p is static then isn''t p + sizeof(*p) etc static and therefore the compiler can figure it out? Stefano, what does it actually compile down to in practice?> If we can sensibly restrict this to naturally-aligned objects smaller > than a cacheline, we should go back to the previous version of this > macro. If not, I think we might as well just use > flush_xen_dcache_va_range(_p, sizeof *_p) and be done with it.I''ve already committed it, so please send a followup patch of whichever sort is needed. Ian.
Tim Deegan
2012-Nov-19 14:43 UTC
Re: [PATCH v3] xen/arm: flush D-cache and I-cache when appropriate
At 13:56 +0000 on 19 Nov (1353333381), Ian Campbell wrote:> On Mon, 2012-11-19 at 13:08 +0000, Tim Deegan wrote: > > Sorry for the late review - I meant to get to these on Thursday but > > hadn''t time. > > > > At 12:55 +0000 on 19 Nov (1353329757), Ian Campbell wrote: > > > > +/* Macro for flushing a single small item. The predicate is always > > > > + * compile-time constant so this will compile down to 3 instructions in > > > > + * the common case. > > > > This is no longer true! The predicate now depends on the alignment of > > the pointer, so this will compile to quite a bit of code, including the > > memory barrier I was trying to avoid. > > If p is static then isn''t p + sizeof(*p) etc static and therefore the > compiler can figure it out?No. Cacheline_bytes isn''t known until run time, and even if p is static, its value isn''t known until link time. :(> Stefano, what does it actually compile down to in practice? > > > If we can sensibly restrict this to naturally-aligned objects smaller > > than a cacheline, we should go back to the previous version of this > > macro. If not, I think we might as well just use > > flush_xen_dcache_va_range(_p, sizeof *_p) and be done with it. > > I''ve already committed it, so please send a followup patch of whichever > sort is needed.Will do. Again, probably Thursday before I can get to it. Tim.