[Xen-devel] [PATCH 0/4] xen/percpu: Cleanup

[Xen-devel] [PATCH 0/4] xen/percpu: Cleanup

[Andrew Cooper]

This is all cleanup which I got tangled in as part of the TSS xpti v2 fix,
before deciding that this is far too invasive to backport, whereas the TSS fix
itself really does want backporting.

This series is based on:
  https://lore.kernel.org/xen-devel/20190726203222.4833-1-andrew.cooper3@citrix.com/T/#t

but is not intended for backport to previous versions of Xen.

Andrew Cooper (4):
  arm/percpu: Move {get,set}_processor_id() into smp.h
  xen/percpu: Drop unused xen/percpu.h includes
  xen/percpu: Drop unused asm/percpu.h includes
  xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

 xen/arch/arm/xen.lds.S               |  1 -
 xen/arch/x86/acpi/cpufreq/cpufreq.c  |  1 -
 xen/arch/x86/acpi/cpufreq/powernow.c |  1 -
 xen/arch/x86/cpu/mcheck/mce.h        |  3 ++-
 xen/arch/x86/crash.c                 |  1 -
 xen/arch/x86/xen.lds.S               |  1 -
 xen/common/core_parking.c            |  2 +-
 xen/drivers/cpufreq/cpufreq.c        |  2 +-
 xen/include/asm-arm/percpu.h         | 11 -----------
 xen/include/asm-arm/smp.h            |  8 ++++++++
 xen/include/asm-x86/asm_defns.h      |  1 -
 xen/include/asm-x86/cpuid.h          |  2 +-
 xen/include/asm-x86/i387.h           |  1 -
 xen/include/asm-x86/irq.h            |  2 +-
 xen/include/asm-x86/percpu.h         |  5 -----
 xen/include/xen/kexec.h              |  1 -
 xen/include/xen/multicall.h          |  1 -
 xen/include/xen/percpu.h             |  6 ++++++
 xen/include/xen/rcupdate.h           |  1 -
 xen/include/xen/rwlock.h             |  1 +
 xen/include/xen/spinlock.h           |  1 -
 xen/xsm/flask/include/avc.h          |  3 ++-
 22 files changed, 23 insertions(+), 33 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/4] arm/percpu: Move {get, set}_processor_id() into smp.h

[Andrew Cooper]

For cleanup purposes, it is necessary for asm/percpu.h to not use
DECLARE_PER_CPU() itself.  asm/smp.h is arguably a better place for it to
live anyway.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/include/asm-arm/percpu.h | 6 ------
 xen/include/asm-arm/smp.h    | 8 ++++++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
index 264120b192..5ce81a1707 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -25,12 +25,6 @@ void percpu_init_areas(void);
 
 #define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name
 
-DECLARE_PER_CPU(unsigned int, cpu_id);
-#define get_processor_id()    (this_cpu(cpu_id))
-#define set_processor_id(id)  do {                      \
-    WRITE_SYSREG(__per_cpu_offset[id], TPIDR_EL2);      \
-    this_cpu(cpu_id) = (id);                            \
-} while(0)
 #endif
 
 #endif /* __ARM_PERCPU_H__ */
diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
index fdbcefa241..7d4edfa0a0 100644
--- a/xen/include/asm-arm/smp.h
+++ b/xen/include/asm-arm/smp.h
@@ -7,11 +7,19 @@
 #include <asm/current.h>
 #endif
 
+DECLARE_PER_CPU(unsigned int, cpu_id);
 DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
 DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
 
 #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
 
