On 17/12/2020 14:13, Alexandru Elisei wrote:
> One common usage for IPIs is for one CPU to write to a shared memory
> location, send the IPI to kick another CPU, and the receiver to read from
> the same location. Proper synchronization is needed to make sure that the
> IPI receiver reads the most recent value and not stale data (for example,
> the write from the sender CPU might still be in a store buffer).
>
> For GICv3, IPIs are generated with a write to the ICC_SGI1R_EL1 register.
> To make sure the memory stores are observable by other CPUs, we need a
> wmb() barrier (DSB ST), which waits for stores to complete.
>
> From the definition of DSB from ARM DDI 0487F.b, page B2-139:
>
> "In addition, no instruction that appears in program order after the DSB
> instruction can alter any state of the system or perform any part of its
> functionality until the DSB completes other than:
>
> - Being fetched from memory and decoded.
> - Reading the general-purpose, SIMD and floating-point, Special-purpose, or
> System registers that are directly or indirectly read without causing
> side-effects."
>
> Similar definition for armv7 (ARM DDI 0406C.d, page A3-150).
>
> The DSB instruction is enough to prevent reordering of the GIC register
> write which comes in program order after the memory access.
>
> This also matches what the Linux GICv3 irqchip driver does (commit
> 21ec30c0ef52 ("irqchip/gic-v3: Use wmb() instead of smb_wmb() in
> gic_raise_softirq()")).
>
> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
Yes, makes sense. Also verified the references in both the ARM ARM and
the Linux code.
Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
Cheers,
Andre
> ---
> lib/arm/gic-v3.c | 6 ++++++
> arm/gic.c | 5 +++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> index a7e2cb819746..2c067e4e9ba2 100644
> --- a/lib/arm/gic-v3.c
> +++ b/lib/arm/gic-v3.c
> @@ -77,6 +77,12 @@ void gicv3_ipi_send_mask(int irq, const cpumask_t *dest)
>
> assert(irq < 16);
>
> + /*
> + * Ensure stores to Normal memory are visible to other CPUs before
> + * sending the IPI.
> + */
> + wmb();
> +
> /*
> * For each cpu in the mask collect its peers, which are also in
> * the mask, in order to form target lists.
> diff --git a/arm/gic.c b/arm/gic.c
> index acb060585fae..fee48f9b4ccb 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -275,6 +275,11 @@ static void gicv3_ipi_send_self(void)
>
> static void gicv3_ipi_send_broadcast(void)
> {
> + /*
> + * Ensure stores to Normal memory are visible to other CPUs before
> + * sending the IPI
> + */
> + wmb();
> gicv3_write_sgi1r(1ULL << 40 | IPI_IRQ << 24);
> isb();
> }
>
