[PATCH 0/6] Support calling functions on dedicated physical cpu

[PATCH 0/6] Support calling functions on dedicated physical cpu

[Juergen Gross]

Some hardware (e.g. Dell Studio laptops) require special functions to
be called on physical cpu 0 in order to avoid occasional hangs. When
running as dom0 under Xen this could be achieved only via special boot
parameters (vcpu pinning) limiting the hypervisor in it's scheduling
decisions.

This patch series is adding a generic function to be able to temporarily
pin a (virtual) cpu to a dedicated physical cpu for executing above
mentioned functions on that specific cpu. The drivers (dcdbas and i8k)
requiring this functionality are modified accordingly.

Juergen Gross (6):
  xen: sync xen header
  sched: add function to execute a function synchronously on a physical
    cpu
  dcdbas: make use of call_sync_on_phys_cpu()
  hwmon: use call_sync_on_phys_cpu() for dell-smm i8k
  virt, sched: add cpu pinning to call_sync_on_phys_cpu()
  xen: add xen_pin_vcpu() to support calling functions on a dedicated
    pcpu

 arch/x86/include/asm/hypervisor.h |   9 ++++
 arch/x86/xen/enlighten.c          |  40 +++++++++++++++
 drivers/firmware/dcdbas.c         |  46 ++++++++----------
 drivers/hwmon/dell-smm-hwmon.c    |  27 +++++-----
 include/linux/sched.h             |  31 ++++++++++++
 include/xen/interface/sched.h     | 100 +++++++++++++++++++++++++++++++-------
 kernel/sched/core.c               |  30 ++++++++++++
 7 files changed, 224 insertions(+), 59 deletions(-)

-- 
2.6.2

[PATCH 1/6] xen: sync xen header

[Juergen Gross]

Import the actual version of include/xen/interface/sched.h from Xen.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/xen/interface/sched.h | 100 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 82 insertions(+), 18 deletions(-)

diff --git a/include/xen/interface/sched.h b/include/xen/interface/sched.h
index f184909..a4c4d73 100644
--- a/include/xen/interface/sched.h
+++ b/include/xen/interface/sched.h
@@ -3,6 +3,24 @@
  *
  * Scheduler state interactions
  *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
  * Copyright (c) 2005, Keir Fraser <keir@xensource.com>
  */
 
@@ -12,18 +30,30 @@
 #include <xen/interface/event_channel.h>
 
 /*
+ * Guest Scheduler Operations
+ *
+ * The SCHEDOP interface provides mechanisms for a guest to interact
+ * with the scheduler, including yield, blocking and shutting itself
+ * down.
+ */
+
+/*
  * The prototype for this hypercall is:
- *  long sched_op_new(int cmd, void *arg)
+ * long HYPERVISOR_sched_op(enum sched_op cmd, void *arg, ...)
+ *
  * @cmd == SCHEDOP_??? (scheduler operation).
  * @arg == Operation-specific extra argument(s), as described below.
+ * ...  == Additional Operation-specific extra arguments, described below.
  *
- * **NOTE**:
- * Versions of Xen prior to 3.0.2 provide only the following legacy version
+ * Versions of Xen prior to 3.0.2 provided only the following legacy version
  * of this hypercall, supporting only the commands yield, block and shutdown:
  *  long sched_op(int cmd, unsigned long arg)
  * @cmd == SCHEDOP_??? (scheduler operation).
  * @arg == 0               (SCHEDOP_yield and SCHEDOP_block)
  *      == SHUTDOWN_* code (SCHEDOP_shutdown)
+ *
+ * This legacy version is available to new guests as:
+ * long HYPERVISOR_sched_op_compat(enum sched_op cmd, unsigned long arg)
  */
 
 /*
@@ -44,12 +74,17 @@
 /*
  * Halt execution of this domain (all VCPUs) and notify the system controller.
  * @arg == pointer to sched_shutdown structure.
+ *
+ * If the sched_shutdown_t reason is SHUTDOWN_suspend then
+ * x86 PV guests must also set RDX (EDX for 32-bit guests) to the MFN
+ * of the guest's start info page.  RDX/EDX is the third hypercall
+ * argument.
+ *
+ * In addition, which reason is SHUTDOWN_suspend this hypercall
+ * returns 1 if suspend was cancelled or the domain was merely
+ * checkpointed, and 0 if it is resuming in a new domain.
  */
 #define SCHEDOP_shutdown    2
