Re: [PATCH v4 11/13] firmware: arm_sdei: add support for CPU private events
Hi Will,
On 24/10/17 18:34, James Morse wrote:
> On 18/10/17 18:19, Will Deacon wrote:
>> On Tue, Oct 17, 2017 at 06:44:30PM +0100, James Morse wrote:
>>> Private SDE events are per-cpu, and need to be registered and enabled
>>> on each CPU.
>>>
>>> Hide this detail from the caller by adapting our {,un}register and
>>> {en,dis}able calls to send an IPI to each CPU if the event is private.
>>>
>>> CPU private events are unregistered when the CPU is powered-off, and
>>> re-registered when the CPU is brought back online. This saves bringing
>>> secondary cores back online to call private_reset() on shutdown, kexec
>>> and resume from hibernate.
>
>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>>> index 28e4c4cbb16d..5598d9ba8b5d 100644
>>> --- a/drivers/firmware/arm_sdei.c
>>> +++ b/drivers/firmware/arm_sdei.c
>>> @@ -610,6 +813,7 @@ static int sdei_device_freeze(struct device *dev)
>>> {
>>> int err;
>>>
>>> + frozen = true;
>>> err = sdei_event_unregister_all();
>
>> Are the release semantics from spin_unlock in sdei_event_unregister_all
>> sufficient for the ordering guarantees you need?
>
> ... ordering ...
>
> The hotplug notifiers don't touch that lock, so at the first level: no.
>
> It looks like I was relying on the cpu-hotplug code using stop-machine for its
> work, and the spinlocks changing the pre-empt count for this to work. Which is
> not something I want to debug if it changes!
>
> I'll post a patch changing this bool to a more sensible atomic type.
Here I show my ignorance: atomic_t didn't do what I thought. But I do take the
spinlock inside the hotplug callback, so I'll move the variable in there.
Thanks,
James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 11/13] firmware: arm_sdei: add support for CPU private events
Hi Will,
On 18/10/17 18:19, Will Deacon wrote:
> On Tue, Oct 17, 2017 at 06:44:30PM +0100, James Morse wrote:
>> Private SDE events are per-cpu, and need to be registered and enabled
>> on each CPU.
>>
>> Hide this detail from the caller by adapting our {,un}register and
>> {en,dis}able calls to send an IPI to each CPU if the event is private.
>>
>> CPU private events are unregistered when the CPU is powered-off, and
>> re-registered when the CPU is brought back online. This saves bringing
>> secondary cores back online to call private_reset() on shutdown, kexec
>> and resume from hibernate.
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 28e4c4cbb16d..5598d9ba8b5d 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -610,6 +813,7 @@ static int sdei_device_freeze(struct device *dev)
>> {
>> int err;
>>
>> +frozen = true;
>> err = sdei_event_unregister_all();
> Are the release semantics from spin_unlock in sdei_event_unregister_all
> sufficient for the ordering guarantees you need?
... ordering ...
The hotplug notifiers don't touch that lock, so at the first level: no.
It looks like I was relying on the cpu-hotplug code using stop-machine for its
work, and the spinlocks changing the pre-empt count for this to work. Which is
not something I want to debug if it changes!
I'll post a patch changing this bool to a more sensible atomic type.
Thanks,
James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 11/13] firmware: arm_sdei: add support for CPU private events
On Tue, Oct 17, 2017 at 06:44:30PM +0100, James Morse wrote:
> Private SDE events are per-cpu, and need to be registered and enabled
> on each CPU.
>
> Hide this detail from the caller by adapting our {,un}register and
> {en,dis}able calls to send an IPI to each CPU if the event is private.
>
> CPU private events are unregistered when the CPU is powered-off, and
> re-registered when the CPU is brought back online. This saves bringing
> secondary cores back online to call private_reset() on shutdown, kexec
> and resume from hibernate.
>
> Signed-off-by: James Morse
> ---
> drivers/firmware/arm_sdei.c | 242
> +---
> 1 file changed, 228 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 28e4c4cbb16d..5598d9ba8b5d 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -21,9 +21,11 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -64,12 +66,49 @@ struct sdei_event {
> u8 priority;
>
> /* This pointer is handed to firmware as the event argument. */
> - struct sdei_registered_event *registered;
> + union {
> + /* Shared events */
> + struct sdei_registered_event *registered;
> +
> + /* CPU private events */
> + struct sdei_registered_event __percpu *private_registered;
> + };
> };
>
> static LIST_HEAD(sdei_events);
> static DEFINE_SPINLOCK(sdei_events_lock);
>
> +/* When frozen, cpu-hotplug notifiers shouldn't unregister/re-register
> events */
> +static bool frozen;
It looks like this can be accessed concurrently, so you need at least
READ_ONCE/WRITE_ONCE.
> /* When entering idle, mask/unmask events for this cpu */
> static int sdei_pm_notifier(struct notifier_block *nb, unsigned long action,
> void *data)
> @@ -610,6 +813,7 @@ static int sdei_device_freeze(struct device *dev)
> {
> int err;
>
> + frozen = true;
> err = sdei_event_unregister_all();
Are the release semantics from spin_unlock in sdei_event_unregister_all
sufficient for the ordering guarantees you need?
Will
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 11/13] firmware: arm_sdei: add support for CPU private events
On Tue, Oct 17, 2017 at 06:44:30PM +0100, James Morse wrote:
> Private SDE events are per-cpu, and need to be registered and enabled
> on each CPU.
>
> Hide this detail from the caller by adapting our {,un}register and
> {en,dis}able calls to send an IPI to each CPU if the event is private.
>
> CPU private events are unregistered when the CPU is powered-off, and
> re-registered when the CPU is brought back online. This saves bringing
> secondary cores back online to call private_reset() on shutdown, kexec
> and resume from hibernate.
>
> Signed-off-by: James Morse
Acked-by: Catalin Marinas
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v4 11/13] firmware: arm_sdei: add support for CPU private events
Private SDE events are per-cpu, and need to be registered and enabled
on each CPU.
Hide this detail from the caller by adapting our {,un}register and
{en,dis}able calls to send an IPI to each CPU if the event is private.
CPU private events are unregistered when the CPU is powered-off, and
re-registered when the CPU is brought back online. This saves bringing
secondary cores back online to call private_reset() on shutdown, kexec
and resume from hibernate.
Signed-off-by: James Morse
---
drivers/firmware/arm_sdei.c | 242 +---
1 file changed, 228 insertions(+), 14 deletions(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 28e4c4cbb16d..5598d9ba8b5d 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -21,9 +21,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -64,12 +66,49 @@ struct sdei_event {
u8 priority;
/* This pointer is handed to firmware as the event argument. */
- struct sdei_registered_event *registered;
+ union {
+ /* Shared events */
+ struct sdei_registered_event *registered;
+
+ /* CPU private events */
+ struct sdei_registered_event __percpu *private_registered;
+ };
};
static LIST_HEAD(sdei_events);
static DEFINE_SPINLOCK(sdei_events_lock);
+/* When frozen, cpu-hotplug notifiers shouldn't unregister/re-register events
*/
+static bool frozen;
+
+/* Private events are registered/enabled via IPI passing one of these */
+struct sdei_crosscall_args {
+ struct sdei_event *event;
+ atomic_t errors;
+ int first_error;
+};
+
+#define CROSSCALL_INIT(arg, event) (arg.event = event, \
+arg.first_error = 0, \
+atomic_set(&arg.errors, 0))
+
+static inline int sdei_do_cross_call(void *fn, struct sdei_event *event)
+{
+ struct sdei_crosscall_args arg;
+
+ CROSSCALL_INIT(arg, event);
+ on_each_cpu(fn, &arg, true);
+
+ return arg.first_error;
+}
+
+static inline void
+sdei_cross_call_return(struct sdei_crosscall_args *arg, int err)
+{
+ if (err && (atomic_inc_return(&arg->errors) == 1))
+ arg->first_error = err;
+}
+
static int sdei_to_linux_errno(unsigned long sdei_err)
{
switch (sdei_err) {
@@ -213,6 +252,26 @@ static struct sdei_event *sdei_event_create(u32 event_num,
reg->callback = cb;
reg->callback_arg = cb_arg;
event->registered = reg;
+ } else {
+ int cpu;
+ struct sdei_registered_event __percpu *regs;
+
+ regs = alloc_percpu(struct sdei_registered_event);
+ if (!regs) {
+ kfree(event);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ for_each_possible_cpu(cpu) {
+ reg = per_cpu_ptr(regs, cpu);
+
+ reg->event_num = event->event_num;
+ reg->priority = event->priority;
+ reg->callback = cb;
+ reg->callback_arg = cb_arg;
+ }
+
+ event->private_registered = regs;
}
if (sdei_event_find(event_num)) {
@@ -234,6 +293,8 @@ static void sdei_event_destroy(struct sdei_event *event)
if (event->type == SDEI_EVENT_TYPE_SHARED)
kfree(event->registered);
+ else
+ free_percpu(event->private_registered);
kfree(event);
}
@@ -264,11 +325,6 @@ static void _ipi_mask_cpu(void *ignored)
sdei_mask_local_cpu();
}
-static int sdei_cpuhp_down(unsigned int ignored)
-{
- return sdei_mask_local_cpu();
-}
-
int sdei_unmask_local_cpu(void)
{
int err;
@@ -290,11 +346,6 @@ static void _ipi_unmask_cpu(void *ignored)
sdei_unmask_local_cpu();
}
-static int sdei_cpuhp_up(unsigned int ignored)
-{
- return sdei_unmask_local_cpu();
-}
-
static void _ipi_private_reset(void *ignored)
{
int err;
@@ -345,6 +396,16 @@ static int sdei_api_event_enable(u32 event_num)
0, NULL);
}
+static void _ipi_event_enable(void *data)
+{
+ int err;
+ struct sdei_crosscall_args *arg = data;
+
+ err = sdei_api_event_enable(arg->event->event_num);
+
+ sdei_cross_call_return(arg, err);
+}
+
int sdei_event_enable(u32 event_num)
{
int err = -EINVAL;
@@ -356,6 +417,8 @@ int sdei_event_enable(u32 event_num)
err = -ENOENT;
else if (event->type == SDEI_EVENT_TYPE_SHARED)
err = sdei_api_event_enable(event->event_num);
+ else
+ err = sdei_do_cross_call(_ipi_event_enable, event);
spin_unlock(&sdei_events_lock);
return err;
@@ -368,6 +431,16 @@ static int sdei_