+#define get_processor_id()     this_cpu(cpu_id)
+#define set_processor_id(id)                            \
+do {                                                    \
+    WRITE_SYSREG(__per_cpu_offset[(id)], TPIDR_EL2);    \
+    this_cpu(cpu_id) = (id);                            \
+} while ( 0 )
+
 #define raw_smp_processor_id() (get_processor_id())
 
 /*
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/4] xen/percpu: Drop unused xen/percpu.h includes

[Andrew Cooper]

None of these headers use any PER_CPU() infrastructure.

xen/rwlock.h however does, and picked it up transitively via xen/spinlock.h,
so include it properly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/include/asm-x86/i387.h  | 1 -
 xen/include/xen/multicall.h | 1 -
 xen/include/xen/rcupdate.h  | 1 -
 xen/include/xen/rwlock.h    | 1 +
 xen/include/xen/spinlock.h  | 1 -
 5 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/include/asm-x86/i387.h b/xen/include/asm-x86/i387.h
index 434a7761a5..a783549db9 100644
--- a/xen/include/asm-x86/i387.h
+++ b/xen/include/asm-x86/i387.h
@@ -12,7 +12,6 @@
 #define __ASM_I386_I387_H
 
 #include <xen/types.h>
-#include <xen/percpu.h>
 
 /* Byte offset of the stored word size within the FXSAVE area/portion. */
 #define FPU_WORD_SIZE_OFFSET 511
diff --git a/xen/include/xen/multicall.h b/xen/include/xen/multicall.h
index d0aa52009c..ac8238660a 100644
--- a/xen/include/xen/multicall.h
+++ b/xen/include/xen/multicall.h
@@ -5,7 +5,6 @@
 #ifndef __XEN_MULTICALL_H__
 #define __XEN_MULTICALL_H__
 
-#include <xen/percpu.h>
 #ifdef CONFIG_COMPAT
 #include <compat/xen.h>
 #endif
diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
index 3402eb5caf..13850865ed 100644
--- a/xen/include/xen/rcupdate.h
+++ b/xen/include/xen/rcupdate.h
@@ -33,7 +33,6 @@
 
 #include <xen/cache.h>
 #include <xen/spinlock.h>
-#include <xen/percpu.h>
 #include <xen/cpumask.h>
 #include <xen/preempt.h>
 
diff --git a/xen/include/xen/rwlock.h b/xen/include/xen/rwlock.h
index 35657c56c4..3dfea1ac2a 100644
--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -1,6 +1,7 @@
 #ifndef __RWLOCK_H__
 #define __RWLOCK_H__
 
+#include <xen/percpu.h>
 #include <xen/spinlock.h>
 
 #include <asm/atomic.h>
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index a811b73bf3..2c7415e23a 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -4,7 +4,6 @@
 #include <asm/system.h>
 #include <asm/spinlock.h>
 #include <asm/types.h>
-#include <xen/percpu.h>
 
 #ifndef NDEBUG
 struct lock_debug {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/4] xen/percpu: Drop unused asm/percpu.h includes

[Andrew Cooper]

These files either don't use any PER_CPU() infrastructure at all, or use
DEFINE_PER_CPU_*().  This is declared in xen/percpu.h, not asm/percpu.h, which
means that xen/percpu.h is included via a different path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/arch/arm/xen.lds.S               | 1 -
 xen/arch/x86/acpi/cpufreq/cpufreq.c  | 1 -
 xen/arch/x86/acpi/cpufreq/powernow.c | 1 -
 xen/arch/x86/crash.c                 | 1 -
 xen/arch/x86/xen.lds.S               | 1 -
 xen/common/core_parking.c            | 2 +-
 xen/drivers/cpufreq/cpufreq.c        | 2 +-
 xen/include/asm-x86/asm_defns.h      | 1 -
 xen/include/xen/kexec.h              | 1 -
 9 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 07cbdf2543..09ec99baf5 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -4,7 +4,6 @@
 
 #include <xen/cache.h>
 #include <asm/page.h>
-#include <asm/percpu.h>
 #undef ENTRY
 #undef ALIGN
 
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 7086d1aa15..f05275578d 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -38,7 +38,6 @@
 #include <asm/msr.h>
 #include <asm/io.h>
 #include <asm/processor.h>
-#include <asm/percpu.h>
 #include <asm/cpufeature.h>
 #include <acpi/acpi.h>
 #include <acpi/cpufreq/cpufreq.h>
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index 72ab6a1eba..3cf9c6cd05 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -32,7 +32,6 @@
 #include <asm/msr.h>
 #include <asm/io.h>
 #include <asm/processor.h>
