Re: [PATCH] irqchip/gic-v3: use dsb(ishst) to synchronize data to smp before issuing ipi

[Marc Zyngier]

On 2022-02-20 13:30, Ard Biesheuvel wrote:
> On Sat, 19 Feb 2022 at 10:57, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > On 2022-02-18 21:55, Barry Song wrote:
> > > dsb(ishst) should be enough here as we only need to guarantee the
> > > visibility of data to other CPUs in smp inner domain before we
> > > send the ipi.
> > >
> > > Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/irqchip/irq-gic-v3.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/irqchip/irq-gic-v3.c
> > > b/drivers/irqchip/irq-gic-v3.c
> > > index 5e935d97207d..0efe1a9a9f3b 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -1211,7 +1211,7 @@ static void gic_ipi_send_mask(struct irq_data
> > > *d, const struct cpumask *mask)
> > >        * Ensure that stores to Normal memory are visible to the
> > >        * other CPUs before issuing the IPI.
> > >        */
> > > -     wmb();
> > > +     dsb(ishst);
> > >
> > >       for_each_cpu(cpu, mask) {
> > >               u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
> >
> > I'm not opposed to that change, but I'm pretty curious whether this
> > makes
> > any visible difference in practice. Could you measure the effect of 
> > this
> > change
> > for any sort of IPI heavy workload?
>
> Does this have to be a DSB ?

We can use a DMB ISHST for the other interrupt controllers that use a
MMIO access to signal the IPI, as we need to order the previous writes 
with
the MMIO access (and DMB fits that bill).

For GICv3 when ICC_SRE_EL1.SRE==1, we need to order a set of writes with
a system register access with side effects, and the only barrier that
allows us to do that is DSB.

It is a bit unfortunate that the relative lightweight nature of the 
sysreg
CPU interface is encumbered by fairly heavy barriers (the most horrible 
one
being the DSB SY on each read of IAR to be able to synchronise the CPU 
interface
and the redistributor).

Thanks,

        M.
--
Jazz is not dead. It just smells funny...