Date:   Mon, 24 Jan 2022 13:30:06 +0100
From:   Heiko Stübner <heiko@...ech.de>
To:     Atish Patra <atishp@...shpatra.org>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        devicetree <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>, Wei Fu <wefu@...hat.com>,
        liush <liush@...winnertech.com>, Guo Ren <guoren@...nel.org>,
        Anup Patel <anup@...infault.org>,
        Drew Fustini <drew@...gleboard.org>,
        Christoph Hellwig <hch@....de>, Arnd Bergmann <arnd@...db.de>,
        Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <maxime@...no.tech>,
        Daniel Lustig <dlustig@...dia.com>,
        Greg Favor <gfavor@...tanamicro.com>,
        Andrea Mondelli <andrea.mondelli@...wei.com>,
        Jonathan Behrens <behrensj@....edu>,
        Xinhaoqu <xinhaoqu@...wei.com>,
        Bill Huffman <huffman@...ence.com>,
        Nick Kossifidis <mick@....forth.gr>,
        Allen Baum <allen.baum@...erantotech.com>,
        Josh Scheid <jscheid@...tanamicro.com>,
        Richard Trauben <rtrauben@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Christoph Muellner <cmuellner@...ux.com>,
        Philipp Tomsich <philipp.tomsich@...ll.eu>
Subject: Re: [PATCH v5 01/14] riscv: only use IPIs to handle cache-flushes on remote cpus

Hi Atish,

Am Samstag, 22. Januar 2022, 04:45:52 CET schrieb Atish Patra:
> On Fri, Jan 21, 2022 at 8:37 AM Heiko Stuebner <heiko@...ech.de> wrote:
> >
> > Right now, the flush_icache functions always use the SBI remote-fence
> > when SBI is available, leaving using IPIs as a fallback mechanism.
> >
> > IPIs on the other hand are more flexible, as the ipi_ops are initially
> > set to go through SBI but later will be overwritten to go through the
> > ACLINT/CLINT.
> >
> > In a discussion we had, Nick was of the opinion that "In general we
> > should prefer doing IPIs on S-mode through CLINT instead of going
> > through SBI/M-mode,
> 
> Yes. Once Anup's ACLINT drivers are merged, that should be the
> preferred approach.
> 
> https://github.com/avpatel/linux/commit/416c667fd77d6f1fc310cbf727ec127aaf96cae2
> 
> >so IMHO we should only be using
> > on_each_cpu_mask(ipi_remote_fence_i) on flush_icache_all()/
> > flush_icache_mm() and remove any explicit calls to sbi_remote_fence_i(),
> 
> That's a bit confusing because we will be using SBI calls for all other fences
> while using IPIs for fence.i
> 
> > because this way we continue using SBI for doing remote fences even after
> > CLINT/ACLINT driver is registered, instead of using direct IPIs through
> > CLINT/ACLINT."
> >
> > So follow this suggestion and just do ipi calls to have the proper kernel
> > parts do them,
> >
> > This also fixes the null-ptr dereference happening when flush_icache_all()
> > is called before sbi_init().
> >
> 
> IMHO, this series should only fix the null-ptr dereference issue.
> The IPI based fence (for all) should only be disabled along with the
> ACLINT driver
> that actually enables S-mode IPIs.

ok, I'll roll this back to simply fixing the null-ptr issue.

Meanwhile I even found a nicer solution without actually touching
the cachflush code.

Without sbi_init() we can assume that we're still before smp bringup,
so the local_flush_icache_all() in flush_icache_all() will do the trick
just fine and sbi_remote_fence_i() simply just needs to be an empty
function until sbi is initialized.


Heiko

> 
> > Suggested-by: Nick Kossifidis <mick@....forth.gr>
> > Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> > ---
> >  arch/riscv/mm/cacheflush.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 6cb7d96ad9c7..c35375cd52ec 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -17,11 +17,7 @@ static void ipi_remote_fence_i(void *info)
> >  void flush_icache_all(void)
> >  {
> >         local_flush_icache_all();
> > -
> > -       if (IS_ENABLED(CONFIG_RISCV_SBI))
> > -               sbi_remote_fence_i(NULL);
> > -       else
> > -               on_each_cpu(ipi_remote_fence_i, NULL, 1);
> > +       on_each_cpu(ipi_remote_fence_i, NULL, 1);
> >  }
> >  EXPORT_SYMBOL(flush_icache_all);
> >
> > @@ -66,8 +62,6 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
> >                  * with flush_icache_deferred().
> >                  */
> >                 smp_mb();
> > -       } else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > -               sbi_remote_fence_i(&others);
> >         } else {
> >                 on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> >         }
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> 

Powered by blists - more mailing lists