[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Anup Patel]

This series improves the barrier usage around device IPI callbacks
such that platform IPI driver can now use relaxed MMIO writes in
device IPI callbacks.

These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
at: https://github.com/avpatel/opensbi.git

Anup Patel (2):
  lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
  lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback

 lib/sbi/sbi_ipi.c           | 31 +++++++++++++++++++++++++------
 lib/utils/ipi/aclint_mswi.c |  4 ++--
 lib/utils/irqchip/imsic.c   |  4 ++--
 3 files changed, 29 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH 1/2] lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback

[Anup Patel]

Currently, we have a smp_wmb() between atomic_raw_set_bit() and
ipi_send() device callback whereas the MMIO writes done by the
device ipi_send() callback will also include a barrier.

We can avoid unnecessary/redundant barriers described above by
allowing relaxed MMIO writes in device ipi_send() callback. To
achieve this, we simply use  wmb() instead of smp_wmb() before
calling device ipi_send().

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/sbi/sbi_ipi.c           | 15 +++++++++++----
 lib/utils/ipi/aclint_mswi.c |  2 +-
 lib/utils/irqchip/imsic.c   |  4 ++--
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c
index 5571b37..1694a23 100644
--- a/lib/sbi/sbi_ipi.c
+++ b/lib/sbi/sbi_ipi.c
@@ -67,14 +67,12 @@ static int sbi_ipi_send(struct sbi_scratch *scratch, u32 remote_hartindex,
 	 * trigger the interrupt
 	 */
 	atomic_raw_set_bit(event, &ipi_data->ipi_type);
-	smp_wmb();
 
-	if (ipi_dev && ipi_dev->ipi_send)
-		ipi_dev->ipi_send(remote_hartindex);
+	ret = sbi_ipi_raw_send(remote_hartindex);
 
 	sbi_pmu_ctr_incr_fw(SBI_PMU_FW_IPI_SENT);
 
-	return 0;
+	return ret;
 }
 
 static int sbi_ipi_sync(struct sbi_scratch *scratch, u32 event)
@@ -243,6 +241,15 @@ int sbi_ipi_raw_send(u32 hartindex)
 	if (!ipi_dev || !ipi_dev->ipi_send)
 		return SBI_EINVAL;
 
+	/*
+	 * Ensure that memory or MMIO writes done before
+	 * this function are not observed after the memory
+	 * or MMIO writes done by the ipi_send() device
+	 * callback. This also allows the ipi_send() device
+	 * callback to use relaxed MMIO writes.
+	 */
+	wmb();
+
 	ipi_dev->ipi_send(hartindex);
 	return 0;
 }
diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
index a3bfb4a..bfd6a45 100644
--- a/lib/utils/ipi/aclint_mswi.c
+++ b/lib/utils/ipi/aclint_mswi.c
@@ -41,7 +41,7 @@ static void mswi_ipi_send(u32 hart_index)
 
 	/* Set ACLINT IPI */
 	msip = (void *)mswi->addr;
-	writel(1, &msip[sbi_hartindex_to_hartid(hart_index) -
+	writel_relaxed(1, &msip[sbi_hartindex_to_hartid(hart_index) -
 			mswi->first_hartid]);
 }
 
diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
index 78f5895..36ef66c 100644
--- a/lib/utils/irqchip/imsic.c
+++ b/lib/utils/irqchip/imsic.c
@@ -186,8 +186,8 @@ static void imsic_ipi_send(u32 hart_index)
 	}
 
 	if (regs->size && (reloff < regs->size))