-#include <asm/percpu.h>
 #include <asm/cpufeature.h>
 #include <acpi/acpi.h>
 #include <acpi/cpufreq/cpufreq.h>
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index a9f3e1890c..ce361afd7c 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -10,7 +10,6 @@
 
 #include <asm/atomic.h>
 #include <asm/elf.h>
-#include <asm/percpu.h>
 #include <xen/types.h>
 #include <xen/irq.h>
 #include <asm/nmi.h>
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index c82e1e504a..fca25ad54c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -3,7 +3,6 @@
 
 #include <xen/cache.h>
 #include <asm/page.h>
-#include <asm/percpu.h>
 #undef ENTRY
 #undef ALIGN
 
diff --git a/xen/common/core_parking.c b/xen/common/core_parking.c
index 7ae5d6278e..c22710f94d 100644
--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -19,7 +19,7 @@
 #include <xen/cpu.h>
 #include <xen/init.h>
 #include <xen/cpumask.h>
-#include <asm/percpu.h>
+
 #include <asm/smp.h>
 
 #define CORE_PARKING_INCREMENT 1
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index ba9897a22b..2d716abf72 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -41,7 +41,7 @@
 #include <asm/bug.h>
 #include <asm/io.h>
 #include <asm/processor.h>
-#include <asm/percpu.h>
+
 #include <acpi/acpi.h>
 #include <acpi/cpufreq/cpufreq.h>
 
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 4fe5556a34..c4f49a35d3 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -7,7 +7,6 @@
 #include <asm/asm-offsets.h>
 #endif
 #include <asm/bug.h>
-#include <asm/percpu.h>
 #include <asm/x86-defns.h>
 #include <xen/stringify.h>
 #include <asm/cpufeature.h>
diff --git a/xen/include/xen/kexec.h b/xen/include/xen/kexec.h
index f5b9d165ae..1068f34c5b 100644
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -4,7 +4,6 @@
 #ifdef CONFIG_KEXEC
 
 #include <public/kexec.h>
-#include <asm/percpu.h>
 #include <xen/elfcore.h>
 #include <xen/kimage.h>
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Andrew Cooper]

These macros are identical across the architectures, and shouldn't be separate
from the DEFINE_PER_CPU*() infrastructure.

This converts the final asm/percpu.h includes, which were all using
DECLARE_PER_CPU(), to include xen/percpu.h instead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/arch/x86/cpu/mcheck/mce.h | 3 ++-
 xen/include/asm-arm/percpu.h  | 5 -----
 xen/include/asm-x86/cpuid.h   | 2 +-
 xen/include/asm-x86/irq.h     | 2 +-
 xen/include/asm-x86/percpu.h  | 5 -----
 xen/include/xen/percpu.h      | 6 ++++++
 xen/xsm/flask/include/avc.h   | 3 ++-
 7 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index f2cebccdd1..7dcea7b454 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -3,12 +3,13 @@
 #define _MCE_H
 
 #include <xen/init.h>
+#include <xen/percpu.h>
 #include <xen/sched.h>
 #include <xen/smp.h>
+
 #include <asm/types.h>
 #include <asm/traps.h>
 #include <asm/atomic.h>
-#include <asm/percpu.h>
 
 #include "x86_mca.h"
 #include "mctelem.h"
diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
index 5ce81a1707..f1a8768080 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -10,9 +10,6 @@ extern char __per_cpu_start[], __per_cpu_data_end[];
 extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 