-struct sched_shutdown {
-    unsigned int reason; /* SHUTDOWN_* */
-};
-DEFINE_GUEST_HANDLE_STRUCT(sched_shutdown);
 
 /*
  * Poll a set of event-channel ports. Return when one or more are pending. An
@@ -57,12 +92,6 @@ DEFINE_GUEST_HANDLE_STRUCT(sched_shutdown);
  * @arg == pointer to sched_poll structure.
  */
 #define SCHEDOP_poll        3
-struct sched_poll {
-    GUEST_HANDLE(evtchn_port_t) ports;
-    unsigned int nr_ports;
-    uint64_t timeout;
-};
-DEFINE_GUEST_HANDLE_STRUCT(sched_poll);
 
 /*
  * Declare a shutdown for another domain. The main use of this function is
@@ -71,15 +100,11 @@ DEFINE_GUEST_HANDLE_STRUCT(sched_poll);
  * @arg == pointer to sched_remote_shutdown structure.
  */
 #define SCHEDOP_remote_shutdown        4
-struct sched_remote_shutdown {
-    domid_t domain_id;         /* Remote domain ID */
-    unsigned int reason;       /* SHUTDOWN_xxx reason */
-};
 
 /*
  * Latch a shutdown code, so that when the domain later shuts down it
  * reports this code to the control tools.
- * @arg == as for SCHEDOP_shutdown.
+ * @arg == sched_shutdown, as for SCHEDOP_shutdown.
  */
 #define SCHEDOP_shutdown_code 5
 
@@ -92,10 +117,47 @@ struct sched_remote_shutdown {
  * With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
  */
 #define SCHEDOP_watchdog    6
+
+/*
+ * Override the current vcpu affinity by pinning it to one physical cpu or
+ * undo this override restoring the previous affinity.
+ * @arg == pointer to sched_pin_override structure.
+ *
+ * A negative pcpu value will undo a previous pin override and restore the
+ * previous cpu affinity.
+ * This call is allowed for the hardware domain only and requires the cpu
+ * to be part of the domain's cpupool.
+ */
+#define SCHEDOP_pin_override 7
+
+struct sched_shutdown {
+    unsigned int reason; /* SHUTDOWN_* => shutdown reason */
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_shutdown);
+
+struct sched_poll {
+    GUEST_HANDLE(evtchn_port_t) ports;
+    unsigned int nr_ports;
+    uint64_t timeout;
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_poll);
+
+struct sched_remote_shutdown {
+    domid_t domain_id;         /* Remote domain ID */
+    unsigned int reason;       /* SHUTDOWN_* => shutdown reason */
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_remote_shutdown);
+
 struct sched_watchdog {
     uint32_t id;                /* watchdog ID */
     uint32_t timeout;           /* timeout */
 };
+DEFINE_GUEST_HANDLE_STRUCT(sched_watchdog);
+
+struct sched_pin_override {
+    int32_t pcpu;
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_pin_override);
 
 /*
  * Reason codes for SCHEDOP_shutdown. These may be interpreted by control
@@ -107,6 +169,7 @@ struct sched_watchdog {
 #define SHUTDOWN_suspend    2  /* Clean up, save suspend info, kill.         */
 #define SHUTDOWN_crash      3  /* Tell controller we've crashed.             */
 #define SHUTDOWN_watchdog   4  /* Restart because watchdog time expired.     */
+
 /*
  * Domain asked to perform 'soft reset' for it. The expected behavior is to
  * reset internal Xen state for the domain returning it to the point where it
@@ -115,5 +178,6 @@ struct sched_watchdog {
  * interfaces again.
  */
 #define SHUTDOWN_soft_reset 5
+#define SHUTDOWN_MAX        5  /* Maximum valid shutdown reason.             */
 
 #endif /* __XEN_PUBLIC_SCHED_H__ */
-- 
2.6.2

[PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Juergen Gross]

On some hardware models (e.g. Dell Studio 1555 laptop) some hardware
related functions (e.g. SMIs) are to be executed on physical cpu 0
only. Instead of open coding such a functionality multiple times in
the kernel add a service function for this purpose. This will enable
the possibility to take special measures in virtualized environments
like Xen, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/linux/sched.h |  9 +++++++++
 kernel/sched/core.c   | 26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a10494a..dfadf1a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2247,6 +2247,7 @@ extern void do_set_cpus_allowed(struct task_struct *p,
 
 extern int set_cpus_allowed_ptr(struct task_struct *p,
 				const struct cpumask *new_mask);
+int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par);
 #else
 static inline void do_set_cpus_allowed(struct task_struct *p,
 				      const struct cpumask *new_mask)
@@ -2259,6 +2260,14 @@ static inline int set_cpus_allowed_ptr(struct task_struct *p,
 		return -EINVAL;
 	return 0;
 }
+static inline int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *),
+					void *par)
+{
+	if (cpu != 0)
+		return -EINVAL;
+
+	return func(par);
+}
 #endif
 
 #ifdef CONFIG_NO_HZ_COMMON
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 41f6b22..cb9955f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1265,6 +1265,32 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 }
 EXPORT_SYMBOL_GPL(set_cpus_allowed_ptr);
 