-		writel(IMSIC_IPI_ID,
-		       (void *)(regs->addr + reloff + IMSIC_MMIO_PAGE_LE));
+		writel_relaxed(IMSIC_IPI_ID,
+			(void *)(regs->addr + reloff + IMSIC_MMIO_PAGE_LE));
 }
 
 static struct sbi_ipi_device imsic_ipi_device = {
-- 
2.34.1

[PATCH 2/2] lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback

[Anup Patel]

Currently, there are no barriers before or after the ipi_clear()
device callback which forces ipi_clear() device callback to always
use non-relaxed MMIO writes.

Instead of above, we use wmb() in after the ipi_clear() device
callback which pairs with the wmb() done before the ipi_send()
device callback. This also allows device ipi_clear() callback
to use relaxed MMIO writes.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/sbi/sbi_ipi.c           | 16 ++++++++++++++--
 lib/utils/ipi/aclint_mswi.c |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/sbi/sbi_ipi.c b/lib/sbi/sbi_ipi.c
index 1694a23..0bf446a 100644
--- a/lib/sbi/sbi_ipi.c
+++ b/lib/sbi/sbi_ipi.c
@@ -220,8 +220,7 @@ void sbi_ipi_process(void)
 	u32 hartindex = sbi_hartid_to_hartindex(current_hartid());
 
 	sbi_pmu_ctr_incr_fw(SBI_PMU_FW_IPI_RECVD);
-	if (ipi_dev && ipi_dev->ipi_clear)
-		ipi_dev->ipi_clear(hartindex);
+	sbi_ipi_raw_clear(hartindex);
 
 	ipi_type = atomic_raw_xchg_ulong(&ipi_data->ipi_type, 0);
 	ipi_event = 0;
@@ -247,6 +246,8 @@ int sbi_ipi_raw_send(u32 hartindex)
 	 * or MMIO writes done by the ipi_send() device
 	 * callback. This also allows the ipi_send() device
 	 * callback to use relaxed MMIO writes.
+	 *
+	 * This pairs with the wmb() in sbi_ipi_raw_clear().
 	 */
 	wmb();
 
@@ -258,6 +259,17 @@ void sbi_ipi_raw_clear(u32 hartindex)
 {
 	if (ipi_dev && ipi_dev->ipi_clear)
 		ipi_dev->ipi_clear(hartindex);
+
+	/*
+	 * Ensure that memory or MMIO writes after this
+	 * function returns are not observed before the
+	 * memory or MMIO writes done by the ipi_clear()
+	 * device callback. This also allows ipi_clear()
+	 * device callback to use relaxed MMIO writes.
+	 *
+	 * This pairs with the wmb() in sbi_ipi_raw_send().
+	 */
+	wmb();
 }
 
 const struct sbi_ipi_device *sbi_ipi_get_device(void)
diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
index bfd6a45..4ae6bb1 100644
--- a/lib/utils/ipi/aclint_mswi.c
+++ b/lib/utils/ipi/aclint_mswi.c
@@ -61,7 +61,7 @@ static void mswi_ipi_clear(u32 hart_index)
 
 	/* Clear ACLINT IPI */
 	msip = (void *)mswi->addr;
-	writel(0, &msip[sbi_hartindex_to_hartid(hart_index) -
+	writel_relaxed(0, &msip[sbi_hartindex_to_hartid(hart_index) -
 			mswi->first_hartid]);
 }
 
-- 
2.34.1

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Xiang W]

? 2023-11-21???? 21:47 +0530?Anup Patel???
> This series improves the barrier usage around device IPI callbacks
> such that platform IPI driver can now use relaxed MMIO writes in
> device IPI callbacks.
> 
> These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
> at: https://github.com/avpatel/opensbi.git
This patch is partly similar to this one?
https://lists.infradead.org/pipermail/opensbi/2023-November/005931.html

Then we may also encounter the following case:

                hart A             hart B
send ipi--------->|                  |
                  | clear ipi        | 
                  |                  | set ipi_type
                  |                  |
                  | xchg ipi_type    |
                  |                  |
                  |<-----------------| send ipi
                  |                  |
                  |                  |

To deal with this case, my patch:
https://lists.infradead.org/pipermail/opensbi/2023-November/005932.html

Regards,
Xiang W
> 
> Anup Patel (2):
> ? lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
> ? lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback
> 
> ?lib/sbi/sbi_ipi.c?????????? | 31 +++++++++++++++++++++++++------
> ?lib/utils/ipi/aclint_mswi.c |? 4 ++--
> ?lib/utils/irqchip/imsic.c?? |? 4 ++--
> ?3 files changed, 29 insertions(+), 10 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Anup Patel]

On Tue, Nov 21, 2023 at 10:18?PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2023-11-21???? 21:47 +0530?Anup Patel???
> > This series improves the barrier usage around device IPI callbacks
> > such that platform IPI driver can now use relaxed MMIO writes in
> > device IPI callbacks.
> >
> > These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
> > at: https://github.com/avpatel/opensbi.git
> This patch is partly similar to this one?
> https://lists.infradead.org/pipermail/opensbi/2023-November/005931.html
>
> Then we may also encounter the following case:
>
>                 hart A             hart B
> send ipi--------->|                  |
>                   | clear ipi        |
>                   |                  | set ipi_type
>                   |                  |
>                   | xchg ipi_type    |
>                   |                  |
>                   |<-----------------| send ipi
>                   |                  |
>                   |                  |

This case is handled.

>
> To deal with this case, my patch:
> https://lists.infradead.org/pipermail/opensbi/2023-November/005932.html

Using spinlock for IPI synchronization is very handed
and reduces performance.

Functionally we have an IPI mux built on-top of one HW IPI
which is functionally similar to the Linux generic IPI-mux.

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > Anup Patel (2):
> >   lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
> >   lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback
> >
> >  lib/sbi/sbi_ipi.c           | 31 +++++++++++++++++++++++++------
> >  lib/utils/ipi/aclint_mswi.c |  4 ++--
> >  lib/utils/irqchip/imsic.c   |  4 ++--
> >  3 files changed, 29 insertions(+), 10 deletions(-)
> >
> > --
> > 2.34.1
> >
> >
>

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Bo Gan]

