xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4 v2] xen: arm: smp & tlb cleanups
@ 2014-03-17 13:51 Ian Campbell
  2014-03-17 13:52 ` [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions Ian Campbell
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 13:51 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, Julien Grall, Tim Deegan

A bit of a grab bag, most of this has been posted individually before
either during the 4.4 freeze or sooner. I've called it v2 just to avoid
confusion with those individual postings, there wasn't a single v1.

Ian.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions
  2014-03-17 13:51 [PATCH 0/4 v2] xen: arm: smp & tlb cleanups Ian Campbell
@ 2014-03-17 13:52 ` Ian Campbell
  2014-03-17 14:10   ` Julien Grall
  2014-03-19 21:10   ` Julien Grall
  2014-03-17 13:52 ` [PATCH 2/4] xen: arm: flush TLB on all CPUs when setting or clearing fixmaps Ian Campbell
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 13:52 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini

All of the flush_xen_*_tlb functions operate on the local processor only. Add
_local to the name and update the comments to clarify.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/mm.c                |   22 +++++++++++-----------
 xen/include/asm-arm/arm32/page.h |   21 +++++++++++++--------
 xen/include/asm-arm/arm64/page.h |   20 ++++++++++++--------
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 305879f..fb1bbf2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -217,7 +217,7 @@ void set_fixmap(unsigned map, unsigned long mfn, unsigned attributes)
     pte.pt.ai = attributes;
     pte.pt.xn = 1;
     write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
-    flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+    flush_xen_data_tlb_local_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
 }
 
 /* Remove a mapping from a fixmap entry */
@@ -225,7 +225,7 @@ void clear_fixmap(unsigned map)
 {
     lpae_t pte = {0};
     write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
-    flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+    flush_xen_data_tlb_local_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
 }
 
 #ifdef CONFIG_DOMAIN_PAGE
@@ -303,7 +303,7 @@ void *map_domain_page(unsigned long mfn)
      * We may not have flushed this specific subpage at map time,
      * since we only flush the 4k page not the superpage
      */
-    flush_xen_data_tlb_range_va(va, PAGE_SIZE);
+    flush_xen_data_tlb_local_range_va(va, PAGE_SIZE);
 
     return (void *)va;
 }
@@ -404,7 +404,7 @@ void __init remove_early_mappings(void)
 {
     lpae_t pte = {0};
     write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
-    flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
+    flush_xen_data_tlb_local_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
 }
 
 extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
@@ -468,7 +468,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     dest_va = BOOT_RELOC_VIRT_START;
     pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT);
     write_pte(boot_second + second_table_offset(dest_va), pte);
-    flush_xen_data_tlb_range_va(dest_va, SECOND_SIZE);
+    flush_xen_data_tlb_local_range_va(dest_va, SECOND_SIZE);
 #ifdef CONFIG_ARM_64
     ttbr = (uintptr_t) xen_pgtable + phys_offset;
 #else
@@ -516,7 +516,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     /* From now on, no mapping may be both writable and executable. */
     WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
     /* Flush everything after setting WXN bit. */
-    flush_xen_text_tlb();
+    flush_xen_text_tlb_local();
 
 #ifdef CONFIG_ARM_32
     per_cpu(xen_pgtable, 0) = cpu0_pgtable;
@@ -589,7 +589,7 @@ void __cpuinit mmu_init_secondary_cpu(void)
 {
     /* From now on, no mapping may be both writable and executable. */
     WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
-    flush_xen_text_tlb();
+    flush_xen_text_tlb_local();
 }
 
 /* Create Xen's mappings of memory.
@@ -617,7 +617,7 @@ static void __init create_32mb_mappings(lpae_t *second,
         write_pte(p + i, pte);
         pte.pt.base += 1 << LPAE_SHIFT;
     }
-    flush_xen_data_tlb();
+    flush_xen_data_tlb_local();
 }
 
 #ifdef CONFIG_ARM_32
@@ -696,7 +696,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
         vaddr += FIRST_SIZE;
     }
 
-    flush_xen_data_tlb();
+    flush_xen_data_tlb_local();
 }
 #endif
 
@@ -841,7 +841,7 @@ static int create_xen_entries(enum xenmap_operation op,
                 BUG();
         }
     }
-    flush_xen_data_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
+    flush_xen_data_tlb_local_range_va(virt, PAGE_SIZE * nr_mfns);
 
     rc = 0;
 
@@ -904,7 +904,7 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
         }
         write_pte(xen_xenmap + i, pte);
     }
-    flush_xen_text_tlb();
+    flush_xen_text_tlb_local();
 }
 
 /* Release all __init and __initdata ranges to be reused */
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index b8221ca..daa4931 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -27,13 +27,15 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
 #define __clean_and_invalidate_xen_dcache_one(R) STORE_CP32(R, DCCIMVAC)
 
 /*
- * Flush all hypervisor mappings from the TLB and branch predictor.
+ * Flush all hypervisor mappings from the TLB and branch predictor of
+ * the local processor.
+ *
  * 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)
+static inline void flush_xen_text_tlb_local(void)
 {
     register unsigned long r0 asm ("r0");
     asm volatile (
@@ -47,10 +49,11 @@ static inline void flush_xen_text_tlb(void)
 }
 
 /*
- * Flush all hypervisor mappings from the data TLB. This is not
- * sufficient when changing code mappings or for self modifying code.
+ * Flush all hypervisor mappings from the data TLB of the local
+ * processor. This is not sufficient when changing code mappings or
+ * for self modifying code.
  */
-static inline void flush_xen_data_tlb(void)
+static inline void flush_xen_data_tlb_local(void)
 {
     register unsigned long r0 asm ("r0");
     asm volatile("dsb;" /* Ensure preceding are visible */
@@ -61,10 +64,12 @@ static inline void flush_xen_data_tlb(void)
 }
 
 /*
- * Flush a range of VA's hypervisor mappings from the data TLB. This is not
- * sufficient when changing code mappings or for self modifying code.
+ * Flush a range of VA's hypervisor mappings from the data TLB of the
+ * local processor. This is not sufficient when changing code mappings
+ * or for self modifying code.
  */
-static inline void flush_xen_data_tlb_range_va(unsigned long va, unsigned long size)
+static inline void flush_xen_data_tlb_local_range_va(unsigned long va,
+                                                     unsigned long size)
 {
     unsigned long end = va + size;
     dsb(); /* Ensure preceding are visible */
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index 3352821..9bea476 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -22,13 +22,14 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
 #define __clean_and_invalidate_xen_dcache_one(R) "dc  civac, %" #R ";"
 
 /*
- * Flush all hypervisor mappings from the TLB
+ * Flush all hypervisor mappings from the TLB of the local processor.
+ *
  * 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)
+static inline void flush_xen_text_tlb_local(void)
 {
     asm volatile (
         "isb;"       /* Ensure synchronization with previous changes to text */
@@ -40,10 +41,11 @@ static inline void flush_xen_text_tlb(void)
 }
 
 /*
- * Flush all hypervisor mappings from the data TLB. This is not
- * sufficient when changing code mappings or for self modifying code.
+ * Flush all hypervisor mappings from the data TLB of the local
+ * processor. This is not sufficient when changing code mappings or
+ * for self modifying code.
  */
-static inline void flush_xen_data_tlb(void)
+static inline void flush_xen_data_tlb_local(void)
 {
     asm volatile (
         "dsb    sy;"                    /* Ensure visibility of PTE writes */
@@ -54,10 +56,12 @@ static inline void flush_xen_data_tlb(void)
 }
 
 /*
- * Flush a range of VA's hypervisor mappings from the data TLB. This is not
- * sufficient when changing code mappings or for self modifying code.
+ * Flush a range of VA's hypervisor mappings from the data TLB of the
+ * local processor. This is not sufficient when changing code mappings
+ * or for self modifying code.
  */
-static inline void flush_xen_data_tlb_range_va(unsigned long va, unsigned long size)
+static inline void flush_xen_data_tlb_local_range_va(unsigned long va,
+                                                     unsigned long size)
 {
     unsigned long end = va + size;
     dsb(); /* Ensure preceding are visible */
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] xen: arm: flush TLB on all CPUs when setting or clearing fixmaps
  2014-03-17 13:51 [PATCH 0/4 v2] xen: arm: smp & tlb cleanups Ian Campbell
  2014-03-17 13:52 ` [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions Ian Campbell
@ 2014-03-17 13:52 ` Ian Campbell
  2014-03-17 13:52 ` [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up Ian Campbell
  2014-03-17 13:52 ` [PATCH 4/4] xen: arm32: don't force the compiler to allocate a dummy register Ian Campbell
  3 siblings, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 13:52 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini

These mappings are global and therefore need flushing on all processors. Add
flush_all_xen_data_tlb_range_va which accomplishes this.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/mm.c                |    4 ++--
 xen/include/asm-arm/arm32/page.h |   19 +++++++++++++++++++
 xen/include/asm-arm/arm64/page.h |   19 +++++++++++++++++++
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fb1bbf2..b7a8df0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -217,7 +217,7 @@ void set_fixmap(unsigned map, unsigned long mfn, unsigned attributes)
     pte.pt.ai = attributes;
     pte.pt.xn = 1;
     write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
-    flush_xen_data_tlb_local_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+    flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
 }
 
 /* Remove a mapping from a fixmap entry */
@@ -225,7 +225,7 @@ void clear_fixmap(unsigned map)
 {
     lpae_t pte = {0};
     write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
-    flush_xen_data_tlb_local_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+    flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
 }
 
 #ifdef CONFIG_DOMAIN_PAGE
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index daa4931..27027d8 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -82,6 +82,25 @@ static inline void flush_xen_data_tlb_local_range_va(unsigned long va,
     isb();
 }
 
+/*
+ * Flush a range of VA's hypervisor mappings from the data TLB on all
+ * processors in the inner-shareable domain. This is not sufficient
+ * when changing code mappings or for self modifying code.
+ */
+static inline void flush_xen_data_tlb_range_va(unsigned long va,
+                                               unsigned long size)
+{
+    unsigned long end = va + size;
+    dsb(); /* Ensure preceding are visible */
+    while ( va < end ) {
+        asm volatile(STORE_CP32(0, TLBIMVAHIS)
+                     : : "r" (va) : "memory");
+        va += PAGE_SIZE;
+    }
+    dsb(); /* Ensure completion of the TLB flush */
+    isb();
+}
+
 /* Ask the MMU to translate a VA for us */
 static inline uint64_t __va_to_par(vaddr_t va)
 {
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index 9bea476..61a3e6c 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -74,6 +74,25 @@ static inline void flush_xen_data_tlb_local_range_va(unsigned long va,
     isb();
 }
 
+/*
+ * Flush a range of VA's hypervisor mappings from the data TLB of all
+ * processors in the inner-shareable domain. This is not sufficient
+ * when changing code mappings or for self modifying code.
+ */
+static inline void flush_xen_data_tlb_range_va(unsigned long va,
+                                               unsigned long size)
+{
+    unsigned long end = va + size;
+    dsb(); /* Ensure preceding are visible */
+    while ( va < end ) {
+        asm volatile("tlbi vae2is, %0;"
+                     : : "r" (va>>PAGE_SHIFT) : "memory");
+        va += PAGE_SIZE;
+    }
+    dsb(); /* Ensure completion of the TLB flush */
+    isb();
+}
+
 /* Ask the MMU to translate a VA for us */
 static inline uint64_t __va_to_par(vaddr_t va)
 {
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up
  2014-03-17 13:51 [PATCH 0/4 v2] xen: arm: smp & tlb cleanups Ian Campbell
  2014-03-17 13:52 ` [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions Ian Campbell
  2014-03-17 13:52 ` [PATCH 2/4] xen: arm: flush TLB on all CPUs when setting or clearing fixmaps Ian Campbell
@ 2014-03-17 13:52 ` Ian Campbell
  2014-03-17 14:18   ` Julien Grall
  2014-04-02 14:03   ` Ian Campbell
  2014-03-17 13:52 ` [PATCH 4/4] xen: arm32: don't force the compiler to allocate a dummy register Ian Campbell
  3 siblings, 2 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 13:52 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini

This ensures that the writeq to the release address has occurred.

In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but
make it explicit.

The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the
only other uses are in the v7 spinlock code which includes a dsb() already.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: typos in the commit message
---
 xen/arch/arm/arm64/smpboot.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 9146476..8e2ecf2 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -36,6 +36,7 @@ static int __init smp_spin_table_cpu_up(int cpu)
 
     iounmap(release);
 
+    dsb(); /* Ensure that the write to cpu_release_addr has occurred. */
     sev();
 
     return cpu_up_send_sgi(cpu);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] xen: arm32: don't force the compiler to allocate a dummy register
  2014-03-17 13:51 [PATCH 0/4 v2] xen: arm: smp & tlb cleanups Ian Campbell
                   ` (2 preceding siblings ...)
  2014-03-17 13:52 ` [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up Ian Campbell
@ 2014-03-17 13:52 ` Ian Campbell
  2014-03-17 14:33   ` Julien Grall
  3 siblings, 1 reply; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 13:52 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini

TLBIALLH, ICIALLU and BPIALL make no use of their register argument. Instead
of making the compiler allocate a dummy register just hardcode r0, there is no
need to represent this in the inline asm since the register is neither
clobbered nor used in any way.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/arm32/page.h      |   14 ++++++--------
 xen/include/asm-arm/arm32/processor.h |    4 ++++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 27027d8..beebe8b 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -37,15 +37,14 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
  */
 static inline void flush_xen_text_tlb_local(void)
 {
-    register unsigned long r0 asm ("r0");
     asm volatile (
         "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 */
+        CMD_CP32(TLBIALLH)            /* Flush hypervisor TLB */
+        CMD_CP32(ICIALLU)             /* Flush I-cache */
+        CMD_CP32(BPIALL)              /* Flush branch predictor */
         "dsb;"                        /* Ensure completion of TLB+BP flush */
         "isb;"
-        : : "r" (r0) /*dummy*/ : "memory");
+        : : : "memory");
 }
 
 /*
@@ -55,12 +54,11 @@ static inline void flush_xen_text_tlb_local(void)
  */
 static inline void flush_xen_data_tlb_local(void)
 {
-    register unsigned long r0 asm ("r0");
     asm volatile("dsb;" /* Ensure preceding are visible */
-                 STORE_CP32(0, TLBIALLH)
+                 CMD_CP32(TLBIALLH)
                  "dsb;" /* Ensure completion of the TLB flush */
                  "isb;"
-                 : : "r" (r0) /* dummy */: "memory");
+                 : : : "memory");
 }
 
 /*
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index 8a35cee..f41644d 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -69,6 +69,10 @@ struct cpu_user_regs
 #define LOAD_CP64(r, name...)  "mrrc " __stringify(CP64(%r, %H##r, name)) ";"
 #define STORE_CP64(r, name...) "mcrr " __stringify(CP64(%r, %H##r, name)) ";"
 
+/* Issue a CP operation which takes no argument,
+ * uses r0 as a placeholder register. */
+#define CMD_CP32(name...)      "mcr " __stringify(CP32(r0, name)) ";"
+
 #ifndef __ASSEMBLY__
 
 /* C wrappers */
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions
  2014-03-17 13:52 ` [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions Ian Campbell
@ 2014-03-17 14:10   ` Julien Grall
  2014-03-19 21:10   ` Julien Grall
  1 sibling, 0 replies; 13+ messages in thread
From: Julien Grall @ 2014-03-17 14:10 UTC (permalink / raw)
  To: Ian Campbell; +Cc: stefano.stabellini, tim, xen-devel

Hi Ian,

On 03/17/2014 01:52 PM, Ian Campbell wrote:
> All of the flush_xen_*_tlb functions operate on the local processor only. Add
> _local to the name and update the comments to clarify.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> -static inline void flush_xen_data_tlb_range_va(unsigned long va, unsigned long size)
> +static inline void flush_xen_data_tlb_local_range_va(unsigned long va,

I think it's more consistent to have local at the end of the function name.

It's what is done for flush_tlb_all_local.

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up
  2014-03-17 13:52 ` [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up Ian Campbell
@ 2014-03-17 14:18   ` Julien Grall
  2014-03-17 14:39     ` Ian Campbell
  2014-04-02 14:03   ` Ian Campbell
  1 sibling, 1 reply; 13+ messages in thread
From: Julien Grall @ 2014-03-17 14:18 UTC (permalink / raw)
  To: Ian Campbell; +Cc: stefano.stabellini, tim, xen-devel

Hi Ian,

On 03/17/2014 01:52 PM, Ian Campbell wrote:
> This ensures that the writeq to the release address has occurred.
> 
> In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but
> make it explicit.

There other place (see xen/arch/arm/platforms/exynos5.c) where we rely
on the dsb in write_pte. I think we should move the dsb in iounmap().

> 
> The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the
> only other uses are in the v7 spinlock code which includes a dsb() already.

Would it be better to add dsb in the sev() macro?

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] xen: arm32: don't force the compiler to allocate a dummy register
  2014-03-17 13:52 ` [PATCH 4/4] xen: arm32: don't force the compiler to allocate a dummy register Ian Campbell
@ 2014-03-17 14:33   ` Julien Grall
  0 siblings, 0 replies; 13+ messages in thread
From: Julien Grall @ 2014-03-17 14:33 UTC (permalink / raw)
  To: Ian Campbell; +Cc: stefano.stabellini, tim, xen-devel

Hi Ian,

On 03/17/2014 01:52 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/include/asm-arm/arm32/page.h      |   14 ++++++--------
>  xen/include/asm-arm/arm32/processor.h |    4 ++++
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
> index 27027d8..beebe8b 100644
> --- a/xen/include/asm-arm/arm32/page.h
> +++ b/xen/include/asm-arm/arm32/page.h
> @@ -37,15 +37,14 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
>   */
>  static inline void flush_xen_text_tlb_local(void)
>  {
> -    register unsigned long r0 asm ("r0");

Good to see it's removed :). It was a GNU C extension and preventing Xen
to build on clang.

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

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up
  2014-03-17 14:18   ` Julien Grall
@ 2014-03-17 14:39     ` Ian Campbell
  2014-03-17 14:43       ` Julien Grall
  0 siblings, 1 reply; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 14:39 UTC (permalink / raw)
  To: Julien Grall; +Cc: stefano.stabellini, tim, xen-devel

On Mon, 2014-03-17 at 14:18 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/17/2014 01:52 PM, Ian Campbell wrote:
> > This ensures that the writeq to the release address has occurred.
> > 
> > In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but
> > make it explicit.
> 
> There other place (see xen/arch/arm/platforms/exynos5.c) where we rely
> on the dsb in write_pte. I think we should move the dsb in iounmap().

Now you put it like that it does seem logical that any writes would be
issued prior to/as part of the iounmap.

In that context I think it would in fact be fine for iounmap to rely on
the dsb in write_pte since both a "change the mappings" functions. IOW
maybe we don't need to do anything after all.

> > The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the
> > only other uses are in the v7 spinlock code which includes a dsb() already.
> 
> Would it be better to add dsb in the sev() macro?

ARM32 has a dsb_sev macro, maybe we should do the same for ARM64.

Ian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up
  2014-03-17 14:39     ` Ian Campbell
@ 2014-03-17 14:43       ` Julien Grall
  2014-03-17 14:50         ` Ian Campbell
  0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2014-03-17 14:43 UTC (permalink / raw)
  To: Ian Campbell; +Cc: stefano.stabellini, tim, xen-devel

On 03/17/2014 02:39 PM, Ian Campbell wrote:
>>> The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the
>>> only other uses are in the v7 spinlock code which includes a dsb() already.
>>
>> Would it be better to add dsb in the sev() macro?
> 
> ARM32 has a dsb_sev macro, maybe we should do the same for ARM64.

Do we really need to rename the macro? From what I understand, most of
the time sev => dsb before. I think we can hide it in sev() macro.

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up
  2014-03-17 14:43       ` Julien Grall
@ 2014-03-17 14:50         ` Ian Campbell
  0 siblings, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-17 14:50 UTC (permalink / raw)
  To: Julien Grall; +Cc: stefano.stabellini, tim, xen-devel

On Mon, 2014-03-17 at 14:43 +0000, Julien Grall wrote:
> On 03/17/2014 02:39 PM, Ian Campbell wrote:
> >>> The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the
> >>> only other uses are in the v7 spinlock code which includes a dsb() already.
> >>
> >> Would it be better to add dsb in the sev() macro?
> > 
> > ARM32 has a dsb_sev macro, maybe we should do the same for ARM64.
> 
> Do we really need to rename the macro?

I think so.

> From what I understand, most of
> the time sev => dsb before. I think we can hide it in sev() macro.

These macros are thin shims around the underlying instructions, having
them do other stuff as well would be potentially misleading.

The current macro has one callsight, I think we can live with the
churn...

Ian.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions
  2014-03-17 13:52 ` [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions Ian Campbell
  2014-03-17 14:10   ` Julien Grall
@ 2014-03-19 21:10   ` Julien Grall
  1 sibling, 0 replies; 13+ messages in thread
From: Julien Grall @ 2014-03-19 21:10 UTC (permalink / raw)
  To: Ian Campbell; +Cc: stefano.stabellini, tim, xen-devel

Hi Ian,

On 03/17/2014 01:52 PM, Ian Campbell wrote:
> All of the flush_xen_*_tlb functions operate on the local processor only. Add
> _local to the name and update the comments to clarify.

You forgot to replace one flush_xen_data_tlb_range_va in setup_pagetables:

mm.c: In function 'setup_pagetables':
mm.c:425:5: error: implicit declaration of function 'flush_xen_data_tlb_range_va' [-Werror=implicit-function-declaration]
mm.c:425:5: error: nested extern declaration of 'flush_xen_data_tlb_range_va' [-Werror=nested-externs]

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up
  2014-03-17 13:52 ` [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up Ian Campbell
  2014-03-17 14:18   ` Julien Grall
@ 2014-04-02 14:03   ` Ian Campbell
  1 sibling, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-04-02 14:03 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, tim, stefano.stabellini

On Mon, 2014-03-17 at 13:52 +0000, Ian Campbell wrote:
> This ensures that the writeq to the release address has occurred.
> 
> In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but
> make it explicit.
> 
> The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the
> only other uses are in the v7 spinlock code which includes a dsb() already.

Thinking about this some more the dsb() in the ioumap is logical and it
is reasonable to rely on it. This also satisfies the need for a dsb
before the sev.

So I intend to just drop this patch for the next iteration.

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: typos in the commit message
> ---
>  xen/arch/arm/arm64/smpboot.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 9146476..8e2ecf2 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -36,6 +36,7 @@ static int __init smp_spin_table_cpu_up(int cpu)
>  
>      iounmap(release);
>  
> +    dsb(); /* Ensure that the write to cpu_release_addr has occurred. */
>      sev();
>  
>      return cpu_up_send_sgi(cpu);

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-04-02 14:03 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-17 13:51 [PATCH 0/4 v2] xen: arm: smp & tlb cleanups Ian Campbell
2014-03-17 13:52 ` [PATCH 1/4] xen: arm: clarify naming of the Xen TLB flushing functions Ian Campbell
2014-03-17 14:10   ` Julien Grall
2014-03-19 21:10   ` Julien Grall
2014-03-17 13:52 ` [PATCH 2/4] xen: arm: flush TLB on all CPUs when setting or clearing fixmaps Ian Campbell
2014-03-17 13:52 ` [PATCH 3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up Ian Campbell
2014-03-17 14:18   ` Julien Grall
2014-03-17 14:39     ` Ian Campbell
2014-03-17 14:43       ` Julien Grall
2014-03-17 14:50         ` Ian Campbell
2014-04-02 14:03   ` Ian Campbell
2014-03-17 13:52 ` [PATCH 4/4] xen: arm32: don't force the compiler to allocate a dummy register Ian Campbell
2014-03-17 14:33   ` Julien Grall

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