xen,arm: enable cpu_hotplug

PATCH Superseded Stefano Stabellini 3 participants 12 messages Oct 21, 2015 – Oct 22, 2015

Subsystems (5)

Arm (32-bit) x86 Xen hypervisor interface Xen hypervisor x86 +1 more subsystems

lore

Share & tools

lore

Stefano Stabellini

author

2015-10-21 11:53 UTC #1 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

Build cpu_hotplug for ARM and ARM64 guests.

Rename arch_(un)register_cpu to xen_(un)register_cpu and provide an empty implementation on ARM and ARM64. On x86 just call arch_(un)register_cpu as we are already doing.

Initialize cpu_hotplug on ARM.

Signed-off-by Stefano Stabellini <[email protected]>

Reviewed-by Julien Grall <[email protected]>

---

Changes in v2:
- don't initialize if not a xen_domain() on arm
- protect xen_arch_(un)register_cpu with ifdef CONFIG_HOTPLUG_CPU on arm

---
 arch/arm/include/asm/xen/hypervisor.h |   10 ++++++++++
 arch/x86/include/asm/xen/hypervisor.h |    5 +++++
 arch/x86/xen/enlighten.c              |   15 +++++++++++++++
 drivers/xen/Makefile                  |    2 --
 drivers/xen/cpu_hotplug.c             |    8 ++++++--
 5 files changed, 36 insertions(+), 4 deletions(-)

arch/arm/include/asm/xen/hypervisor.h +10 -0

index 04ff8e7..9525151 100644
@@ -26,4 +26,14 @@ void __init xen_early_init(void);
 static inline void xen_early_init(void) { return; }
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+static inline void xen_arch_register_cpu(int num)
+{
+}
+
+static inline void xen_arch_unregister_cpu(int num)
+{
+}
+#endif
+
 #endif /* _ASM_ARM_XEN_HYPERVISOR_H */

arch/x86/include/asm/xen/hypervisor.h +5 -0

index d866959..8b2d4be 100644
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
 }
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif
+
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */

arch/x86/xen/enlighten.c +15 -0

index 11d6fb4..ba62d8e 100644
@@ -71,6 +71,7 @@
 #include <asm/mwait.h>
 #include <asm/pci_x86.h>
 #include <asm/pat.h>
+#include <asm/cpu.h>
 
 #ifdef CONFIG_ACPI
 #include <linux/acpi.h>

@@ -1868,3 +1869,17 @@ const struct hypervisor_x86 x86_hyper_xen = {
 	.set_cpu_features       = xen_set_cpu_features,
 };
 EXPORT_SYMBOL(x86_hyper_xen);
+
+#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num)
+{
+	arch_register_cpu(num);
+}
+EXPORT_SYMBOL(xen_arch_register_cpu);
+
+void xen_arch_unregister_cpu(int num)
+{
+	arch_unregister_cpu(num);
+}
+EXPORT_SYMBOL(xen_arch_unregister_cpu);
+#endif

drivers/xen/Makefile +0 -2

index e293bc5..aa8a7f7 100644
@@ -1,6 +1,4 @@
-ifeq ($(filter y, $(CONFIG_ARM) $(CONFIG_ARM64)),)
 obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
-endif
 obj-$(CONFIG_X86)			+= fallback.o
 obj-y	+= grant-table.o features.o balloon.o manage.o preempt.o
 obj-y	+= events/

drivers/xen/cpu_hotplug.c +6 -2

index cc6513a..43de1f5 100644
@@ -11,7 +11,7 @@
 static void enable_hotplug_cpu(int cpu)
 {
 	if (!cpu_present(cpu))
-		arch_register_cpu(cpu);
+		xen_arch_register_cpu(cpu);
 
 	set_cpu_present(cpu, true);
 }

@@ -19,7 +19,7 @@ static void enable_hotplug_cpu(int cpu)
 static void disable_hotplug_cpu(int cpu)
 {
 	if (cpu_present(cpu))
-		arch_unregister_cpu(cpu);
+		xen_arch_unregister_cpu(cpu);
 
 	set_cpu_present(cpu, false);
 }

@@ -102,7 +102,11 @@ static int __init setup_vcpu_hotplug_event(void)
 	static struct notifier_block xsn_cpu = {
 		.notifier_call = setup_cpu_watcher };
 
+#ifdef CONFIG_X86
 	if (!xen_pv_domain())
+#else
+	if (!xen_domain())
+#endif
 		return -ENODEV;
 
 	register_xenstore_notifier(&xsn_cpu);

Signature · 1 line

1.7.10.4

Stefano Stabellini

author

2015-10-21 13:00 UTC #2 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

CC'ing few more x86 people as it contains a few x86 changes.

If you are OK with this, I'll go ahead and apply the series to xentip/for-linus-4.4.

Quoted 135 lines · Stefano Stabellini

Build cpu_hotplug for ARM and ARM64 guests.

Rename arch_(un)register_cpu to xen_(un)register_cpu and provide an empty implementation on ARM and ARM64. On x86 just call arch_(un)register_cpu as we are already doing.

Initialize cpu_hotplug on ARM.

Signed-off-by Stefano Stabellini <[email protected]>

Reviewed-by Julien Grall <[email protected]>

---

Changes in v2:
- don't initialize if not a xen_domain() on arm
- protect xen_arch_(un)register_cpu with ifdef CONFIG_HOTPLUG_CPU on arm

---
 arch/arm/include/asm/xen/hypervisor.h |   10 ++++++++++
 arch/x86/include/asm/xen/hypervisor.h |    5 +++++
 arch/x86/xen/enlighten.c              |   15 +++++++++++++++
 drivers/xen/Makefile                  |    2 --
 drivers/xen/cpu_hotplug.c             |    8 ++++++--
 5 files changed, 36 insertions(+), 4 deletions(-)

arch/arm/include/asm/xen/hypervisor.h +10 -0

index 04ff8e7..9525151 100644
@@ -26,4 +26,14 @@ void __init xen_early_init(void);
 static inline void xen_early_init(void) { return; }
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+static inline void xen_arch_register_cpu(int num)
+{
+}
+
+static inline void xen_arch_unregister_cpu(int num)
+{
+}
+#endif
+
 #endif /* _ASM_ARM_XEN_HYPERVISOR_H */

arch/x86/include/asm/xen/hypervisor.h +5 -0

index d866959..8b2d4be 100644
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
 }
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif
+
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */

arch/x86/xen/enlighten.c +15 -0

index 11d6fb4..ba62d8e 100644
@@ -71,6 +71,7 @@
 #include <asm/mwait.h>
 #include <asm/pci_x86.h>
 #include <asm/pat.h>
+#include <asm/cpu.h>
 
 #ifdef CONFIG_ACPI
 #include <linux/acpi.h>

@@ -1868,3 +1869,17 @@ const struct hypervisor_x86 x86_hyper_xen = {
 	.set_cpu_features       = xen_set_cpu_features,
 };
 EXPORT_SYMBOL(x86_hyper_xen);
+
+#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num)
+{
+	arch_register_cpu(num);
+}
+EXPORT_SYMBOL(xen_arch_register_cpu);
+
+void xen_arch_unregister_cpu(int num)
+{
+	arch_unregister_cpu(num);
+}
+EXPORT_SYMBOL(xen_arch_unregister_cpu);
+#endif

drivers/xen/Makefile +0 -2

index e293bc5..aa8a7f7 100644
@@ -1,6 +1,4 @@
-ifeq ($(filter y, $(CONFIG_ARM) $(CONFIG_ARM64)),)
 obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
-endif
 obj-$(CONFIG_X86)			+= fallback.o
 obj-y	+= grant-table.o features.o balloon.o manage.o preempt.o
 obj-y	+= events/

drivers/xen/cpu_hotplug.c +6 -2

index cc6513a..43de1f5 100644
@@ -11,7 +11,7 @@
 static void enable_hotplug_cpu(int cpu)
 {
 	if (!cpu_present(cpu))
-		arch_register_cpu(cpu);
+		xen_arch_register_cpu(cpu);
 
 	set_cpu_present(cpu, true);
 }

@@ -19,7 +19,7 @@ static void enable_hotplug_cpu(int cpu)
 static void disable_hotplug_cpu(int cpu)
 {
 	if (cpu_present(cpu))
-		arch_unregister_cpu(cpu);
+		xen_arch_unregister_cpu(cpu);
 
 	set_cpu_present(cpu, false);
 }

@@ -102,7 +102,11 @@ static int __init setup_vcpu_hotplug_event(void)
 	static struct notifier_block xsn_cpu = {
 		.notifier_call = setup_cpu_watcher };
 
+#ifdef CONFIG_X86
 	if (!xen_pv_domain())
+#else
+	if (!xen_domain())
+#endif
 		return -ENODEV;
 
 	register_xenstore_notifier(&xsn_cpu);

Signature · 1 line

1.7.10.4

Boris Ostrovsky

reviewer

2015-10-21 13:19 UTC #3 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

Quoted 9 lines · Stefano Stabellini

Quoted 9 lines

arch/x86/include/asm/xen/hypervisor.h +0 -0

index d866959..8b2d4be 100644
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
  }
  #endif

Quoted 4 lines

arch/x86/include/asm/xen/hypervisor.h static inline bool xen_x2apic_para_available(void) · line 63
Quoted 4 lines
arch/x86/include/asm/xen/hypervisor.h +4 -0
+#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif

Why not inline them here, just like you did for ARM?

-boris

Stefano Stabellini

author

2015-10-21 15:36 UTC #4 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

Quoted 16 lines · Boris Ostrovsky

Quoted 13 lines · Stefano Stabellini
arch/x86/include/asm/xen/hypervisor.h static inline bool xen_x2apic_para_available(void) · line 62
Quoted 13 lines
arch/x86/include/asm/xen/hypervisor.h +3 -0
index d866959..8b2d4be 100644
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
  }
  #endif
  +#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif
Why not inline them here, just like you did for ARM?

I don't think is good practice to define static inline functions under arch/something, then use them under drivers/something_else. It is tolerable if the static inline functions are empty and the driver in question cannot be compiled as module, like in this case for the arm.

In addition the x86 implementation calls arch_(un)register_cpu, which requires #include <asm/cpu.h>, which doesn't compile if added to arch/x86/include/asm/xen/hypervisor.h.

Stefano Stabellini

author

2015-10-22 16:13 UTC #5 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

Quoted 18 lines · Stefano Stabellini

Quoted 16 lines · Boris Ostrovsky
Quoted 13 lines · Stefano Stabellini
arch/x86/include/asm/xen/hypervisor.h static inline bool xen_x2apic_para_available(void) · line 62
Quoted 13 lines
arch/x86/include/asm/xen/hypervisor.h +3 -0
index d866959..8b2d4be 100644
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
  }
  #endif
  +#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif
Why not inline them here, just like you did for ARM?

Quoted 8 lines

I don't think is good practice to define static inline functions under arch/something, then use them under drivers/something_else. It is tolerable if the static inline functions are empty and the driver in question cannot be compiled as module, like in this case for the arm.
In addition the x86 implementation calls arch_(un)register_cpu, which requires #include <asm/cpu.h>, which doesn't compile if added to arch/x86/include/asm/xen/hypervisor.h.

Boris, does this explanation satisfy you?
Do you want me to change anything?

Boris Ostrovsky

reviewer

2015-10-22 16:15 UTC #6 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

Quoted 21 lines · Stefano Stabellini

Quoted 20 lines · Stefano Stabellini
Quoted 14 lines · Boris Ostrovsky
Quoted 4 lines · Stefano Stabellini
Quoted 4 lines
arch/x86/include/asm/xen/hypervisor.h +0 -1
index d866959..8b2d4be 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
Quoted 8 lines
arch/x86/include/asm/xen/hypervisor.h static inline bool xen_x2apic_para_available(void) · line 62
Quoted 8 lines
+++ b/arch/x86/include/asm/xen/hypervisor.h
arch/x86/include/asm/xen/hypervisor.h +3 -0
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
   }
   #endif
   +#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif
Why not inline them here, just like you did for ARM?
I don't think is good practice to define static inline functions under arch/something, then use them under drivers/something_else. It is tolerable if the static inline functions are empty and the driver in question cannot be compiled as module, like in this case for the arm.

Quoted 5 lines

Quoted 3 lines
In addition the x86 implementation calls arch_(un)register_cpu, which requires #include <asm/cpu.h>, which doesn't compile if added to arch/x86/include/asm/xen/hypervisor.h.
Boris, does this explanation satisfy you?
Do you want me to change anything?

Sorry, I forgot to respond!

Reviewed-by Boris Ostrovsky <[email protected]>

Stefano Stabellini

author

2015-10-22 16:22 UTC #7 lore raw

PATCH v4 1/3 xen/arm: Enable cpu_hotplug.c

Quoted 25 lines · Boris Ostrovsky

Quoted 24 lines · Stefano Stabellini
Quoted 23 lines · Stefano Stabellini
Quoted 14 lines · Boris Ostrovsky
Quoted 4 lines · Stefano Stabellini
Quoted 4 lines
arch/x86/include/asm/xen/hypervisor.h +0 -1
index d866959..8b2d4be 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
Quoted 8 lines
arch/x86/include/asm/xen/hypervisor.h static inline bool xen_x2apic_para_available(void) · line 62
Quoted 8 lines
+++ b/arch/x86/include/asm/xen/hypervisor.h
arch/x86/include/asm/xen/hypervisor.h +3 -0
@@ -57,4 +57,9 @@ static inline bool xen_x2apic_para_available(void)
   }
   #endif
   +#ifdef CONFIG_HOTPLUG_CPU
+void xen_arch_register_cpu(int num);
+void xen_arch_unregister_cpu(int num);
+#endif
Why not inline them here, just like you did for ARM?
I don't think is good practice to define static inline functions under arch/something, then use them under drivers/something_else. It is tolerable if the static inline functions are empty and the driver in question cannot be compiled as module, like in this case for the arm.
In addition the x86 implementation calls arch_(un)register_cpu, which requires #include <asm/cpu.h>, which doesn't compile if added to arch/x86/include/asm/xen/hypervisor.h.

Quoted 6 lines

Quoted 2 lines
Boris, does this explanation satisfy you?
Do you want me to change anything?
Sorry, I forgot to respond!
Reviewed-by Boris Ostrovsky <[email protected]>

Fantatic, thank you!
I'll apply to for-linus-4.4.

Stefano Stabellini

author

2015-10-21 11:53 UTC #8 lore raw

PATCH v4 2/3 xen, cpu_hotplug: call device_offline instead of cpu_down

In reply to Stefano Stabellini

When offlining a cpu, instead of cpu_down, call device_offline, which also takes care of updating the cpu.dev.offline field. This keeps the sysfs file /sys/devices/system/cpu/cpuN/online, up to date. Also move the call to disable_hotplug_cpu, because it makes more sense to have it there.

We don't call device_online at cpu-hotplug time, because that would immediately take the cpu online, while we want to retain the current
behaviour: the user needs to explicitly enable the cpu after it has been hotplugged.

Signed-off-by Stefano Stabellini <[email protected]>

CC konrad.wilk at oracle.com

CC boris.ostrovsky at oracle.com

CC david.vrabel at citrix.com

---

Changes in v4:
- protect device_offline with lock/unlock_device_hotplug
- improve commit message

---
 drivers/xen/cpu_hotplug.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/xen/cpu_hotplug.c +5 -1

