[PATCH v2] xen,arm: enable cpu_hotplug

[PATCH v2] xen,arm: enable cpu_hotplug

[Stefano Stabellini]

Hi all,

this small patch series enable cpu_hotplug in ARM and ARM64 guests,
using the PV path to plug and unplug the cpus and psci to enable/disable
them.


Stefano Stabellini (3):
      xen/arm: Enable cpu_hotplug.c
      xen, cpu_hotplug: call device_offline instead of cpu_down
      xen/arm: don't try to re-register vcpu_info on cpu_hotplug.

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

Cheers,

Stefano

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

[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 <stefano.stabellini@eu.citrix.com>

---

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

diff --git a/arch/arm/include/asm/xen/hypervisor.h b/arch/arm/include/asm/xen/hypervisor.h
index 04ff8e7..9525151 100644
--- a/arch/arm/include/asm/xen/hypervisor.h
+++ b/arch/arm/include/asm/xen/hypervisor.h
@@ -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 */
diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index d866959..8b2d4be 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -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 */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 11d6fb4..ba62d8e 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -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
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index e293bc5..aa8a7f7 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -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/
diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index cc6513a..43de1f5 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -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);
-- 
1.7.9.5

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

[Julien Grall]

Hi Stefano,

On 16/10/15 17:34, Stefano Stabellini wrote:
> 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 <stefano.stabellini@eu.citrix.com>

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

[PATCH v2 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down

[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 <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/cpu_hotplug.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index 43de1f5..78804b5 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -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:
-- 
1.7.9.5

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

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/enlighten.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 6c09cc4..b193811 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -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);
 
-- 
1.7.9.5

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

[Julien Grall]

Hi Stefano,

On 16/10/15 17:35, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 6c09cc4..b193811 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -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;
> +	}


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,


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


-- 
Julien Grall

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

[Stefano Stabellini]

On Fri, 16 Oct 2015, Julien Grall wrote:
> Hi Stefano,
> 
> On 16/10/15 17:35, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/arm/xen/enlighten.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 6c09cc4..b193811 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -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;
> > +	}
> 
> 
> 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?

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


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

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

[Julien Grall]

On 19/10/15 11:10, Stefano Stabellini wrote:
> On Fri, 16 Oct 2015, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 16/10/15 17:35, Stefano Stabellini wrote:
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> ---
>>>  arch/arm/xen/enlighten.c |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>>> index 6c09cc4..b193811 100644
>>> --- a/arch/arm/xen/enlighten.c
>>> +++ b/arch/arm/xen/enlighten.c
>>> @@ -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;
>>> +	}
>>
>>
>> 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?
> 
> 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,

-- 
Julien Grall

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

[Stefano Stabellini]

On Mon, 19 Oct 2015, Julien Grall wrote:
> On 19/10/15 11:10, Stefano Stabellini wrote:
> > On Fri, 16 Oct 2015, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 16/10/15 17:35, Stefano Stabellini wrote:
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> ---
> >>>  arch/arm/xen/enlighten.c |   10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >>> index 6c09cc4..b193811 100644
> >>> --- a/arch/arm/xen/enlighten.c
> >>> +++ b/arch/arm/xen/enlighten.c
> >>> @@ -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;
> >>> +	}
> >>
> >>
> >> 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?
> > 
> > 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.


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