+int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
+{
+	cpumask_var_t old_mask;
+	int ret;
+
+	if (cpu >= nr_cpu_ids)
+		return -EINVAL;
+
+	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
+		return -ENOMEM;
+
+	cpumask_copy(old_mask, &current->cpus_allowed);
+	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
+	if (ret)
+		goto out;
+
+	ret = func(par);
+
+	set_cpus_allowed_ptr(current, old_mask);
+
+out:
+	free_cpumask_var(old_mask);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(call_sync_on_phys_cpu);
+
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
 #ifdef CONFIG_SCHED_DEBUG
-- 
2.6.2

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Peter Zijlstra]

On Fri, Mar 11, 2016 at 12:59:30PM +0100, Juergen Gross wrote:
> +int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
> +{
> +	cpumask_var_t old_mask;
> +	int ret;
> +
> +	if (cpu >= nr_cpu_ids)
> +		return -EINVAL;
> +
> +	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	cpumask_copy(old_mask, &current->cpus_allowed);
> +	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
> +	if (ret)
> +		goto out;

So what happens if someone does sched_setaffinity() right about here?

> +
> +	ret = func(par);
> +
> +	set_cpus_allowed_ptr(current, old_mask);
> +
> +out:
> +	free_cpumask_var(old_mask);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(call_sync_on_phys_cpu);

This is disgusting, and you're adding this to !Xen kernels too.

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Peter Zijlstra]

On Fri, Mar 11, 2016 at 01:19:50PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 11, 2016 at 12:59:30PM +0100, Juergen Gross wrote:
> > +int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
> > +{
> > +	cpumask_var_t old_mask;
> > +	int ret;
> > +
> > +	if (cpu >= nr_cpu_ids)
> > +		return -EINVAL;
> > +
> > +	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
> > +		return -ENOMEM;
> > +
> > +	cpumask_copy(old_mask, &current->cpus_allowed);
> > +	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
> > +	if (ret)
> > +		goto out;
> 
> So what happens if someone does sched_setaffinity() right about here?
> 
> > +
> > +	ret = func(par);
> > +
> > +	set_cpus_allowed_ptr(current, old_mask);
> > +
> > +out:
> > +	free_cpumask_var(old_mask);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(call_sync_on_phys_cpu);
> 
> This is disgusting, and you're adding this to !Xen kernels too.

how about something like:

struct xen_callback_struct {
	struct work_struct	work;
	struct completion	done;
	void *			data;
	int			ret;
};

static void xen_callback_f(struct work_struct *work)
{
	struct xen_callback_struct *xcs = container_of(work, struct xen_callback_struct, work);

	xcs->ret = xcs->func(xcs->data);

	complete(&xcs->done);
}

xen_call_on_cpu_sync(int cpu, int (*func)(void *), void *data)
{
	struct xen_callback_state xcs = {
		.work = __WORK_INITIALIZER(xcs.work, xen_callback_f);
		.done = COMPLETION_INITIALIZER_ONSTACK(xcs.done),
		.data = data,
	};

	queue_work_on(&work, cpu);
	wait_for_completion(&xcs.done);

	return xcs.ret;
}

No mucking about with the scheduler state, no new exported functions
etc..

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Juergen Gross]

