Date:   Fri, 21 Jan 2022 19:45:52 -0800
From:   Atish Patra <atishp@...shpatra.org>
To:     Heiko Stuebner <heiko@...ech.de>
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

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.

> 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



-- 
Regards,
Atish

Powered by blists - more mailing lists