Re: [RFC PATCH v2 06/10] riscv: Add functions to register ghes having SSE notification

Clément Léger
2025-11-05T10:33:18Z

On 10/29/25 12:26, Himanshu Chauhan wrote:
> Add functions to register the ghes entries which have SSE as
> notification type. The vector inside the ghes is the SSE event
> ID that should be registered.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
> drivers/firmware/riscv/riscv_sbi_sse.c | 147 +++++++++++++++++++++++++
> include/linux/riscv_sbi_sse.h | 16 +++
> 2 files changed, 163 insertions(+)
>
> diff --git a/drivers/firmware/riscv/riscv_sbi_sse.c b/drivers/firmware/riscv/riscv_sbi_sse.c
> index 6561c7acdaaa..46ebc9e9651c 100644
> --- a/drivers/firmware/riscv/riscv_sbi_sse.c
> +++ b/drivers/firmware/riscv/riscv_sbi_sse.c
> @@ -5,6 +5,8 @@
>
> #define pr_fmt(fmt) "sse: " fmt
>
> +#include <acpi/ghes.h>
> +#include <linux/acpi.h>
> #include <linux/cpu.h>
> #include <linux/cpuhotplug.h>
> #include <linux/cpu_pm.h>
> @@ -700,3 +702,148 @@ static int __init sse_init(void)
> return ret;
> }
> arch_initcall(sse_init);
> +
> +struct sse_ghes_callback {
> + struct list_head head;
> + struct ghes *ghes;
> + sse_event_handler_fn *callback;
> +};
> +
> +struct sse_ghes_event_data {
> + struct list_head head;
> + u32 event_num;
> + struct list_head callback_list;
> + struct sse_event *event;
> +};
> +
> +static DEFINE_SPINLOCK(sse_ghes_event_list_lock);
> +static LIST_HEAD(sse_ghes_event_list);

Hi Himanshu,

Please declare these structs/functions at the beggining of the file.

> +
> +static int sse_ghes_handler(u32 event_num, void *arg, struct pt_regs *regs)
> +{
> + struct sse_ghes_event_data *ev_data = arg;
> + struct sse_ghes_callback *cb = NULL;
> +
> + list_for_each_entry(cb, &ev_data->callback_list, head) {
> + if (cb && cb->ghes && cb->callback) {
> + cb->callback(ev_data->event_num, cb->ghes, regs);
> + }
> + }
> +
> + return 0;
> +}
> +
> +int sse_register_ghes(struct ghes *ghes, sse_event_handler_fn *lo_cb,
> + sse_event_handler_fn *hi_cb)
> +{
> + struct sse_ghes_event_data *ev_data, *evd;
> + struct sse_ghes_callback *cb;
> + u32 ev_num;
> + int err;
> +
> + if (!sse_available)
> + return -EOPNOTSUPP;
> + if (!ghes || !lo_cb || !hi_cb)
> + return -EINVAL;
> +
> + ev_num = ghes->generic->notify.vector;
> +
> + ev_data = NULL;
> + spin_lock(&sse_ghes_event_list_lock);
> + list_for_each_entry(evd, &sse_ghes_event_list, head) {
> + if (evd->event_num == ev_num) {
> + ev_data = evd;
> + break;
> + }
> + }
> + spin_unlock(&sse_ghes_event_list_lock);

That lock should cover the whole ev_data creation. Because if two CPUs
enters this function at the same time, the following scneario can occur:

CPU0 CPU1
lock
ev_data = NULL
unlock
lock
ev_data = NULL
unlock

create ev_data create ev_data

-> Both will have read a ev_data = NULL and create an ev_data.

The lock should be kept and unlocked at the end of the function, you can
use a guard() for that.

> +
> + if (!ev_data) {
> + ev_data = kzalloc(sizeof(*ev_data), GFP_KERNEL);
> + if (!ev_data)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&ev_data->head);

I think this isn't necessary since list_add_tail() will anyway overwrite
the head->next/prev field. BTW it's confusing to call this member head
since it will be used as a node in the list. It could probably be
renamed node/list.

> + ev_data->event_num = ev_num;
> +
> + INIT_LIST_HEAD(&ev_data->callback_list);
> +
> + ev_data->event = sse_event_register(ev_num, ev_num,
> + sse_ghes_handler, ev_data);
> + if (IS_ERR(ev_data->event)) {
> + pr_err("%s: Couldn't register event 0x%x\n", __func__, ev_num);
> + kfree(ev_data);
> + return -ENOMEM;
> + }
> +
> + err = sse_event_enable(ev_data->event);
> + if (err) {
> + pr_err("%s: Couldn't enable event 0x%x\n", __func__, ev_num);
> + sse_event_unregister(ev_data->event);
> + kfree(ev_data);
> + return err;
> + }
> +
> + spin_lock(&sse_ghes_event_list_lock);
> + list_add_tail(&ev_data->head, &sse_ghes_event_list);
> + spin_unlock(&sse_ghes_event_list_lock);
> + }
> +
> + list_for_each_entry(cb, &ev_data->callback_list, head) {
> + if (cb->ghes == ghes)
> + return -EALREADY;
> + }
> +
> + cb = kzalloc(sizeof(*cb), GFP_KERNEL);
> + if (!cb)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&cb->head);
> + cb->ghes = ghes;
> + cb->callback = lo_cb;
> + list_add_tail(&cb->head, &ev_data->callback_list);