On 11/03/16 13:42, Peter Zijlstra wrote:
> On Fri, Mar 11, 2016 at 01:19:50PM +0100, Peter Zijlstra wrote:
>> On Fri, Mar 11, 2016 at 12:59:30PM +0100, Juergen Gross wrote:
>>> +int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
>>> +{
>>> +	cpumask_var_t old_mask;
>>> +	int ret;
>>> +
>>> +	if (cpu >= nr_cpu_ids)
>>> +		return -EINVAL;
>>> +
>>> +	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
>>> +		return -ENOMEM;
>>> +
>>> +	cpumask_copy(old_mask, &current->cpus_allowed);
>>> +	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
>>> +	if (ret)
>>> +		goto out;
>>
>> So what happens if someone does sched_setaffinity() right about here?
>>
>>> +
>>> +	ret = func(par);
>>> +
>>> +	set_cpus_allowed_ptr(current, old_mask);
>>> +
>>> +out:
>>> +	free_cpumask_var(old_mask);
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(call_sync_on_phys_cpu);
>>
>> This is disgusting, and you're adding this to !Xen kernels too.
> 
> how about something like:
> 
> struct xen_callback_struct {
> 	struct work_struct	work;
> 	struct completion	done;
> 	void *			data;
> 	int			ret;
> };
> 
> static void xen_callback_f(struct work_struct *work)
> {
> 	struct xen_callback_struct *xcs = container_of(work, struct xen_callback_struct, work);
> 
> 	xcs->ret = xcs->func(xcs->data);
> 
> 	complete(&xcs->done);
> }
> 
> xen_call_on_cpu_sync(int cpu, int (*func)(void *), void *data)
> {
> 	struct xen_callback_state xcs = {
> 		.work = __WORK_INITIALIZER(xcs.work, xen_callback_f);
> 		.done = COMPLETION_INITIALIZER_ONSTACK(xcs.done),
> 		.data = data,
> 	};
> 
> 	queue_work_on(&work, cpu);
> 	wait_for_completion(&xcs.done);
> 
> 	return xcs.ret;
> }
> 
> No mucking about with the scheduler state, no new exported functions
> etc..
> 

Hey, I like it. Can't be limited to Xen as on bare metal the function
needs to be called on cpu 0, too. But avoiding the scheduler fiddling
is much better! As this seems to be required for Dell hardware only,
I could add it to some Dell base driver in case you don't want to add
it to core code.


Juergen

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Peter Zijlstra]

On Fri, Mar 11, 2016 at 01:48:12PM +0100, Juergen Gross wrote:
> On 11/03/16 13:42, Peter Zijlstra wrote:
> > how about something like:
> > 
> > struct xen_callback_struct {
> > 	struct work_struct	work;
> > 	struct completion	done;
	int			(*func)(void*);
> > 	void *			data;
> > 	int			ret;
> > };
> > 
> > static void xen_callback_f(struct work_struct *work)
> > {
> > 	struct xen_callback_struct *xcs = container_of(work, struct xen_callback_struct, work);
> > 
> > 	xcs->ret = xcs->func(xcs->data);
> > 
> > 	complete(&xcs->done);
> > }
> > 
> > xen_call_on_cpu_sync(int cpu, int (*func)(void *), void *data)
> > {
> > 	struct xen_callback_state xcs = {
> > 		.work = __WORK_INITIALIZER(xcs.work, xen_callback_f);
> > 		.done = COMPLETION_INITIALIZER_ONSTACK(xcs.done),
		.func = func,
> > 		.data = data,
> > 	};
> > 
> > 	queue_work_on(&work, cpu);
> > 	wait_for_completion(&xcs.done);
> > 
> > 	return xcs.ret;
> > }
> > 
> > No mucking about with the scheduler state, no new exported functions
> > etc..
> > 
> 
> Hey, I like it. Can't be limited to Xen as on bare metal the function
> needs to be called on cpu 0, too. But avoiding the scheduler fiddling
> is much better! As this seems to be required for Dell hardware only,
> I could add it to some Dell base driver in case you don't want to add
> it to core code.

Urgh yeah, saw that in your other mail. It looks like I should go look
at set_cpus_allowed_ptr() abuse :/

Not sure where this would fit best, maybe somewhere near workqueue.c or
smp.c.

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Juergen Gross]

