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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Feb 2022 09:09:20 +1300
From:   Barry Song <21cnbao@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Ard Biesheuvel <ardb@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linuxarm <linuxarm@...wei.com>,
        Barry Song <song.bao.hua@...ilicon.com>
Subject: Re: [PATCH] irqchip/gic-v3: use dsb(ishst) to synchronize data to smp
 before issuing ipi

On Mon, Feb 21, 2022 at 4:05 AM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> On Sun, Feb 20, 2022 at 02:30:24PM +0100, Ard Biesheuvel wrote:
> > On Sat, 19 Feb 2022 at 10:57, Marc Zyngier <maz@...nel.org> 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@...ilicon.com>
> > > > ---
> > > >  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 ?
>
> Are you suggesting that smp_wmb() may suffice (which is a dmb(ishst)) ?

usually smp_wmb() is ok, for example drivers/irqchip/irq-bcm2836.c:

static void bcm2836_arm_irqchip_ipi_send_mask(struct irq_data *d,
                                              const struct cpumask *mask)
{
        int cpu;
        void __iomem *mailbox0_base = intc.base + LOCAL_MAILBOX0_SET0;

        /*
         * Ensure that stores to normal memory are visible to the
         * other CPUs before issuing the IPI.
         */
        smp_wmb();

        for_each_cpu(cpu, mask)
                writel_relaxed(BIT(d->hwirq), mailbox0_base + 16 * cpu);
}

but the instruction to send ipi for irq-gic-v3.c isn't a store, so
this driver has been
changed by this commit from dmb(ish) to dsb(sy):

commit 21ec30c0ef5234fb1039cc7c7737d885bf875a9e
Author: Shanker Donthineni <shankerd@...eaurora.org>

    irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()

    A DMB instruction can be used to ensure the relative order of only
    memory accesses before and after the barrier. Since writes to system
    registers are not memory operations, barrier DMB is not sufficient
    for observability of memory accesses that occur before ICC_SGI1R_EL1
    writes.

    A DSB instruction ensures that no instructions that appear in program
    order after the DSB instruction, can execute until the DSB instruction
    has completed.

    ...

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d71be9a1f9d2..d99cc07903ec 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask
*mask, unsigned int irq)
         * Ensure that stores to Normal memory are visible to the
         * other CPUs before issuing the IPI.
         */
-       smp_wmb();
+       wmb();

        for_each_cpu(cpu, mask) {
                u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));

my understanding is that dtb(sy) is too strong as this case we are
asking data to
be observed by other CPUs in smp just as smp_wmb is doing that in other drivers
by dmb(isb). we are not requiring data is observed by everyone in the system.

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Thanks
Barry