AFAIU, at this point, the SSE event is already enabled, it means the
sse_ghes_handler() can be called. This one can potentially access
&ev_data->callback_list concurrently which would result in a corrupted
list. You should mask/disable the SSE event while adding the callback to
this list.

BTW, accessing the ev_data->callback here means that if multiple CPUs
access this function at the same time, it could result in a corrupted
ev_data list. Not sure if it can happen but better be safe than sorry.

> +
> + return 0;
> +}
> +
> +int sse_unregister_ghes(struct ghes *ghes)
> +{
> + struct sse_ghes_event_data *ev_data, *tmp;
> + struct sse_ghes_callback *cb;
> + int free_ev_data = 0;
> +
> + if (!ghes)
> + return -EINVAL;
> +
> + spin_lock(&sse_ghes_event_list_lock);
> +
> + list_for_each_entry_safe(ev_data, tmp, &sse_ghes_event_list, head) {
> + list_for_each_entry(cb, &ev_data->callback_list, head) {
> + if (cb->ghes != ghes)
> + continue;
> +
> + list_del(&cb->head);
> + kfree(cb);
> + break;
> + }
> +
> + if (list_empty(&ev_data->callback_list))
> + free_ev_data = 1;
> +
> + if (free_ev_data) {

Remove free_ev_data and use the following:

if (list_empty(&ev_data->callback_list)) {> +
spin_unlock(&sse_ghes_event_list_lock);
> +
> + sse_event_disable(ev_data->event);
> + sse_event_unregister(ev_data->event);
> + ev_data->event = NULL;
> +
> + spin_lock(&sse_ghes_event_list_lock);
> +
> + list_del(&ev_data->head);
> + kfree(ev_data);
> + }
> + }
> +
> + spin_unlock(&sse_ghes_event_list_lock);
> +
> + return 0;
> +}

Please declare this above the arch_initcall() function

> diff --git a/include/linux/riscv_sbi_sse.h b/include/linux/riscv_sbi_sse.h
> index a1b58e89dd19..cd615b479f82 100644
> --- a/include/linux/riscv_sbi_sse.h
> +++ b/include/linux/riscv_sbi_sse.h
> @@ -11,6 +11,7 @@
>
> struct sse_event;
> struct pt_regs;
> +struct ghes;
>
> typedef int (sse_event_handler_fn)(u32 event_num, void *arg,
> struct pt_regs *regs);
> @@ -24,6 +25,10 @@ void sse_event_unregister(struct sse_event *evt);
>
> int sse_event_set_target_cpu(struct sse_event *sse_evt, unsigned int cpu);
>
> +int sse_register_ghes(struct ghes *ghes, sse_event_handler_fn *lo_cb,
> + sse_event_handler_fn *hi_cb);
> +int sse_unregister_ghes(struct ghes *ghes);
> +
> int sse_event_enable(struct sse_event *sse_evt);
>
> void sse_event_disable(struct sse_event *sse_evt);
> @@ -47,6 +52,17 @@ static inline int sse_event_set_target_cpu(struct sse_event *sse_evt,
> return -EOPNOTSUPP;
> }
>
> +static inline int sse_register_ghes(struct ghes *ghes, sse_event_handler_fn *lo_cb,
> + sse_event_handler_fn *hi_cb)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static inline int sse_unregister_ghes(struct ghes *ghes)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> static inline int sse_event_enable(struct sse_event *sse_evt)
> {
> return -EOPNOTSUPP;