On 11/03/16 13:57, Peter Zijlstra wrote:
> On Fri, Mar 11, 2016 at 01:48:12PM +0100, Juergen Gross wrote:
>> On 11/03/16 13:42, Peter Zijlstra wrote:
>>> how about something like:
>>>
>>> struct xen_callback_struct {
>>> 	struct work_struct	work;
>>> 	struct completion	done;
> 	int			(*func)(void*);
>>> 	void *			data;
>>> 	int			ret;
>>> };
>>>
>>> static void xen_callback_f(struct work_struct *work)
>>> {
>>> 	struct xen_callback_struct *xcs = container_of(work, struct xen_callback_struct, work);
>>>
>>> 	xcs->ret = xcs->func(xcs->data);
>>>
>>> 	complete(&xcs->done);
>>> }
>>>
>>> xen_call_on_cpu_sync(int cpu, int (*func)(void *), void *data)
>>> {
>>> 	struct xen_callback_state xcs = {
>>> 		.work = __WORK_INITIALIZER(xcs.work, xen_callback_f);
>>> 		.done = COMPLETION_INITIALIZER_ONSTACK(xcs.done),
> 		.func = func,
>>> 		.data = data,
>>> 	};
>>>
>>> 	queue_work_on(&work, cpu);
>>> 	wait_for_completion(&xcs.done);
>>>
>>> 	return xcs.ret;
>>> }
>>>
>>> No mucking about with the scheduler state, no new exported functions
>>> etc..
>>>
>>
>> Hey, I like it. Can't be limited to Xen as on bare metal the function
>> needs to be called on cpu 0, too. But avoiding the scheduler fiddling
>> is much better! As this seems to be required for Dell hardware only,
>> I could add it to some Dell base driver in case you don't want to add
>> it to core code.
> 
> Urgh yeah, saw that in your other mail. It looks like I should go look
> at set_cpus_allowed_ptr() abuse :/
> 
> Not sure where this would fit best, maybe somewhere near workqueue.c or
> smp.c.

At a first glance I think smp.c would be the better choice. I'll have a try.

Thanks,

Juergen

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Juergen Gross]

On 11/03/16 13:19, Peter Zijlstra wrote:
> On Fri, Mar 11, 2016 at 12:59:30PM +0100, Juergen Gross wrote:
>> +int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
>> +{
>> +	cpumask_var_t old_mask;
>> +	int ret;
>> +
>> +	if (cpu >= nr_cpu_ids)
>> +		return -EINVAL;
>> +
>> +	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
>> +		return -ENOMEM;
>> +
>> +	cpumask_copy(old_mask, &current->cpus_allowed);
>> +	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
>> +	if (ret)
>> +		goto out;
> 
> So what happens if someone does sched_setaffinity() right about here?

Aah, okay. Seems I should add preempt_disable() in this patch already
and retry the set_cpus_allowed_ptr() in case cpus_allowed has changed.

> 
>> +
>> +	ret = func(par);
>> +
>> +	set_cpus_allowed_ptr(current, old_mask);
>> +
>> +out:
>> +	free_cpumask_var(old_mask);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(call_sync_on_phys_cpu);
> 
> This is disgusting, and you're adding this to !Xen kernels too.

Sure. It is called on !Xen kernels too. Without Xen it is just omitting
the vcpu pinning. I've copied above logic from dcdbas/i8k (it was open
coded twice).

BTW: I tried to get rid of the complete logic to call a function on cpu
0 only. It was rejected by the Dell maintainers.


Juergen

Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Peter Zijlstra]