-#define __DEFINE_PER_CPU(attr, type, name) \
-    attr __typeof__(type) per_cpu_ ## name
-
 #define per_cpu(var, cpu)  \
     (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
 #define this_cpu(var) \
@@ -23,8 +20,6 @@ void percpu_init_areas(void);
 #define this_cpu_ptr(var) \
     (*RELOC_HIDE(var, READ_SYSREG(TPIDR_EL2)))
 
-#define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name
-
 #endif
 
 #endif /* __ARM_PERCPU_H__ */
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 548108f948..1b00e832d6 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -2,11 +2,11 @@
 #define __X86_CPUID_H__
 
 #include <asm/cpufeatureset.h>
-#include <asm/percpu.h>
 
 #ifndef __ASSEMBLY__
 #include <xen/types.h>
 #include <xen/kernel.h>
+#include <xen/percpu.h>
 
 #include <xen/lib/x86/cpu-policy.h>
 #include <xen/lib/x86/cpuid.h>
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index bc0c0c15d2..d3124f7b5d 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -6,10 +6,10 @@
 #include <asm/atomic.h>
 #include <asm/numa.h>
 #include <xen/cpumask.h>
+#include <xen/percpu.h>
 #include <xen/smp.h>
 #include <asm/hvm/irq.h>
 #include <irq_vectors.h>
-#include <asm/percpu.h>
 
 extern unsigned int nr_irqs_gsi;
 extern unsigned int nr_irqs;
diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
index 5b6cef04c4..2b0c29a233 100644
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -7,17 +7,12 @@ extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 #endif
 
-#define __DEFINE_PER_CPU(attr, type, name) \
-    attr __typeof__(type) per_cpu_ ## name
-
 /* var is in discarded region: offset to particular copy we want */
 #define per_cpu(var, cpu)  \
     (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
 #define this_cpu(var) \
     (*RELOC_HIDE(&per_cpu__##var, get_cpu_info()->per_cpu_offset))
 
-#define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name
-
 #define this_cpu_ptr(var) \
     (*RELOC_HIDE(var, get_cpu_info()->per_cpu_offset))
 
diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
index 71a31cc361..48a43f769d 100644
--- a/xen/include/xen/percpu.h
+++ b/xen/include/xen/percpu.h
@@ -3,6 +3,12 @@
 
 #include <asm/percpu.h>
 
+#define DECLARE_PER_CPU(type, name) \
+    extern __typeof__(type) per_cpu__ ## name
+
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
+
 /*
  * Separate out the type, so (int[3], foo) works.
  *
diff --git a/xen/xsm/flask/include/avc.h b/xen/xsm/flask/include/avc.h
index 93386bd7a1..c14bd07a2b 100644
--- a/xen/xsm/flask/include/avc.h
+++ b/xen/xsm/flask/include/avc.h
@@ -11,8 +11,9 @@
 
 #include <xen/errno.h>
 #include <xen/lib.h>
+#include <xen/percpu.h>
 #include <xen/spinlock.h>
-#include <asm/percpu.h>
+
 #include "flask.h"
 #include "av_permissions.h"
 #include "security.h"
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] arm/percpu: Move {get, set}_processor_id() into smp.h

[Julien Grall]

Hi Andrew,

Thank you for looking into this.

On 7/26/19 10:08 PM, Andrew Cooper wrote:
> For cleanup purposes, it is necessary for asm/percpu.h to not use
> DECLARE_PER_CPU() itself.  asm/smp.h is arguably a better place for it to
> live anyway.
I have noticed that a lot of arch helpers called by common code 
are defined in different headers. The most offenders are in mm.h
and page.h.

Looking at x86, the two helpers are defined in current.h. So I
think it would make sense to define them at the same places. I
don't have a particular preference between smp.h and current.h,
just want some consistency :).

FWIW, Xen build nicely when the helpers are moved in current.h:

diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
index c4af66fbb9..23fa6009d8 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -39,6 +39,13 @@ static inline struct cpu_info *get_cpu_info(void)
 
 #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn)
 
+DECLARE_PER_CPU(unsigned int, cpu_id);
+#define get_processor_id()    (this_cpu(cpu_id))
+#define set_processor_id(id)  do {                      \
+    WRITE_SYSREG(__per_cpu_offset[id], TPIDR_EL2);      \
+    this_cpu(cpu_id) = (id);                            \
+} while(0)
+
 #endif
 
 #endif /* __ARM_CURRENT_H__ */
diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
index 9584b830d4..011016347b 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -27,12 +27,6 @@ void percpu_init_areas(void);
 
 #define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name
 
-DECLARE_PER_CPU(unsigned int, cpu_id);
-#define get_processor_id()    (this_cpu(cpu_id))
-#define set_processor_id(id)  do {                      \
-    WRITE_SYSREG(__per_cpu_offset[id], TPIDR_EL2);      \
-    this_cpu(cpu_id) = (id);                            \
-} while(0)
 #endif
 
 #endif /* __ARM_PERCPU_H__ */


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/percpu: Drop unused asm/percpu.h includes

[Julien Grall]

Hi Andrew,

On 7/26/19 10:08 PM, Andrew Cooper wrote:
> These files either don't use any PER_CPU() infrastructure at all, or use
> DEFINE_PER_CPU_*().  This is declared in xen/percpu.h, not asm/percpu.h, which
> means that xen/percpu.h is included via a different path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Julien Grall]

Hi Andrew,

On 7/26/19 10:08 PM, Andrew Cooper wrote:
> These macros are identical across the architectures, and shouldn't be separate
> from the DEFINE_PER_CPU*() infrastructure.
> 
> This converts the final asm/percpu.h includes, which were all using
> DECLARE_PER_CPU(), to include xen/percpu.h instead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] arm/percpu: Move {get, set}_processor_id() into smp.h

[Andrew Cooper]

On 26/07/2019 23:30, Julien Grall wrote:
> Hi Andrew,
>
> Thank you for looking into this.
>
> On 7/26/19 10:08 PM, Andrew Cooper wrote:
>> For cleanup purposes, it is necessary for asm/percpu.h to not use
>> DECLARE_PER_CPU() itself.  asm/smp.h is arguably a better place for it to
>> live anyway.
> I have noticed that a lot of arch helpers called by common code 
> are defined in different headers. The most offenders are in mm.h
> and page.h.

Those are massive rats nests.  At least we're slowly making progress on
the outskirts.

Lonterm, I'd like to get to a position where the only time you'd ever
include an asm/ header is either from the common header itself, or for
headers unique to the current architecture.  I expect this to simplify
things massively.

>
> Looking at x86, the two helpers are defined in current.h. So I
> think it would make sense to define them at the same places. I
> don't have a particular preference between smp.h and current.h,
> just want some consistency :).
>
> FWIW, Xen build nicely when the helpers are moved in current.h:

