linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] xen,arm: enable cpu_hotplug
@ 2015-10-21 11:52 Stefano Stabellini
  2015-10-21 11:53 ` [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c Stefano Stabellini
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-21 11:52 UTC (permalink / raw)
  To: linux-arm-kernel

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              |   12 ++++++++++++
 arch/x86/include/asm/xen/hypervisor.h |    5 +++++
 arch/x86/xen/enlighten.c              |   15 +++++++++++++++
 drivers/xen/Makefile                  |    2 --
 drivers/xen/cpu_hotplug.c             |   14 +++++++++++---
 6 files changed, 53 insertions(+), 5 deletions(-)

Cheers,

Stefano

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-21 11:52 [PATCH v4] xen,arm: enable cpu_hotplug Stefano Stabellini
@ 2015-10-21 11:53 ` Stefano Stabellini
  2015-10-21 13:00   ` Stefano Stabellini
  2015-10-21 11:53 ` [PATCH v4 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down Stefano Stabellini
  2015-10-21 11:53 ` [PATCH v4 3/3] xen/arm: don't try to re-register vcpu_info on cpu_hotplug Stefano Stabellini
  2 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-21 11:53 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down
  2015-10-21 11:52 [PATCH v4] xen,arm: enable cpu_hotplug Stefano Stabellini
  2015-10-21 11:53 ` [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c Stefano Stabellini
@ 2015-10-21 11:53 ` Stefano Stabellini
  2015-10-21 12:56   ` Boris Ostrovsky
  2015-10-21 11:53 ` [PATCH v4 3/3] xen/arm: don't try to re-register vcpu_info on cpu_hotplug Stefano Stabellini
  2 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-21 11:53 UTC (permalink / raw)
  To: linux-arm-kernel

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

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 3/3] xen/arm: don't try to re-register vcpu_info on cpu_hotplug.
  2015-10-21 11:52 [PATCH v4] xen,arm: enable cpu_hotplug Stefano Stabellini
  2015-10-21 11:53 ` [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c Stefano Stabellini
  2015-10-21 11:53 ` [PATCH v4 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down Stefano Stabellini
@ 2015-10-21 11:53 ` Stefano Stabellini
  2015-10-21 11:55   ` [Xen-devel] " Julien Grall
  2 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-21 11:53 UTC (permalink / raw)
  To: linux-arm-kernel

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

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 6c09cc4..1efa22b 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -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;
 	}
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Xen-devel] [PATCH v4 3/3] xen/arm: don't try to re-register vcpu_info on cpu_hotplug.
  2015-10-21 11:53 ` [PATCH v4 3/3] xen/arm: don't try to re-register vcpu_info on cpu_hotplug Stefano Stabellini
@ 2015-10-21 11:55   ` Julien Grall
  0 siblings, 0 replies; 12+ messages in thread
From: Julien Grall @ 2015-10-21 11:55 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Stefano,

On 21/10/15 12:53, Stefano Stabellini wrote:
> 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>

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

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down
  2015-10-21 11:53 ` [PATCH v4 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down Stefano Stabellini
@ 2015-10-21 12:56   ` Boris Ostrovsky
  0 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2015-10-21 12:56 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/21/2015 07:53 AM, 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.
>
> 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 <stefano.stabellini@eu.citrix.com>
> CC: konrad.wilk at oracle.com
> CC: boris.ostrovsky at oracle.com
> CC: david.vrabel at citrix.com

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-21 11:53 ` [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c Stefano Stabellini
@ 2015-10-21 13:00   ` Stefano Stabellini
  2015-10-21 13:19     ` Boris Ostrovsky
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-21 13:00 UTC (permalink / raw)
  To: linux-arm-kernel

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.

On Wed, 21 Oct 2015, 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>
> 
> ---
> 
> 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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-21 13:00   ` Stefano Stabellini
@ 2015-10-21 13:19     ` Boris Ostrovsky
  2015-10-21 15:36       ` Stefano Stabellini
  0 siblings, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2015-10-21 13:19 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/21/2015 09:00 AM, Stefano Stabellini wrote:
>>
>> 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

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


-boris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-21 13:19     ` Boris Ostrovsky
@ 2015-10-21 15:36       ` Stefano Stabellini
  2015-10-22 16:13         ` Stefano Stabellini
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-21 15:36 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, 21 Oct 2015, Boris Ostrovsky wrote:
> On 10/21/2015 09:00 AM, Stefano Stabellini wrote:
> > > 
> > > 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
> 
> 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-21 15:36       ` Stefano Stabellini
@ 2015-10-22 16:13         ` Stefano Stabellini
  2015-10-22 16:15           ` Boris Ostrovsky
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-22 16:13 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, 21 Oct 2015, Stefano Stabellini wrote:
> On Wed, 21 Oct 2015, Boris Ostrovsky wrote:
> > On 10/21/2015 09:00 AM, Stefano Stabellini wrote:
> > > > 
> > > > 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
> > 
> > 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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-22 16:13         ` Stefano Stabellini
@ 2015-10-22 16:15           ` Boris Ostrovsky
  2015-10-22 16:22             ` Stefano Stabellini
  0 siblings, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2015-10-22 16:15 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/22/2015 12:13 PM, Stefano Stabellini wrote:
> On Wed, 21 Oct 2015, Stefano Stabellini wrote:
>> On Wed, 21 Oct 2015, Boris Ostrovsky wrote:
>>> On 10/21/2015 09:00 AM, Stefano Stabellini wrote:
>>>>> 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
>>> 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.
> Boris, does this explanation satisfy you?
> Do you want me to change anything?

Sorry, I forgot to respond!

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c
  2015-10-22 16:15           ` Boris Ostrovsky
@ 2015-10-22 16:22             ` Stefano Stabellini
  0 siblings, 0 replies; 12+ messages in thread
From: Stefano Stabellini @ 2015-10-22 16:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 22 Oct 2015, Boris Ostrovsky wrote:
> On 10/22/2015 12:13 PM, Stefano Stabellini wrote:
> > On Wed, 21 Oct 2015, Stefano Stabellini wrote:
> > > On Wed, 21 Oct 2015, Boris Ostrovsky wrote:
> > > > On 10/21/2015 09:00 AM, Stefano Stabellini wrote:
> > > > > > 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
> > > > 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.
> > Boris, does this explanation satisfy you?
> > Do you want me to change anything?
> 
> Sorry, I forgot to respond!
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-10-22 16:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-21 11:52 [PATCH v4] xen,arm: enable cpu_hotplug Stefano Stabellini
2015-10-21 11:53 ` [PATCH v4 1/3] xen/arm: Enable cpu_hotplug.c Stefano Stabellini
2015-10-21 13:00   ` Stefano Stabellini
2015-10-21 13:19     ` Boris Ostrovsky
2015-10-21 15:36       ` Stefano Stabellini
2015-10-22 16:13         ` Stefano Stabellini
2015-10-22 16:15           ` Boris Ostrovsky
2015-10-22 16:22             ` Stefano Stabellini
2015-10-21 11:53 ` [PATCH v4 2/3] xen, cpu_hotplug: call device_offline instead of cpu_down Stefano Stabellini
2015-10-21 12:56   ` Boris Ostrovsky
2015-10-21 11:53 ` [PATCH v4 3/3] xen/arm: don't try to re-register vcpu_info on cpu_hotplug Stefano Stabellini
2015-10-21 11:55   ` [Xen-devel] " Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).