On Fri, Mar 11, 2016 at 01:43:53PM +0100, Juergen Gross wrote:
> On 11/03/16 13:19, Peter Zijlstra wrote:
> > On Fri, Mar 11, 2016 at 12:59:30PM +0100, Juergen Gross wrote:
> >> +int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
> >> +{
> >> +	cpumask_var_t old_mask;
> >> +	int ret;
> >> +
> >> +	if (cpu >= nr_cpu_ids)
> >> +		return -EINVAL;
> >> +
> >> +	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
> >> +		return -ENOMEM;
> >> +
> >> +	cpumask_copy(old_mask, &current->cpus_allowed);
> >> +	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
> >> +	if (ret)
> >> +		goto out;
> > 
> > So what happens if someone does sched_setaffinity() right about here?
> 
> Aah, okay. Seems I should add preempt_disable() in this patch already
> and retry the set_cpus_allowed_ptr() in case cpus_allowed has changed.

Doesn't help, you should simply not _ever_ touch this for random tasks.

[PATCH 3/6] dcdbas: make use of call_sync_on_phys_cpu()

[Juergen Gross]

Use call_sync_on_phys_cpu() to raise SMI on cpu 0.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/firmware/dcdbas.c | 46 ++++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 829eec8..659c616 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -238,33 +238,14 @@ static ssize_t host_control_on_shutdown_store(struct device *dev,
 	return count;
 }
 
-/**
- * dcdbas_smi_request: generate SMI request
- *
- * Called with smi_data_lock.
- */
-int dcdbas_smi_request(struct smi_cmd *smi_cmd)
+static int raise_smi(void *par)
 {
-	cpumask_var_t old_mask;
-	int ret = 0;
+	struct smi_cmd *smi_cmd = par;
 
-	if (smi_cmd->magic != SMI_CMD_MAGIC) {
-		dev_info(&dcdbas_pdev->dev, "%s: invalid magic value\n",
-			 __func__);
-		return -EBADR;
-	}
-
-	/* SMI requires CPU 0 */
-	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
-		return -ENOMEM;
-
-	cpumask_copy(old_mask, &current->cpus_allowed);
-	set_cpus_allowed_ptr(current, cpumask_of(0));
 	if (smp_processor_id() != 0) {
 		dev_dbg(&dcdbas_pdev->dev, "%s: failed to get CPU 0\n",
 			__func__);
-		ret = -EBUSY;
-		goto out;
+		return -EBUSY;
 	}
 
 	/* generate SMI */
@@ -280,10 +261,23 @@ int dcdbas_smi_request(struct smi_cmd *smi_cmd)
 		: "memory"
 	);
 
-out:
-	set_cpus_allowed_ptr(current, old_mask);
-	free_cpumask_var(old_mask);
-	return ret;
+	return 0;
+}
+/**
+ * dcdbas_smi_request: generate SMI request
+ *
+ * Called with smi_data_lock.
+ */
+int dcdbas_smi_request(struct smi_cmd *smi_cmd)
+{
+	if (smi_cmd->magic != SMI_CMD_MAGIC) {
+		dev_info(&dcdbas_pdev->dev, "%s: invalid magic value\n",
+			 __func__);
+		return -EBADR;
+	}
+
+	/* SMI requires CPU 0 */
+	return call_sync_on_phys_cpu(0, raise_smi, smi_cmd);
 }
 
 /**
-- 
2.6.2

[PATCH 4/6] hwmon: use call_sync_on_phys_cpu() for dell-smm i8k

[Juergen Gross]

Use the call_sync_on_phys_cpu() function to call system management mode
on cpu 0.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index c43318d..7772c27 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -130,23 +130,15 @@ static inline const char *i8k_get_dmi_data(int field)
 /*
  * Call the System Management Mode BIOS. Code provided by Jonathan Buzzard.
  */