I did consider current.h, but couldn't decide and flipped a coin.

I'm perfectly happy to put it here if that's what you'd prefer.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] arm/percpu: Move {get, set}_processor_id() into smp.h

[Julien Grall]

Hi Andrew,

On 7/26/19 11:37 PM, Andrew Cooper wrote:
> On 26/07/2019 23:30, Julien Grall wrote:
>> Hi Andrew,
>>
>> Thank you for looking into this.
>>
>> On 7/26/19 10:08 PM, Andrew Cooper wrote:
>>> For cleanup purposes, it is necessary for asm/percpu.h to not use
>>> DECLARE_PER_CPU() itself.  asm/smp.h is arguably a better place for it to
>>> live anyway.
>> I have noticed that a lot of arch helpers called by common code
>> are defined in different headers. The most offenders are in mm.h
>> and page.h.
> 
> Those are massive rats nests.  At least we're slowly making progress on
> the outskirts.
> 
> Lonterm, I'd like to get to a position where the only time you'd ever
> include an asm/ header is either from the common header itself, or for
> headers unique to the current architecture.  I expect this to simplify
> things massively.
> 
>>
>> Looking at x86, the two helpers are defined in current.h. So I
>> think it would make sense to define them at the same places. I
>> don't have a particular preference between smp.h and current.h,
>> just want some consistency :).
>>
>> FWIW, Xen build nicely when the helpers are moved in current.h:
> 
> I did consider current.h, but couldn't decide and flipped a coin.
> 
> I'm perfectly happy to put it here if that's what you'd prefer.

I would prefer consistency with x86 so, current.h :). Feel free to 
re-use the diff I provided and add my acked-by.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen/percpu: Drop unused xen/percpu.h includes

[Roger Pau Monné]

On Fri, Jul 26, 2019 at 10:08:52PM +0100, Andrew Cooper wrote:
> None of these headers use any PER_CPU() infrastructure.
> 
> xen/rwlock.h however does, and picked it up transitively via xen/spinlock.h,
> so include it properly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/percpu: Drop unused asm/percpu.h includes

[Roger Pau Monné]

