[PATCH v3] xen,arm: enable cpu_hotplug

[PATCH v3] 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              |   14 ++++++++++++++
 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, 52 insertions(+), 5 deletions(-)

Cheers,

Stefano

[PATCH v3 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>
Reviewed-by: Julien Grall <julien.grall@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.10.4

[PATCH v3 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>
CC: konrad.wilk at oracle.com
CC: boris.ostrovsky at oracle.com
CC: david.vrabel at 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.10.4

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

[Boris Ostrovsky]

On 10/19/2015 12:55 PM, Stefano Stabellini wrote:
> 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>
> CC: konrad.wilk at oracle.com
> CC: boris.ostrovsky at oracle.com
> CC: david.vrabel at 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);

Do we want to call device_online() when plugging cpu in?

And take sysfs lock?


-boris


>   
> @@ -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:

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

[Stefano Stabellini]

On Mon, 19 Oct 2015, Boris Ostrovsky wrote:
> On 10/19/2015 12:55 PM, Stefano Stabellini wrote:
> > 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>
> > CC: konrad.wilk at oracle.com
> > CC: boris.ostrovsky at oracle.com
> > CC: david.vrabel at 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);
> 
> Do we want to call device_online() when plugging cpu in?

I don't know: that would automatically bring the cpu online after the
user hot-plugs it. Is it what we want? Current one would have to manually:

echo 1 > /sys/devices/system/cpu/cpu1/online

I thought that was the expected behaviour?


> And take sysfs lock?
 
I think you are right: we need to protect the device_offline (and
device_online if we add it) with lock_device_hotplug and
unlock_device_hotplug. Thanks!
 

> >   @@ -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:
> 

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

[Boris Ostrovsky]

On 10/20/2015 06:28 AM, Stefano Stabellini wrote:
> On Mon, 19 Oct 2015, Boris Ostrovsky wrote:
>> On 10/19/2015 12:55 PM, Stefano Stabellini wrote:
>>> 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>
>>> CC: konrad.wilk at oracle.com
>>> CC: boris.ostrovsky at oracle.com
>>> CC: david.vrabel at 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);
>> Do we want to call device_online() when plugging cpu in?
> I don't know: that would automatically bring the cpu online after the
> user hot-plugs it. Is it what we want? Current one would have to manually:
>
> echo 1 > /sys/devices/system/cpu/cpu1/online
>
> I thought that was the expected behaviour?

Right, we should keep onlining as an explicit operation. I think it's 
worth mentioning in the commit message why we don't call device_online() 
while using device_offline(), which is its complement.


-boris

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

[Stefano Stabellini]

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

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 |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 6c09cc4..074b1a2 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -93,6 +93,17 @@ 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) {
+		enable_percpu_irq(xen_events_irq, 0);
+		put_cpu();
+		return;
+	}
+
 	pr_info("Xen: initializing cpu%d\n", cpu);
 	vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
 
@@ -131,6 +142,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;
 	}
-- 
1.7.10.4

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

[Julien Grall]

Hi Stefano,

On 19/10/15 17:55, Stefano Stabellini wrote:
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 6c09cc4..074b1a2 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -93,6 +93,17 @@ 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) {
> +		enable_percpu_irq(xen_events_irq, 0);
> +		put_cpu();
> +		return;

I might be too picky, but this function can be reworked to avoid
duplicating the code. Something like:

if (per_cpu(xen_vcpu, cpu) != NULL)
  goto skip_register_vcpu;

> +	}
> +
>  	pr_info("Xen: initializing cpu%d\n", cpu);
>  	vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>  
> @@ -131,6 +142,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;
>  	}
> 


-- 
Julien Grall