Re: [PATCH] implement flush_cache_vmap for RISC-V

From: Jiuyang Liu <liu@jiuyang.me>
To: Jisheng Zhang <Jisheng.Zhang@synaptics.com>, Alex Ghiti <alex@ghiti.fr>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Waterman <waterman@eecs.berkeley.edu>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Andrew Morton <akpm@linux-foundation.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] implement flush_cache_vmap for RISC-V
Date: Wed, 14 Apr 2021 09:16:34 +0000	[thread overview]
Message-ID: <9908880.BHKHtspdbC@nuc> (raw)
In-Reply-To: <18d198ac-7bc0-934d-e1e9-eca01b790d61@ghiti.fr>

On Wednesday, April 14, 2021 7:03:13 AM UTC you wrote:
> Hi,
> 
> Le 4/12/21 à 3:08 AM, Jisheng Zhang a écrit :
> > Hi Jiuyang,
> > 
> > On Mon, 12 Apr 2021 00:05:30 +0000 Jiuyang Liu <liu@jiuyang.me> wrote:
> >> This patch implements flush_cache_vmap for RISC-V, since it modifies PTE.
> >> Without this patch, SFENCE.VMA won't be added to related codes, which
> >> might introduce a bug in the out-of-order micro-architecture
> >> implementations.
> >> 
> >> Signed-off-by: Jiuyang Liu <liu@jiuyang.me>
> >> Reviewed-by: Alexandre Ghiti <alex@ghiti.fr>
> >> Reviewed-by: Palmer Dabbelt <palmer@dabbelt.com>
> > 
> > IIRC, Palmer hasn't given this Reviewed-by tag.
> > 
> >> ---
> > 
> > Could you plz add version and changes? IIRC, this is the v3.
> > 
> >>   arch/riscv/include/asm/cacheflush.h | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/arch/riscv/include/asm/cacheflush.h
> >> b/arch/riscv/include/asm/cacheflush.h index 23ff70350992..3fd528badc35
> >> 100644
> >> --- a/arch/riscv/include/asm/cacheflush.h
> >> +++ b/arch/riscv/include/asm/cacheflush.h
> >> @@ -30,6 +30,12 @@ static inline void flush_dcache_page(struct page
> >> *page)
> >> 
> >>   #define flush_icache_user_page(vma, pg, addr, len) \
> >>   
> >>          flush_icache_mm(vma->vm_mm, 0)
> >> 
> >> +/*
> >> + * flush_cache_vmap is invoked after map_kernel_range() has installed
> >> the page + * table entries, which modifies PTE, SFENCE.VMA should be
> >> inserted.> 
> > Just my humble opinion, flush_cache_vmap() may not be necessary.
> > vmalloc_fault can take care of this, and finally sfence.vma is inserted
> > in related path.
> I believe Palmer and Jisheng are right, my initial proposal to implement
> flush_cache_vmap is wrong.
> 
> But then, Jiuyang should not have noticed any problem here, so what's
> wrong? @Jiuyang: Does implementing flush_cache_vmap fix your issue?
Yes, I just checked this today, it did fix our issue.
But I'm happy to rework on this, thanks for your help! This week I will try to 
trace wave of our core, and give a more detail email then.
> And regarding flush_cache_vunmap, from Jisheng call stack, it seems also
> not necessary.
> 
> @Jiuyang: Can you tell us more about what you noticed?
> 
> > Regards
> > 
> >> + */
> >> +#define flush_cache_vmap(start, end) flush_tlb_all()
> >> +
> >> 
> >>   #ifndef CONFIG_SMP
> >>   
> >>   #define flush_icache_all() local_flush_icache_all()
> >> 
> >> --
> >> 2.31.1
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks,
Jiuyang