On Fri, Jul 26, 2019 at 10:08:53PM +0100, Andrew Cooper wrote:
> These files either don't use any PER_CPU() infrastructure at all, or use
> DEFINE_PER_CPU_*().  This is declared in xen/percpu.h, not asm/percpu.h, which
> means that xen/percpu.h is included via a different path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Roger Pau Monné]

On Fri, Jul 26, 2019 at 10:08:54PM +0100, Andrew Cooper wrote:
> These macros are identical across the architectures, and shouldn't be separate
> from the DEFINE_PER_CPU*() infrastructure.
> 
> This converts the final asm/percpu.h includes, which were all using
> DECLARE_PER_CPU(), to include xen/percpu.h instead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen/percpu: Drop unused xen/percpu.h includes

[Jan Beulich]

On 29.07.2019 10:55, Roger Pau Monné  wrote:
> On Fri, Jul 26, 2019 at 10:08:52PM +0100, Andrew Cooper wrote:
>> None of these headers use any PER_CPU() infrastructure.
>>
>> xen/rwlock.h however does, and picked it up transitively via xen/spinlock.h,
>> so include it properly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen/percpu: Drop unused asm/percpu.h includes

[Jan Beulich]

On 26.07.2019 23:08, Andrew Cooper wrote:
> These files either don't use any PER_CPU() infrastructure at all, or use
> DEFINE_PER_CPU_*().  This is declared in xen/percpu.h, not asm/percpu.h, which
> means that xen/percpu.h is included via a different path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with a remark:

> --- a/xen/common/core_parking.c
> +++ b/xen/common/core_parking.c
> @@ -19,7 +19,7 @@
>   #include <xen/cpu.h>
>   #include <xen/init.h>
>   #include <xen/cpumask.h>
> -#include <asm/percpu.h>
> +
>   #include <asm/smp.h>

I don't believe in such blank lines to be overly useful,
especially when ...

> --- a/xen/include/xen/kexec.h
> +++ b/xen/include/xen/kexec.h
> @@ -4,7 +4,6 @@
>   #ifdef CONFIG_KEXEC
>   
>   #include <public/kexec.h>
> -#include <asm/percpu.h>
>   #include <xen/elfcore.h>
>   #include <xen/kimage.h>

... this model isn't being followed consistently.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Jan Beulich]