-static int i8k_smm(struct smm_regs *regs)
+static int i8k_smm_func(void *par)
 {
 	int rc;
+	struct smm_regs *regs = par;
 	int eax = regs->eax;
-	cpumask_var_t old_mask;
 
 	/* SMM requires CPU 0 */
-	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
-		return -ENOMEM;
-	cpumask_copy(old_mask, &current->cpus_allowed);
-	rc = set_cpus_allowed_ptr(current, cpumask_of(0));
-	if (rc)
-		goto out;
-	if (smp_processor_id() != 0) {
-		rc = -EBUSY;
-		goto out;
-	}
+	if (smp_processor_id() != 0)
+		return -EBUSY;
 
 #if defined(CONFIG_X86_64)
 	asm volatile("pushq %%rax\n\t"
@@ -204,13 +196,18 @@ static int i8k_smm(struct smm_regs *regs)
 	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
 		rc = -EINVAL;
 
-out:
-	set_cpus_allowed_ptr(current, old_mask);
-	free_cpumask_var(old_mask);
 	return rc;
 }
 
 /*
+ * Call the System Management Mode BIOS.
+ */
+static int i8k_smm(struct smm_regs *regs)
+{
+	return call_sync_on_phys_cpu(0, i8k_smm_func, regs);
+}
+
+/*
  * Read the fan status.
  */
 static int i8k_get_fan_status(int fan)
-- 
2.6.2

[PATCH 5/6] virt, sched: add cpu pinning to call_sync_on_phys_cpu()

[Juergen Gross]

Add generic virtualization support for pinning the current vcpu to a
specified physical cpu. As this operation isn't performance critical
(a very limited set of operations like BIOS calls and SMIs is expected
to need this) just add a hypervisor specific indirection.

Such a pinning should last as short as possible as it might block
sensible vcpu scheduling and maybe other hypervisor functions like
suspending the system which rely on scheduling. To ensure this don't
let the current thread be preempted while the vcpu is pinned in
call_sync_on_phys_cpu().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/hypervisor.h |  9 +++++++++
 include/linux/sched.h             | 24 +++++++++++++++++++++++-
 kernel/sched/core.c               |  4 ++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 055ea99..13f80a2 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -43,6 +43,9 @@ struct hypervisor_x86 {
 
 	/* X2APIC detection (run once per boot) */
 	bool		(*x2apic_available)(void);
+
+	/* pin current vcpu to specified physical cpu (run rarely) */
+	void		(*pin_vcpu)(int);
 };
 
 extern const struct hypervisor_x86 *x86_hyper;
@@ -56,6 +59,12 @@ extern const struct hypervisor_x86 x86_hyper_kvm;
 extern void init_hypervisor(struct cpuinfo_x86 *c);
 extern void init_hypervisor_platform(void);
 extern bool hypervisor_x2apic_available(void);
+
+static inline void hypervisor_pin_vcpu(int cpu)
+{
+	if (x86_hyper->pin_vcpu)
+		x86_hyper->pin_vcpu(cpu);
+}
 #else
 static inline void init_hypervisor(struct cpuinfo_x86 *c) { }
 static inline void init_hypervisor_platform(void) { }
diff --git a/include/linux/sched.h b/include/linux/sched.h
index dfadf1a..53b33d5 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -61,6 +61,9 @@ struct sched_param {
 #include <linux/cgroup-defs.h>
 
 #include <asm/processor.h>
+#ifdef CONFIG_HYPERVISOR_GUEST
+#include <asm/hypervisor.h>
+#endif
 
 #define SCHED_ATTR_SIZE_VER0	48	/* sizeof first published struct */
 
@@ -2230,6 +2233,17 @@ static inline void rcu_copy_process(struct task_struct *p)
 #endif /* #ifdef CONFIG_TASKS_RCU */
 }
 
+#ifdef CONFIG_HYPERVISOR_GUEST
+static inline void pin_vcpu(int cpu)
+{
+	hypervisor_pin_vcpu(cpu);
+}
+#else
+static inline void pin_vcpu(int cpu)
+{
+}
+#endif
+
 static inline void tsk_restore_flags(struct task_struct *task,
 				unsigned long orig_flags, unsigned long flags)
 {
@@ -2263,10 +2277,18 @@ static inline int set_cpus_allowed_ptr(struct task_struct *p,
 static inline int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *),
 					void *par)
 {
+	int ret;
+
 	if (cpu != 0)
 		return -EINVAL;
 
-	return func(par);
+	preempt_disable();
+	pin_vcpu(0);
+	ret = func(par);
+	pin_vcpu(-1);
+	preempt_enable();
+
+	return ret;
 }
 #endif
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cb9955f..2dc27ca 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1281,7 +1281,11 @@ int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
 	if (ret)
 		goto out;
 
+	preempt_disable();
+	pin_vcpu(cpu);
 	ret = func(par);
+	pin_vcpu(-1);
+	preempt_enable();
 
 	set_cpus_allowed_ptr(current, old_mask);
 
-- 
2.6.2

[PATCH 6/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu

[Juergen Gross]

Some hardware models (e.g. Dell Studio 1555 laptops) require calls to
the firmware to be issued on cpu 0 only. As Dom0 might have to use
these calls, add xen_pin_vcpu() to achieve this functionality.

In case either the domain doesn't have the privilege to make the
related hypercall or the hypervisor isn't supporting it, issue a
warning once and disable further pinning attempts.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index d09e4c9..325e108 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1884,6 +1884,45 @@ static void xen_set_cpu_features(struct cpuinfo_x86 *c)
 	}
 }
 
