irqchip/riscv-imsic: Fix irq migration failure issue when cpu hotplug.

[PATCH] irqchip/riscv-imsic: Fix irq migration failure issue when cpu hotplug.

Posted by Yingjun Ni 4 days, 7 hours ago

Add a null pointer check for irq_write_msi_msg to fix NULL pointer
dereference issue when migrating irq.

Modify the return value of imsic_irq_set_affinity to let the subdomain
PCI-MSIX migrate the irq to a new cpu when cpu hotplug.

Don't set vec->move_next in imsic_vector_move_update when the cpu is
offline, because it will never be cleared.

crash log:
Unable to handle kernel NULL pointer dereference at virtual address 0
...
Stopper: multi_cpu_stop+0x0/0x24c <- stop_machine_cpuslocked+0x160/0x1b4
epc : 0x0
 ra : imsic_irq_set_affinity+0x174/0x1ac

Reproduce step:
1. start riscv kernel by qemu.

$QEMU -M virt,aia=aplic-imsic,aia-guests=5 -m 512M -smp 4 \
      -kernel $KERNEL -initrd $ROOTFS  -bios $OPENSBI \
      -device e1000e,netdev=net0,mac=52:54:00:12:34:56 \
      -netdev user,id=net0,net=10.0.2.0/24,dhcpstart=10.0.2.11 \
      -nographic

2. set cpu3 offline.

cat /proc/interrupts
    CPU0  CPU1  CPU2 CPU3
23:   0    0     0    66   PCI-MSIX-0000:00:01.0 eth0-rx-0

echo 0 > /sys/bus/cpu/devices/cpu3/online

Signed-off-by: Yingjun Ni <yingjun.ni@riscv-computing.com>
---
 drivers/irqchip/irq-riscv-imsic-platform.c | 8 ++++++--
 drivers/irqchip/irq-riscv-imsic-state.c    | 5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
index 643c8e459611..131e4f2b5431 100644
--- a/drivers/irqchip/irq-riscv-imsic-platform.c
+++ b/drivers/irqchip/irq-riscv-imsic-platform.c
@@ -93,9 +93,13 @@ static void imsic_irq_compose_msg(struct irq_data *d, struct msi_msg *msg)
 static void imsic_msi_update_msg(struct irq_data *d, struct imsic_vector *vec)
 {
 	struct msi_msg msg = { };
+	struct irq_chip *irq_chip = irq_data_get_irq_chip(d);
+
+	if (!irq_chip->irq_write_msi_msg)
+		return;
 
 	imsic_irq_compose_vector_msg(vec, &msg);
-	irq_data_get_irq_chip(d)->irq_write_msi_msg(d, &msg);
+	irq_chip->irq_write_msi_msg(d, &msg);
 }
 
 static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
@@ -173,7 +177,7 @@ static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask
 	/* Move state of the old vector to the new vector */
 	imsic_vector_move(old_vec, new_vec);
 
-	return IRQ_SET_MASK_OK_DONE;
+	return IRQ_SET_MASK_OK;
 }
 
 static void imsic_irq_force_complete_move(struct irq_data *d)
diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
index b6cebfee9461..cd1bf9516878 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.c
+++ b/drivers/irqchip/irq-riscv-imsic-state.c
@@ -362,6 +362,10 @@ static bool imsic_vector_move_update(struct imsic_local_priv *lpriv,
 	/* Update enable and move details */
 	enabled = READ_ONCE(vec->enable);
 	WRITE_ONCE(vec->enable, new_enable);
+
+	if (!cpu_online(vec->cpu) && is_old_vec)
+		goto out;
+
 	if (is_old_vec)
 		WRITE_ONCE(vec->move_next, move_vec);
 	else
@@ -371,6 +375,7 @@ static bool imsic_vector_move_update(struct imsic_local_priv *lpriv,
 	bitmap_set(lpriv->dirty_bitmap, vec->local_id, 1);
 	__imsic_remote_sync(lpriv, vec->cpu);
 
+out:
 	raw_spin_unlock_irqrestore(&lpriv->lock, flags);
 
 	return enabled;
-- 
2.43.0

Re: [PATCH] irqchip/riscv-imsic: Fix irq migration failure issue when cpu hotplug.

Posted by Thomas Gleixner 4 days, 1 hour ago

On Tue, Feb 03 2026 at 16:02, Yingjun Ni wrote:
> Add a null pointer check for irq_write_msi_msg to fix NULL pointer
> dereference issue when migrating irq.
>
> Modify the return value of imsic_irq_set_affinity to let the subdomain
> PCI-MSIX migrate the irq to a new cpu when cpu hotplug.
>
> Don't set vec->move_next in imsic_vector_move_update when the cpu is
> offline, because it will never be cleared.

You completely fail to explain the actual problem and the root
cause. See

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

>  drivers/irqchip/irq-riscv-imsic-platform.c | 8 ++++++--
>  drivers/irqchip/irq-riscv-imsic-state.c    | 5 +++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
> index 643c8e459611..131e4f2b5431 100644
> --- a/drivers/irqchip/irq-riscv-imsic-platform.c
> +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
> @@ -93,9 +93,13 @@ static void imsic_irq_compose_msg(struct irq_data *d, struct msi_msg *msg)
>  static void imsic_msi_update_msg(struct irq_data *d, struct imsic_vector *vec)
>  {
>  	struct msi_msg msg = { };
> +	struct irq_chip *irq_chip = irq_data_get_irq_chip(d);
> +
> +	if (!irq_chip->irq_write_msi_msg)
> +		return;

I have no idea how this ever worked. The irq_data pointer belongs to the
IMSIC base domain, which definitely does not have a irq_write_msi_msg()
callback and never can have one.

The write message callback is always implemented by the top most domain,
in this case the PCI/MSI[x] per device domain.

So this code is simply broken and your NULL pointer check just makes it
differently broken.

>  	imsic_irq_compose_vector_msg(vec, &msg);
> -	irq_data_get_irq_chip(d)->irq_write_msi_msg(d, &msg);
> +	irq_chip->irq_write_msi_msg(d, &msg);
>  }
>  
>  static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> @@ -173,7 +177,7 @@ static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask
>  	/* Move state of the old vector to the new vector */
>  	imsic_vector_move(old_vec, new_vec);
>  
> -	return IRQ_SET_MASK_OK_DONE;
> +	return IRQ_SET_MASK_OK;

Have you actually looked at the consequences of this change?

>  }
>  
>  static void imsic_irq_force_complete_move(struct irq_data *d)
> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index b6cebfee9461..cd1bf9516878 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> @@ -362,6 +362,10 @@ static bool imsic_vector_move_update(struct imsic_local_priv *lpriv,
>  	/* Update enable and move details */
>  	enabled = READ_ONCE(vec->enable);
>  	WRITE_ONCE(vec->enable, new_enable);
> +
> +	if (!cpu_online(vec->cpu) && is_old_vec)
> +		goto out;

This is definitely not correct as this should still cleanup software
state, no?

Thanks,

        tglx

Re: [PATCH] irqchip/riscv-imsic: Fix irq migration failure issue when cpu hotplug.

Posted by Yingjun Ni 3 days, 13 hours ago

> On Tue, Feb 03 2026 at 16:02, Yingjun Ni wrote:
>> Add a null pointer check for irq_write_msi_msg to fix NULL pointer
>> dereference issue when migrating irq.
>>
>> Modify the return value of imsic_irq_set_affinity to let the subdomain
>> PCI-MSIX migrate the irq to a new cpu when cpu hotplug.
>>
>> Don't set vec->move_next in imsic_vector_move_update when the cpu is
>> offline, because it will never be cleared.
> You completely fail to explain the actual problem and the root
> cause. See
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
>>   drivers/irqchip/irq-riscv-imsic-platform.c | 8 ++++++--
>>   drivers/irqchip/irq-riscv-imsic-state.c    | 5 +++++
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
>> index 643c8e459611..131e4f2b5431 100644
>> --- a/drivers/irqchip/irq-riscv-imsic-platform.c
>> +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
>> @@ -93,9 +93,13 @@ static void imsic_irq_compose_msg(struct irq_data *d, struct msi_msg *msg)
>>   static void imsic_msi_update_msg(struct irq_data *d, struct imsic_vector *vec)
>>   {
>>   	struct msi_msg msg = { };
>> +	struct irq_chip *irq_chip = irq_data_get_irq_chip(d);
>> +
>> +	if (!irq_chip->irq_write_msi_msg)
>> +		return;
> I have no idea how this ever worked. The irq_data pointer belongs to the
> IMSIC base domain, which definitely does not have a irq_write_msi_msg()
> callback and never can have one.
>
> The write message callback is always implemented by the top most domain,
> in this case the PCI/MSI[x] per device domain.
>
> So this code is simply broken and your NULL pointer check just makes it
> differently broken.
Sorry, my mistake, the NULL pointer issue has been fixed by commit 
(c475c0b71314 irqchip/riscv-imsic: Remove redundant irq_data lookups)
>>   	imsic_irq_compose_vector_msg(vec, &msg);
>> -	irq_data_get_irq_chip(d)->irq_write_msi_msg(d, &msg);
>> +	irq_chip->irq_write_msi_msg(d, &msg);
>>   }
>>   
>>   static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>> @@ -173,7 +177,7 @@ static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask
>>   	/* Move state of the old vector to the new vector */
>>   	imsic_vector_move(old_vec, new_vec);
>>   
>> -	return IRQ_SET_MASK_OK_DONE;
>> +	return IRQ_SET_MASK_OK;
> Have you actually looked at the consequences of this change?
>
>>   }
>>   
>>   static void imsic_irq_force_complete_move(struct irq_data *d)
>> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
>> index b6cebfee9461..cd1bf9516878 100644
>> --- a/drivers/irqchip/irq-riscv-imsic-state.c
>> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
>> @@ -362,6 +362,10 @@ static bool imsic_vector_move_update(struct imsic_local_priv *lpriv,
>>   	/* Update enable and move details */
>>   	enabled = READ_ONCE(vec->enable);
>>   	WRITE_ONCE(vec->enable, new_enable);
>> +
>> +	if (!cpu_online(vec->cpu) && is_old_vec)
>> +		goto out;
> This is definitely not correct as this should still cleanup software
> state, no?

if vec->move_next is not cleared when the cpu is offline, the following 
issue will occur.

cat /proc/interrupts
     CPU0  CPU1  CPU2 CPU3
23:   0       0         0       66   PCI-MSIX-0000:00:01.0 eth0-rx-0

echo 0 > /sys/bus/cpu/devices/cpu3/online

cat /proc/interrupts
     CPU0  CPU1  CPU2
23:   0       0         66   PCI-MSIX-0000:00:01.0 eth0-rx-0

echo 0 > /sys/bus/cpu/devices/cpu2/online
[   35.697380] IRQ23: set affinity failed(-16).
[   35.698381] CPU2: off

> Thanks,
>
>          tglx

Re: [PATCH] irqchip/riscv-imsic: Fix irq migration failure issue when cpu hotplug.

Posted by Anup Patel 3 days, 22 hours ago

On Tue, Feb 3, 2026 at 7:05 PM Thomas Gleixner <tglx@kernel.org> wrote:
>
> On Tue, Feb 03 2026 at 16:02, Yingjun Ni wrote:
> > Add a null pointer check for irq_write_msi_msg to fix NULL pointer
> > dereference issue when migrating irq.
> >
> > Modify the return value of imsic_irq_set_affinity to let the subdomain
> > PCI-MSIX migrate the irq to a new cpu when cpu hotplug.
> >
> > Don't set vec->move_next in imsic_vector_move_update when the cpu is
> > offline, because it will never be cleared.
>
> You completely fail to explain the actual problem and the root
> cause. See
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> >  drivers/irqchip/irq-riscv-imsic-platform.c | 8 ++++++--
> >  drivers/irqchip/irq-riscv-imsic-state.c    | 5 +++++
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
> > index 643c8e459611..131e4f2b5431 100644
> > --- a/drivers/irqchip/irq-riscv-imsic-platform.c
> > +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
> > @@ -93,9 +93,13 @@ static void imsic_irq_compose_msg(struct irq_data *d, struct msi_msg *msg)
> >  static void imsic_msi_update_msg(struct irq_data *d, struct imsic_vector *vec)
> >  {
> >       struct msi_msg msg = { };
> > +     struct irq_chip *irq_chip = irq_data_get_irq_chip(d);
> > +
> > +     if (!irq_chip->irq_write_msi_msg)
> > +             return;
>
> I have no idea how this ever worked. The irq_data pointer belongs to the
> IMSIC base domain, which definitely does not have a irq_write_msi_msg()
> callback and never can have one.

The imsic_irq_set_affinity() passes irq_get_irq_data(d->irq) as
irq_data pointer to imsic_msi_update_msg() expecting it to be
the top-level irq_data. The imsic_msi_update_msg() assumes
that the top-level irq_data always has irq_write_msi_msg() but if
this assumption is not correct then we need an if-check over here.

>
> The write message callback is always implemented by the top most domain,
> in this case the PCI/MSI[x] per device domain.
>
> So this code is simply broken and your NULL pointer check just makes it
> differently broken.
>
> >       imsic_irq_compose_vector_msg(vec, &msg);
> > -     irq_data_get_irq_chip(d)->irq_write_msi_msg(d, &msg);
> > +     irq_chip->irq_write_msi_msg(d, &msg);
> >  }
> >
> >  static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> > @@ -173,7 +177,7 @@ static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask
> >       /* Move state of the old vector to the new vector */
> >       imsic_vector_move(old_vec, new_vec);
> >
> > -     return IRQ_SET_MASK_OK_DONE;
> > +     return IRQ_SET_MASK_OK;
>
> Have you actually looked at the consequences of this change?

I agree. This seems unrelated and there is no explanation
in the commit description.

>
> >  }
> >
> >  static void imsic_irq_force_complete_move(struct irq_data *d)
> > diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> > index b6cebfee9461..cd1bf9516878 100644
> > --- a/drivers/irqchip/irq-riscv-imsic-state.c
> > +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> > @@ -362,6 +362,10 @@ static bool imsic_vector_move_update(struct imsic_local_priv *lpriv,
> >       /* Update enable and move details */
> >       enabled = READ_ONCE(vec->enable);
> >       WRITE_ONCE(vec->enable, new_enable);
> > +
> > +     if (!cpu_online(vec->cpu) && is_old_vec)
> > +             goto out;
>
> This is definitely not correct as this should still cleanup software
> state, no?
>
> Thanks,
>
>         tglx

Regards,
Anup

Re: [PATCH] irqchip/riscv-imsic: Fix irq migration failure issue when cpu hotplug.

Posted by Thomas Gleixner 3 days, 20 hours ago

On Tue, Feb 03 2026 at 22:39, Anup Patel wrote:
> On Tue, Feb 3, 2026 at 7:05 PM Thomas Gleixner <tglx@kernel.org> wrote:
>> >  static void imsic_msi_update_msg(struct irq_data *d, struct imsic_vector *vec)
>> >  {
>> >       struct msi_msg msg = { };
>> > +     struct irq_chip *irq_chip = irq_data_get_irq_chip(d);
>> > +
>> > +     if (!irq_chip->irq_write_msi_msg)
>> > +             return;
>>
>> I have no idea how this ever worked. The irq_data pointer belongs to the
>> IMSIC base domain, which definitely does not have a irq_write_msi_msg()
>> callback and never can have one.
>
> The imsic_irq_set_affinity() passes irq_get_irq_data(d->irq) as
> irq_data pointer to imsic_msi_update_msg() expecting it to be
> the top-level irq_data. The imsic_msi_update_msg() assumes
> that the top-level irq_data always has irq_write_msi_msg() but if
> this assumption is not correct then we need an if-check over here.

Indeed. I misread that part and yes, you can assume that the top level
domain has a write MSI callback because that's what this MSI parent
domain is about.

We might check for that in the msi lib. See below

Thanks,

        tglx
---        
--- a/drivers/irqchip/irq-msi-lib.c
+++ b/drivers/irqchip/irq-msi-lib.c
@@ -48,6 +48,9 @@ bool msi_lib_init_dev_msi_info(struct de
 		return false;
 	}
 
+	if (WARN__ON_ONCE(!chip->irq_write_msi_msg))
+		return false;
+
 	required_flags = pops->required_flags;
 
 	/* Is the target domain bus token supported? */

[PATCH] irqchip/msi-lib: Refuse initialization when irq_write_msi_msg() is missing

Posted by Thomas Gleixner 3 days, 18 hours ago

MSI parent domains rely on the fact that the top level device domain
provides a irq_write_msi_msg() callback.

Check for that and if missing warn and refuse to initialize the device domain.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 drivers/irqchip/irq-msi-lib.c |    3 +++
 1 file changed, 3 insertions(+)

--- a/drivers/irqchip/irq-msi-lib.c
+++ b/drivers/irqchip/irq-msi-lib.c
@@ -48,6 +48,9 @@ bool msi_lib_init_dev_msi_info(struct de
 		return false;
 	}
 
+	if (WARN_ON_ONCE(!chip->irq_write_msi_msg))
+		return false;
+
 	required_flags = pops->required_flags;
 
 	/* Is the target domain bus token supported? */