On 26.07.2019 23:08, Andrew Cooper wrote:
> --- a/xen/include/xen/percpu.h
> +++ b/xen/include/xen/percpu.h
> @@ -3,6 +3,12 @@
>   
>   #include <asm/percpu.h>
>   
> +#define DECLARE_PER_CPU(type, name) \
> +    extern __typeof__(type) per_cpu__ ## name
> +
> +#define __DEFINE_PER_CPU(attr, type, name) \
> +    attr __typeof__(type) per_cpu_ ## name
> +
>   /*
>    * Separate out the type, so (int[3], foo) works.
>    *

By moving things here you render stale the remainder of the
comment in context above: No per-arch symbol name prefix is going
to be possible anymore. I'm not against it, but this comment
would then want adjusting. What's not immediately clear to me is
whether the two-stage concatenation of an underscore each is then
still necessary.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Andrew Cooper]

On 29/07/2019 14:00, Jan Beulich wrote:
> On 26.07.2019 23:08, Andrew Cooper wrote:
>> --- a/xen/include/xen/percpu.h
>> +++ b/xen/include/xen/percpu.h
>> @@ -3,6 +3,12 @@
>>   
>>   #include <asm/percpu.h>
>>   
>> +#define DECLARE_PER_CPU(type, name) \
>> +    extern __typeof__(type) per_cpu__ ## name
>> +
>> +#define __DEFINE_PER_CPU(attr, type, name) \
>> +    attr __typeof__(type) per_cpu_ ## name
>> +
>>   /*
>>    * Separate out the type, so (int[3], foo) works.
>>    *
> By moving things here you render stale the remainder of the
> comment in context above: No per-arch symbol name prefix is going
> to be possible anymore. I'm not against it, but this comment
> would then want adjusting. What's not immediately clear to me is
> whether the two-stage concatenation of an underscore each is then
> still necessary.

Yes it is still necessary.  See the TSS thread for why.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Jan Beulich]

On 29.07.2019 15:23, Andrew Cooper wrote:
> On 29/07/2019 14:00, Jan Beulich wrote:
>> On 26.07.2019 23:08, Andrew Cooper wrote:
>>> --- a/xen/include/xen/percpu.h
>>> +++ b/xen/include/xen/percpu.h
>>> @@ -3,6 +3,12 @@
>>>    
>>>    #include <asm/percpu.h>
>>>    
>>> +#define DECLARE_PER_CPU(type, name) \
>>> +    extern __typeof__(type) per_cpu__ ## name
>>> +
>>> +#define __DEFINE_PER_CPU(attr, type, name) \
>>> +    attr __typeof__(type) per_cpu_ ## name
>>> +
>>>    /*
>>>     * Separate out the type, so (int[3], foo) works.
>>>     *
>> By moving things here you render stale the remainder of the
>> comment in context above: No per-arch symbol name prefix is going
>> to be possible anymore. I'm not against it, but this comment
>> would then want adjusting. What's not immediately clear to me is
>> whether the two-stage concatenation of an underscore each is then
>> still necessary.
> 
> Yes it is still necessary.  See the TSS thread for why.

No, that thread doesn't explain it. From an initial look I think
two-stage expansion is still necessary, but it could then be
_ ## name on the first and per_cpu ## name on the second (i.e.
no double underscore in the middle anymore). Of course there may
be reasons why we actually _want_ a double underscore there.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Andrew Cooper]

On 29/07/2019 14:54, Jan Beulich wrote:
> On 29.07.2019 15:23, Andrew Cooper wrote:
>> On 29/07/2019 14:00, Jan Beulich wrote:
>>> On 26.07.2019 23:08, Andrew Cooper wrote:
>>>> --- a/xen/include/xen/percpu.h
>>>> +++ b/xen/include/xen/percpu.h
>>>> @@ -3,6 +3,12 @@
>>>>    
>>>>    #include <asm/percpu.h>
>>>>    
>>>> +#define DECLARE_PER_CPU(type, name) \
>>>> +    extern __typeof__(type) per_cpu__ ## name
>>>> +
>>>> +#define __DEFINE_PER_CPU(attr, type, name) \
>>>> +    attr __typeof__(type) per_cpu_ ## name
>>>> +
>>>>    /*
>>>>     * Separate out the type, so (int[3], foo) works.
>>>>     *
>>> By moving things here you render stale the remainder of the
>>> comment in context above: No per-arch symbol name prefix is going
>>> to be possible anymore. I'm not against it, but this comment
>>> would then want adjusting. What's not immediately clear to me is
>>> whether the two-stage concatenation of an underscore each is then
>>> still necessary.
>> Yes it is still necessary.  See the TSS thread for why.
> No, that thread doesn't explain it. From an initial look I think
> two-stage expansion is still necessary

It is about preventing 'name' being expanded, due to the mess with
cpumask_scratch, which requires a ## at least at the top level.

I personally think that fixing cpumask_scratch is the right way to go,
but I specifically didn't touch that so as to avoid wreaking havoc with
Juergen's core-scheduling series.

> , but it could then be
> _ ## name on the first and per_cpu ## name on the second (i.e.
> no double underscore in the middle anymore).

Hmm, probably, but...

> Of course there may
> be reasons why we actually _want_ a double underscore there.

... I don't have the effort or energy - but most importantly, time - to
rewrite Xen from scratch.

If there is a concrete reason why dropping the double underscore is
good/necessary/other, then it should be present as its own patch, not
crowbarred into an unrelated cleanup patch.

Until then, the inertia of "because its already like this" wins.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Jan Beulich]

On 29.07.2019 20:03, Andrew Cooper wrote:
> On 29/07/2019 14:54, Jan Beulich wrote:
>> On 29.07.2019 15:23, Andrew Cooper wrote:
>>> On 29/07/2019 14:00, Jan Beulich wrote:
>>>> On 26.07.2019 23:08, Andrew Cooper wrote:
>>>>> --- a/xen/include/xen/percpu.h
>>>>> +++ b/xen/include/xen/percpu.h
>>>>> @@ -3,6 +3,12 @@
>>>>>     
>>>>>     #include <asm/percpu.h>
>>>>>     
>>>>> +#define DECLARE_PER_CPU(type, name) \
>>>>> +    extern __typeof__(type) per_cpu__ ## name
>>>>> +
>>>>> +#define __DEFINE_PER_CPU(attr, type, name) \
>>>>> +    attr __typeof__(type) per_cpu_ ## name
>>>>> +
>>>>>     /*
>>>>>      * Separate out the type, so (int[3], foo) works.
>>>>>      *
>>>> By moving things here you render stale the remainder of the
>>>> comment in context above: No per-arch symbol name prefix is going
>>>> to be possible anymore. I'm not against it, but this comment
>>>> would then want adjusting. What's not immediately clear to me is
>>>> whether the two-stage concatenation of an underscore each is then
>>>> still necessary.
>>> Yes it is still necessary.  See the TSS thread for why.
>> No, that thread doesn't explain it. From an initial look I think
>> two-stage expansion is still necessary
> 
> It is about preventing 'name' being expanded, due to the mess with
> cpumask_scratch, which requires a ## at least at the top level.
> 
> I personally think that fixing cpumask_scratch is the right way to go,
> but I specifically didn't touch that so as to avoid wreaking havoc with
> Juergen's core-scheduling series.
> 
>> , but it could then be
>> _ ## name on the first and per_cpu ## name on the second (i.e.
>> no double underscore in the middle anymore).
> 
> Hmm, probably, but...
> 
>> Of course there may
>> be reasons why we actually _want_ a double underscore there.
> 
> ... I don't have the effort or energy - but most importantly, time - to
> rewrite Xen from scratch.
> 
> If there is a concrete reason why dropping the double underscore is
> good/necessary/other, then it should be present as its own patch, not
> crowbarred into an unrelated cleanup patch.
> 
> Until then, the inertia of "because its already like this" wins.

And this is all fine, as long as the comment in question continues
to reflect reality after this change.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen/percpu: Make DECLARE_PER_CPU() and __DEFINE_PER_CPU() common

[Juergen Gross]

On 29.07.19 20:03, Andrew Cooper wrote:
> On 29/07/2019 14:54, Jan Beulich wrote:
>> On 29.07.2019 15:23, Andrew Cooper wrote:
>>> On 29/07/2019 14:00, Jan Beulich wrote:
>>>> On 26.07.2019 23:08, Andrew Cooper wrote:
>>>>> --- a/xen/include/xen/percpu.h
>>>>> +++ b/xen/include/xen/percpu.h
>>>>> @@ -3,6 +3,12 @@
>>>>>     
>>>>>     #include <asm/percpu.h>
>>>>>     
>>>>> +#define DECLARE_PER_CPU(type, name) \
>>>>> +    extern __typeof__(type) per_cpu__ ## name
>>>>> +
>>>>> +#define __DEFINE_PER_CPU(attr, type, name) \
>>>>> +    attr __typeof__(type) per_cpu_ ## name
>>>>> +
>>>>>     /*
>>>>>      * Separate out the type, so (int[3], foo) works.
>>>>>      *
>>>> By moving things here you render stale the remainder of the
>>>> comment in context above: No per-arch symbol name prefix is going
>>>> to be possible anymore. I'm not against it, but this comment
>>>> would then want adjusting. What's not immediately clear to me is
>>>> whether the two-stage concatenation of an underscore each is then
>>>> still necessary.
>>> Yes it is still necessary.  See the TSS thread for why.
>> No, that thread doesn't explain it. From an initial look I think
>> two-stage expansion is still necessary
> 
> It is about preventing 'name' being expanded, due to the mess with
> cpumask_scratch, which requires a ## at least at the top level.
> 
> I personally think that fixing cpumask_scratch is the right way to go,
> but I specifically didn't touch that so as to avoid wreaking havoc with
> Juergen's core-scheduling series.

I appreciate that, but I don't think a large series like mine should
block efforts to make Xen cleaner.

Especially this case should be rather easy to handle, as there is no
change of the logic of the patches to be expected.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel