xen,arm: enable cpu_hotplug

PATCH Superseded Stefano Stabellini 2 participants 9 messages Oct 16, 2015 – Oct 19, 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-16 16:34 UTC #1 lore raw

PATCH v2 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]>

---

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

Julien Grall

2015-10-16 16:40 UTC #2 lore raw

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

Hi Stefano,

Quoted 9 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]>

Regards,

Signature · 1 line

Julien Grall

Stefano Stabellini

author

2015-10-16 16:35 UTC #3 lore raw

PATCH v2 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.

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

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

drivers/xen/cpu_hotplug.c +2 -1

index 43de1f5..78804b5 100644
@@ -18,6 +18,8 @@ static void enable_hotplug_cpu(int cpu)
 
 static void disable_hotplug_cpu(int cpu)
 {
+	if (cpu_online(cpu))
+		device_offline(get_cpu_device(cpu));
 	if (cpu_present(cpu))
 		xen_arch_unregister_cpu(cpu);

@@ -55,7 +57,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.9.5

Stefano Stabellini

author

2015-10-16 16:35 UTC #4 lore raw

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

In reply to Stefano Stabellini

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

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

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

index 6c09cc4..b193811 100644
@@ -93,6 +93,16 @@ 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) {
+		put_cpu();
+		return;
+	}
+
 	pr_info("Xen: initializing cpu%d\n", cpu);
 	vcpup = per_cpu_ptr(xen_vcpu_info, cpu);

Signature · 1 line

1.7.9.5

Julien Grall

2015-10-16 16:49 UTC #5 lore raw

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

Hi Stefano,

Quoted 12 lines · Stefano Stabellini

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

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

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

index 6c09cc4..b193811 100644
@@ -93,6 +93,16 @@ static void xen_percpu_init(void)
 	int err;
 	int cpu = get_cpu();

arch/arm/xen/enlighten.c static void xen_percpu_init(void) · line 104

Quoted 10 lines

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

 
+	/* 
+	 * 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) {
+		put_cpu();
+		return;
+	}

Not really related to this patch. By side effect this patch is also not calling irq_percpu_enable.

Looking around, all the caller of irq_percpu_enable will call irq_percpu_disable when the CPU is dying. Is there any side effect to not doing this?

Regards,

Quoted 5 lines

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

+
 	pr_info("Xen: initializing cpu%d\n", cpu);
 	vcpup = per_cpu_ptr(xen_vcpu_info, cpu);

Signature · 1 line

Julien Grall

Stefano Stabellini

author

2015-10-19 10:10 UTC #6 lore raw

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

Quoted 30 lines · Julien Grall

Hi Stefano,
Quoted 12 lines · Stefano Stabellini
Signed-off-by Stefano Stabellini <[email protected]>
---
 arch/arm/xen/enlighten.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
arch/arm/xen/enlighten.c +0 -0
index 6c09cc4..b193811 100644
@@ -93,6 +93,16 @@ static void xen_percpu_init(void)
 	int err;
 	int cpu = get_cpu();
arch/arm/xen/enlighten.c static void xen_percpu_init(void) · line 104
Quoted 10 lines
arch/arm/xen/enlighten.c +9 -0
 
+	/* 
+	 * 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) {
+		put_cpu();
+		return;
+	}
Not really related to this patch. By side effect this patch is also not calling irq_percpu_enable.

Quoted 3 lines

Looking around, all the caller of irq_percpu_enable will call irq_percpu_disable when the CPU is dying. Is there any side effect to not doing this?

Given that Xen won't inject any event irqs when a cpu is offline, I don't think there are any side effects.

Quoted 10 lines

Quoted 5 lines
arch/arm/xen/enlighten.c +1 -0
+
 	pr_info("Xen: initializing cpu%d\n", cpu);
 	vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
 
Signature · 1 line
Julien Grall

Julien Grall

2015-10-19 10:27 UTC #7 lore raw

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

Quoted 31 lines · Stefano Stabellini

Quoted 30 lines · Julien Grall
Hi Stefano,
Quoted 12 lines · Stefano Stabellini
Signed-off-by Stefano Stabellini <[email protected]>
---
 arch/arm/xen/enlighten.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
arch/arm/xen/enlighten.c +0 -0
index 6c09cc4..b193811 100644
@@ -93,6 +93,16 @@ static void xen_percpu_init(void)
 	int err;
 	int cpu = get_cpu();
arch/arm/xen/enlighten.c static void xen_percpu_init(void) · line 104
Quoted 10 lines
arch/arm/xen/enlighten.c +9 -0
 
+	/* 
+	 * 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) {
+		put_cpu();
+		return;
+	}
Not really related to this patch. By side effect this patch is also not calling irq_percpu_enable.

Quoted 6 lines

Quoted 3 lines
Looking around, all the caller of irq_percpu_enable will call irq_percpu_disable when the CPU is dying. Is there any side effect to not doing this?
Given that Xen won't inject any event irqs when a cpu is offline, I don't think there are any side effects.

I'm not talking about the behavior of Xen when the vCPU is down but any side effect in Linux (i.e not re-enabling the IRQ or else).

All the other user of percpu IRQ is doing the call so there must be a reason to do it.

Looking more closely to the code, the GIC driver will reset the vCPU interface every time the vCPU is coming back online (see gic_secondary_init).

Resetting the vCPU interface means all the PPIs will be disabled (see gic_cpu_config). As the event channel IRQ is a PPI it won't be re-enabled and the vCPU will never receive event channel.

So you have to re-enable this interrupt unconditionally. At the same time, I'd like to see the irq_percpu_disable to match irq_percpu_enable as the other users do.

Regards,

Signature · 1 line

Julien Grall

Stefano Stabellini

author

2015-10-19 16:50 UTC #8 lore raw

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

Quoted 49 lines · Julien Grall

Quoted 31 lines · Stefano Stabellini
Quoted 30 lines · Julien Grall
Hi Stefano,
Quoted 12 lines · Stefano Stabellini
Signed-off-by Stefano Stabellini <[email protected]>
---
 arch/arm/xen/enlighten.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
arch/arm/xen/enlighten.c +0 -0
index 6c09cc4..b193811 100644
@@ -93,6 +93,16 @@ static void xen_percpu_init(void)
 	int err;
 	int cpu = get_cpu();
arch/arm/xen/enlighten.c static void xen_percpu_init(void) · line 104
Quoted 10 lines
arch/arm/xen/enlighten.c +9 -0
 
+	/* 
+	 * 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) {
+		put_cpu();
+		return;
+	}
Not really related to this patch. By side effect this patch is also not calling irq_percpu_enable.
Quoted 6 lines
Quoted 3 lines
Looking around, all the caller of irq_percpu_enable will call irq_percpu_disable when the CPU is dying. Is there any side effect to not doing this?
Given that Xen won't inject any event irqs when a cpu is offline, I don't think there are any side effects.
I'm not talking about the behavior of Xen when the vCPU is down but any side effect in Linux (i.e not re-enabling the IRQ or else).
All the other user of percpu IRQ is doing the call so there must be a reason to do it.
Looking more closely to the code, the GIC driver will reset the vCPU interface every time the vCPU is coming back online (see gic_secondary_init).

Quoted 7 lines

Resetting the vCPU interface means all the PPIs will be disabled (see gic_cpu_config). As the event channel IRQ is a PPI it won't be re-enabled and the vCPU will never receive event channel.
So you have to re-enable this interrupt unconditionally. At the same time, I'd like to see the irq_percpu_disable to match irq_percpu_enable as the other users do.

OK, I'll add a disable_percpu_irq call and an enable_percpu_irq call.