[3/5] irqchip/riscv-imsic: Use wmb() to order normal writes and IPI writes

Commit Message

Xu Lu Jan. 13, 2025, 3:09 p.m. UTC

During an IPI procedure, we need to ensure all previous write operations
are visible to other CPUs before sending a real IPI. We use wmb() barrier
to ensure this as IMSIC issues IPI via mmio writes.

Signed-off-by: Xu Lu <[e-mail]>
---
 drivers/irqchip/irq-riscv-imsic-early.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Anup Patel Jan. 14, 2025, 4:34 a.m. UTC | #1

On Mon, Jan 13, 2025 at 8:39 PM Xu Lu <[e-mail]> wrote:
>
> During an IPI procedure, we need to ensure all previous write operations
> are visible to other CPUs before sending a real IPI. We use wmb() barrier
> to ensure this as IMSIC issues IPI via mmio writes.
>
> Signed-off-by: Xu Lu <[e-mail]>
> ---
>  drivers/irqchip/irq-riscv-imsic-early.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> index 63097f2bbadf..c6317cb557fb 100644
> --- a/drivers/irqchip/irq-riscv-imsic-early.c
> +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> @@ -29,6 +29,12 @@ static void imsic_ipi_send(unsigned int cpu)
>  {
>         struct imsic_local_config *local = per_cpu_ptr(imsic->global.local, cpu);
>
> +       /*
> +        * Ensure that stores to normal memory are visible to the other CPUs
> +        * before issuing IPI.
> +        */
> +       wmb();
> +

The imsic_ipi_send() is called through ipi_mux_send_mask()
which does smp_mb__after_atomic() before calling so no need
for any barrier here. Also, barriers need to be in-pair so adding
a single barrier at random location is inappropriate.
(Refer, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/ipi-mux.c?h=v6.13-rc7#n78)

Based on the above, this patch is not needed.

Regards,
Anup

Xu Lu Jan. 14, 2025, 6:39 a.m. UTC | #2

Hi Anup,

I want to ensure when the receiver CPU traps into interrupt, it can
see all memory updates made by the sender before.

The smp_mb__after_atomic() uses 'fence rw, rw' which orders normal
memory reads/writes. The IMSIC write is actually IO writes and is not
ordered by it.

So I worry in extreme cases, the receiver CPU can receive the IPI
first, see no updates to ipi_mux_pcpu->bits and return immediately.
Then this IPI will not be handled until another IPI comes.

Best Regards,

Xu Lu

On Tue, Jan 14, 2025 at 12:34 PM Anup Patel <[e-mail]> wrote:
>
> On Mon, Jan 13, 2025 at 8:39 PM Xu Lu <[e-mail]> wrote:
> >
> > During an IPI procedure, we need to ensure all previous write operations
> > are visible to other CPUs before sending a real IPI. We use wmb() barrier
> > to ensure this as IMSIC issues IPI via mmio writes.
> >
> > Signed-off-by: Xu Lu <[e-mail]>
> > ---
> >  drivers/irqchip/irq-riscv-imsic-early.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> > index 63097f2bbadf..c6317cb557fb 100644
> > --- a/drivers/irqchip/irq-riscv-imsic-early.c
> > +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> > @@ -29,6 +29,12 @@ static void imsic_ipi_send(unsigned int cpu)
> >  {
> >         struct imsic_local_config *local = per_cpu_ptr(imsic->global.local, cpu);
> >
> > +       /*
> > +        * Ensure that stores to normal memory are visible to the other CPUs
> > +        * before issuing IPI.
> > +        */
> > +       wmb();
> > +
>
> The imsic_ipi_send() is called through ipi_mux_send_mask()
> which does smp_mb__after_atomic() before calling so no need
> for any barrier here. Also, barriers need to be in-pair so adding
> a single barrier at random location is inappropriate.
> (Refer, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/ipi-mux.c?h=v6.13-rc7#n78)
>
> Based on the above, this patch is not needed.
>
> Regards,
> Anup

Anup Patel Jan. 14, 2025, 8:58 a.m. UTC | #3

On Tue, Jan 14, 2025 at 12:09 PM Xu Lu <[e-mail]> wrote:
>
> Hi Anup,
>
> I want to ensure when the receiver CPU traps into interrupt, it can
> see all memory updates made by the sender before.
>
> The smp_mb__after_atomic() uses 'fence rw, rw' which orders normal
> memory reads/writes. The IMSIC write is actually IO writes and is not
> ordered by it.
>
> So I worry in extreme cases, the receiver CPU can receive the IPI
> first, see no updates to ipi_mux_pcpu->bits and return immediately.
> Then this IPI will not be handled until another IPI comes.

Using wmb() is still inappropriate because if you want to simply
ensure IO writes are not ordered before the normal memory writes
then just convert writel_relaxed() to write().

Regards,
Anup

>
> Best Regards,
>
> Xu Lu
>
> On Tue, Jan 14, 2025 at 12:34 PM Anup Patel <[e-mail]> wrote:
> >
> > On Mon, Jan 13, 2025 at 8:39 PM Xu Lu <[e-mail]> wrote:
> > >
> > > During an IPI procedure, we need to ensure all previous write operations
> > > are visible to other CPUs before sending a real IPI. We use wmb() barrier
> > > to ensure this as IMSIC issues IPI via mmio writes.
> > >
> > > Signed-off-by: Xu Lu <[e-mail]>
> > > ---
> > >  drivers/irqchip/irq-riscv-imsic-early.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> > > index 63097f2bbadf..c6317cb557fb 100644
> > > --- a/drivers/irqchip/irq-riscv-imsic-early.c
> > > +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> > > @@ -29,6 +29,12 @@ static void imsic_ipi_send(unsigned int cpu)
> > >  {
> > >         struct imsic_local_config *local = per_cpu_ptr(imsic->global.local, cpu);
> > >
> > > +       /*
> > > +        * Ensure that stores to normal memory are visible to the other CPUs
> > > +        * before issuing IPI.
> > > +        */
> > > +       wmb();
> > > +
> >
> > The imsic_ipi_send() is called through ipi_mux_send_mask()
> > which does smp_mb__after_atomic() before calling so no need
> > for any barrier here. Also, barriers need to be in-pair so adding
> > a single barrier at random location is inappropriate.
> > (Refer, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/ipi-mux.c?h=v6.13-rc7#n78)
> >
> > Based on the above, this patch is not needed.
> >
> > Regards,
> > Anup

Xu Lu Jan. 14, 2025, 9:07 a.m. UTC | #4

On Tue, Jan 14, 2025 at 4:59 PM Anup Patel <[e-mail]> wrote:
>
> On Tue, Jan 14, 2025 at 12:09 PM Xu Lu <[e-mail]> wrote:
> >
> > Hi Anup,
> >
> > I want to ensure when the receiver CPU traps into interrupt, it can
> > see all memory updates made by the sender before.
> >
> > The smp_mb__after_atomic() uses 'fence rw, rw' which orders normal
> > memory reads/writes. The IMSIC write is actually IO writes and is not
> > ordered by it.
> >
> > So I worry in extreme cases, the receiver CPU can receive the IPI
> > first, see no updates to ipi_mux_pcpu->bits and return immediately.
> > Then this IPI will not be handled until another IPI comes.
>
> Using wmb() is still inappropriate because if you want to simply
> ensure IO writes are not ordered before the normal memory writes
> then just convert writel_relaxed() to write().

Got it. I will use writel() instead.

Thanks!

Xu Lu

>
> Regards,
> Anup
>
> >
> > Best Regards,
> >
> > Xu Lu
> >
> > On Tue, Jan 14, 2025 at 12:34 PM Anup Patel <[e-mail]> wrote:
> > >
> > > On Mon, Jan 13, 2025 at 8:39 PM Xu Lu <[e-mail]> wrote:
> > > >
> > > > During an IPI procedure, we need to ensure all previous write operations
> > > > are visible to other CPUs before sending a real IPI. We use wmb() barrier
> > > > to ensure this as IMSIC issues IPI via mmio writes.
> > > >
> > > > Signed-off-by: Xu Lu <[e-mail]>
> > > > ---
> > > >  drivers/irqchip/irq-riscv-imsic-early.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> > > > index 63097f2bbadf..c6317cb557fb 100644
> > > > --- a/drivers/irqchip/irq-riscv-imsic-early.c
> > > > +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> > > > @@ -29,6 +29,12 @@ static void imsic_ipi_send(unsigned int cpu)
> > > >  {
> > > >         struct imsic_local_config *local = per_cpu_ptr(imsic->global.local, cpu);
> > > >
> > > > +       /*
> > > > +        * Ensure that stores to normal memory are visible to the other CPUs
> > > > +        * before issuing IPI.
> > > > +        */
> > > > +       wmb();
> > > > +
> > >
> > > The imsic_ipi_send() is called through ipi_mux_send_mask()
> > > which does smp_mb__after_atomic() before calling so no need
> > > for any barrier here. Also, barriers need to be in-pair so adding
> > > a single barrier at random location is inappropriate.
> > > (Refer, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/ipi-mux.c?h=v6.13-rc7#n78)
> > >
> > > Based on the above, this patch is not needed.
> > >
> > > Regards,
> > > Anup

Patch

diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
index 63097f2bbadf..c6317cb557fb 100644
--- a/drivers/irqchip/irq-riscv-imsic-early.c
+++ b/drivers/irqchip/irq-riscv-imsic-early.c
@@ -29,6 +29,12 @@  static void imsic_ipi_send(unsigned int cpu)
 {
 	struct imsic_local_config *local = per_cpu_ptr(imsic->global.local, cpu);
 
+	/*
+	 * Ensure that stores to normal memory are visible to the other CPUs
+	 * before issuing IPI.
+	 */
+	wmb();
+
 	writel_relaxed(IMSIC_IPI_ID, local->msi_va);
 }