On 11/21/23 8:17 AM, Anup Patel wrote:
> This series improves the barrier usage around device IPI callbacks
> such that platform IPI driver can now use relaxed MMIO writes in
> device IPI callbacks.
> 
> These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
> at: https://github.com/avpatel/opensbi.git
> 
> Anup Patel (2):
>    lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
>    lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback
> 
>   lib/sbi/sbi_ipi.c           | 31 +++++++++++++++++++++++++------
>   lib/utils/ipi/aclint_mswi.c |  4 ++--
>   lib/utils/irqchip/imsic.c   |  4 ++--
>   3 files changed, 29 insertions(+), 10 deletions(-)
> 
Hi Anup,

Thank you so much for fixing this. I hope my previous discussion helped.
Would you mind having a Reported-by: Bo Gan <ganboing@gmail.com> ?
Really appreciate you helping me understand the code.

Bo

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Xiang W]

? 2023-11-21???? 23:03 +0530?Anup Patel???
> On Tue, Nov 21, 2023 at 10:18?PM Xiang W <wxjstz@126.com> wrote:
> > 
> > ? 2023-11-21???? 21:47 +0530?Anup Patel???
> > > This series improves the barrier usage around device IPI callbacks
> > > such that platform IPI driver can now use relaxed MMIO writes in
> > > device IPI callbacks.
> > > 
> > > These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
> > > at: https://github.com/avpatel/opensbi.git
> > This patch is partly similar to this one?
> > https://lists.infradead.org/pipermail/opensbi/2023-November/005931.html
> > 
> > Then we may also encounter the following case:
> > 
> > ??????????????? hart A???????????? hart B
> > send ipi--------->|????????????????? |
> > ????????????????? | clear ipi??????? |
> > ????????????????? |????????????????? | set ipi_type
> > ????????????????? |????????????????? |
> > ????????????????? | xchg ipi_type??? |
> > ????????????????? |????????????????? |
> > ????????????????? |<-----------------| send ipi
> > ????????????????? |????????????????? |
> > ????????????????? |????????????????? |
> 
> This case is handled.
> 
> > 
> > To deal with this case, my patch:
> > https://lists.infradead.org/pipermail/opensbi/2023-November/005932.html
> 
> Using spinlock for IPI synchronization is very handed
> and reduces performance.
> 
> Functionally we have an IPI mux built on-top of one HW IPI
> which is functionally similar to the Linux generic IPI-mux.

New patch at
https://lists.infradead.org/pipermail/opensbi/2023-November/005965.html

Regards,
Xiang W

> 
> Regards,
> Anup
> 
> > 
> > Regards,
> > Xiang W
> > > 
> > > Anup Patel (2):
> > > ? lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
> > > ? lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback
> > > 
> > > ?lib/sbi/sbi_ipi.c?????????? | 31 +++++++++++++++++++++++++------
> > > ?lib/utils/ipi/aclint_mswi.c |? 4 ++--
> > > ?lib/utils/irqchip/imsic.c?? |? 4 ++--
> > > ?3 files changed, 29 insertions(+), 10 deletions(-)
> > > 
> > > --
> > > 2.34.1
> > > 
> > > 
> > 

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Anup Patel]

On Wed, Nov 22, 2023 at 6:15?AM Bo Gan <ganboing@gmail.com> wrote:
>
> On 11/21/23 8:17 AM, Anup Patel wrote:
> > This series improves the barrier usage around device IPI callbacks
> > such that platform IPI driver can now use relaxed MMIO writes in
> > device IPI callbacks.
> >
> > These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
> > at: https://github.com/avpatel/opensbi.git
> >
> > Anup Patel (2):
> >    lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
> >    lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback
> >
> >   lib/sbi/sbi_ipi.c           | 31 +++++++++++++++++++++++++------
> >   lib/utils/ipi/aclint_mswi.c |  4 ++--
> >   lib/utils/irqchip/imsic.c   |  4 ++--
> >   3 files changed, 29 insertions(+), 10 deletions(-)
> >
> Hi Anup,
>
> Thank you so much for fixing this. I hope my previous discussion helped.
> Would you mind having a Reported-by: Bo Gan <ganboing@gmail.com> ?
> Really appreciate you helping me understand the code.

Sure, I will add Reported-by

Regards,
Anup

[PATCH 0/2] Allow relaxed MMIO writes in device IPI callbacks

[Anup Patel]

On Tue, Nov 21, 2023 at 9:48?PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> This series improves the barrier usage around device IPI callbacks
> such that platform IPI driver can now use relaxed MMIO writes in
> device IPI callbacks.
>
> These patches can also be found in the sbi_ipi_barrier_imp_v1 branch
> at: https://github.com/avpatel/opensbi.git
>
> Anup Patel (2):
>   lib: sbi: Allow relaxed MMIO writes in device ipi_send() callback
>   lib: sbi: Allow relaxed MMIO writes in device ipi_clear() callback

Applied this series to the riscv/opensbi repo.

Regards,
Anup

>
>  lib/sbi/sbi_ipi.c           | 31 +++++++++++++++++++++++++------
>  lib/utils/ipi/aclint_mswi.c |  4 ++--
>  lib/utils/irqchip/imsic.c   |  4 ++--
>  3 files changed, 29 insertions(+), 10 deletions(-)
>
> --
> 2.34.1
>