index 43de1f5..5676aef 100644
@@ -18,6 +18,11 @@ static void enable_hotplug_cpu(int cpu)
 
 static void disable_hotplug_cpu(int cpu)
 {
+	if (cpu_online(cpu)) {
+		lock_device_hotplug();
+		device_offline(get_cpu_device(cpu));
+		unlock_device_hotplug();
+	}
 	if (cpu_present(cpu))
 		xen_arch_unregister_cpu(cpu);

@@ -55,7 +60,6 @@ static void vcpu_hotplug(unsigned int cpu)
 		enable_hotplug_cpu(cpu);
 		break;
 	case 0:
-		(void)cpu_down(cpu);
 		disable_hotplug_cpu(cpu);
 		break;
 	default:

Signature · 1 line

1.7.10.4

Boris Ostrovsky

reviewer

2015-10-21 12:56 UTC #9 lore raw

PATCH v4 2/3 xen, cpu_hotplug: call device_offline instead of cpu_down

Quoted 11 lines · Stefano Stabellini

When offlining a cpu, instead of cpu_down, call device_offline, which also takes care of updating the cpu.dev.offline field. This keeps the sysfs file /sys/devices/system/cpu/cpuN/online, up to date. Also move the call to disable_hotplug_cpu, because it makes more sense to have it there.
We don't call device_online at cpu-hotplug time, because that would immediately take the cpu online, while we want to retain the current
behaviour: the user needs to explicitly enable the cpu after it has been hotplugged.

Quoted 4 lines

Signed-off-by Stefano Stabellini <[email protected]>
CC konrad.wilk at oracle.com
CC boris.ostrovsky at oracle.com
CC david.vrabel at citrix.com

Reviewed-by Boris Ostrovsky <[email protected]>

Stefano Stabellini

author

2015-10-21 11:53 UTC #10 lore raw

PATCH v4 3/3 xen/arm: don't try to re-register vcpu_info on cpu_hotplug.

In reply to Stefano Stabellini

Call disable_percpu_irq on CPU_DYING and enable_percpu_irq when the cpu is coming up.

Signed-off-by Stefano Stabellini <[email protected]>

---

Changes in v4:
- use goto
- call disable_percpu_irq on CPU_DYING

Changes in v3:
- call disable_percpu_irq on CPU_DYING
- call enable_percpu_irq even when VCPUOP_register_vcpu_info has already been called

Changes in v2:
- better comment

---
 arch/arm/xen/enlighten.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/arm/xen/enlighten.c +12 -0

index 6c09cc4..1efa22b 100644
@@ -93,6 +93,14 @@ static void xen_percpu_init(void)
 	int err;
 	int cpu = get_cpu();
 
+	/* 
+	 * VCPUOP_register_vcpu_info cannot be called twice for the same
+	 * vcpu, so if vcpu_info is already registered, just get out. This
+	 * can happen with cpu-hotplug.
+	 */
+	if (per_cpu(xen_vcpu, cpu) != NULL)
+		goto after_register_vcpu_info;
+
 	pr_info("Xen: initializing cpu%d\n", cpu);
 	vcpup = per_cpu_ptr(xen_vcpu_info, cpu);

@@ -103,6 +111,7 @@ static void xen_percpu_init(void)
 	BUG_ON(err);
 	per_cpu(xen_vcpu, cpu) = vcpup;
 
+after_register_vcpu_info:
 	enable_percpu_irq(xen_events_irq, 0);
 	put_cpu();
 }

@@ -131,6 +140,9 @@ static int xen_cpu_notification(struct notifier_block *self,
 	case CPU_STARTING:
 		xen_percpu_init();
 		break;
+	case CPU_DYING:
+		disable_percpu_irq(xen_events_irq);
+		break;
 	default:
 		break;
 	}

Signature · 1 line

1.7.10.4

Julien Grall

2015-10-21 11:55 UTC #11 lore raw

PATCH v4 3/3 xen/arm: don't try to re-register vcpu_info on cpu_hotplug.

Hi Stefano,

Quoted 4 lines · Stefano Stabellini

Call disable_percpu_irq on CPU_DYING and enable_percpu_irq when the cpu is coming up.
Signed-off-by Stefano Stabellini <[email protected]>

Reviewed-by Julien Grall <[email protected]>

Regards,

Signature · 1 line

Julien Grall