+static void xen_pin_vcpu(int cpu)
+{
+	static bool disable_pinning;
+	struct sched_pin_override pin_override;
+	int ret;
+
+	if (disable_pinning)
+		return;
+
+	pin_override.pcpu = cpu;
+	ret = HYPERVISOR_sched_op(SCHEDOP_pin_override, &pin_override);
+	if (cpu < 0)
+		return;
+
+	switch (ret) {
+	case -ENOSYS:
+		pr_warn("The kernel tried to call a function on physical cpu %d, but Xen isn't\n"
+			"supporting this. In case of problems you might consider vcpu pinning.\n",
+			cpu);
+		disable_pinning = true;
+		break;
+	case -EPERM:
+		WARN(1, "Trying to pin vcpu without having privilege to do so\n");
+		disable_pinning = true;
+		break;
+	case -EINVAL:
+	case -EBUSY:
+		pr_warn("The kernel tried to call a function on physical cpu %d, but this cpu\n"
+			"seems not to be available. Please check your Xen cpu configuration.\n",
+			cpu);
+		break;
+	case 0:
+		break;
+	default:
+		WARN(1, "rc %d while trying to pin vcpu\n", ret);
+		disable_pinning = true;
+	}
+}
+
 const struct hypervisor_x86 x86_hyper_xen = {
 	.name			= "Xen",
 	.detect			= xen_platform,
@@ -1892,6 +1931,7 @@ const struct hypervisor_x86 x86_hyper_xen = {
 #endif
 	.x2apic_available	= xen_x2apic_para_available,
 	.set_cpu_features       = xen_set_cpu_features,
+	.pin_vcpu               = xen_pin_vcpu,
 };
 EXPORT_SYMBOL(x86_hyper_xen);
 
-- 
2.6.2

Re: [PATCH 0/6] Support calling functions on dedicated physical cpu

[Peter Zijlstra]

On Fri, Mar 11, 2016 at 12:59:28PM +0100, Juergen Gross wrote:
> Some hardware (e.g. Dell Studio laptops) require special functions to
> be called on physical cpu 0 in order to avoid occasional hangs. When
> running as dom0 under Xen this could be achieved only via special boot
> parameters (vcpu pinning) limiting the hypervisor in it's scheduling
> decisions.

So instead of telling Dell to get their act together and fix their damn
firmware, we're going to add the most horrid gunk to the kernel? How
does that make sense?

Re: [PATCH 0/6] Support calling functions on dedicated physical cpu

[Pali Rohár]

On Friday 11 March 2016 13:25:14 Peter Zijlstra wrote:
> On Fri, Mar 11, 2016 at 12:59:28PM +0100, Juergen Gross wrote:
> > Some hardware (e.g. Dell Studio laptops) require special functions to
> > be called on physical cpu 0 in order to avoid occasional hangs. When
> > running as dom0 under Xen this could be achieved only via special boot
> > parameters (vcpu pinning) limiting the hypervisor in it's scheduling
> > decisions.
> 
> So instead of telling Dell to get their act together and fix their damn
> firmware, we're going to add the most horrid gunk to the kernel? How
> does that make sense?

Hi Peter! That problem is for old laptops and I doubt that Dell would
fix and distribute patches for 5-15 years old laptops...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/6] Support calling functions on dedicated physical cpu

[One Thousand Gnomes]

On Fri, 11 Mar 2016 13:25:14 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Mar 11, 2016 at 12:59:28PM +0100, Juergen Gross wrote:
> > Some hardware (e.g. Dell Studio laptops) require special functions to
> > be called on physical cpu 0 in order to avoid occasional hangs. When
> > running as dom0 under Xen this could be achieved only via special boot
> > parameters (vcpu pinning) limiting the hypervisor in it's scheduling
> > decisions.  
> 
> So instead of telling Dell to get their act together and fix their damn
> firmware, we're going to add the most horrid gunk to the kernel? How
> does that make sense?

It's been normal forever. The convention with a lot of older BIOS crap
was always that it should be called on the boot CPU (APM. PnPBIOS etc).

It's a stretch pre-EFI to even call it a "bug"

Alan

Re: [PATCH 0/6] Support calling functions on dedicated physical cpu

[Peter Zijlstra]

On Fri, Mar 11, 2016 at 01:15:04PM +0000, One Thousand Gnomes wrote:
> On Fri, 11 Mar 2016 13:25:14 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, Mar 11, 2016 at 12:59:28PM +0100, Juergen Gross wrote:
> > > Some hardware (e.g. Dell Studio laptops) require special functions to
> > > be called on physical cpu 0 in order to avoid occasional hangs. When
> > > running as dom0 under Xen this could be achieved only via special boot
> > > parameters (vcpu pinning) limiting the hypervisor in it's scheduling
> > > decisions.  
> > 
> > So instead of telling Dell to get their act together and fix their damn
> > firmware, we're going to add the most horrid gunk to the kernel? How
> > does that make sense?
> 
> It's been normal forever. The convention with a lot of older BIOS crap
> was always that it should be called on the boot CPU (APM. PnPBIOS etc).
> 
> It's a stretch pre-EFI to even call it a "bug"

Yeah, I knew about the APM/PnP muck, but I was under the impression this
was about new hardware.