From: Palmer Dabbelt <palmer@dabbelt.com> To: alex@ghiti.fr Cc: Jisheng.Zhang@synaptics.com, liu@jiuyang.me, waterman@eecs.berkeley.edu, Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu, akpm@linux-foundation.org, 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: Thu, 22 Apr 2021 21:39:24 -0700 (PDT) [thread overview] Message-ID: <mhng-12e52134-80b2-409c-bf30-1300875c54a2@palmerdabbelt-glaptop> (raw) In-Reply-To: <18d198ac-7bc0-934d-e1e9-eca01b790d61@ghiti.fr> On Wed, 14 Apr 2021 00:03:13 PDT (-0700), alex@ghiti.fr 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. Yes. In fact, I gave the opposite of a RB: we shouldn't have this, at least without some demonstration of a meaningful performance improvement and likely with a host of other changes to change the whole port over to avoid relying on traps to handle new mappings. I really don't think that's a sane way to go, as the theory is that reasonable microarchitectures won't have big windows over which these faults can occur so there won't be that many of them. If it ends up being an issue on real hardware we can try and sort something out, but it's going to be a lot of work as we'll need to avoid hurting performance on implementations that don't make invalid mappings visible often. >> >>> --- >> >> 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? > > And regarding flush_cache_vunmap, from Jisheng call stack, it seems also > not necessary. FWIW: I still think that flush_cache_vunmap() is necessary -- we don't have any other way to guarantee that mapping isn't visible. Implementing flush_cache_vmap() could work around the real bug of lacking flush_cache_vunmap(), as we'd see stale mappings. That said, it could just be covering up some other bug. Wouldn't be surprised if it's a bug in our port, but this is the sort of thing that could also be a hardware bug of some sort. > > @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 >>
WARNING: multiple messages have this Message-ID (diff)
From: Palmer Dabbelt <palmer@dabbelt.com> To: alex@ghiti.fr Cc: Jisheng.Zhang@synaptics.com, liu@jiuyang.me, waterman@eecs.berkeley.edu, Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu, akpm@linux-foundation.org, 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: Thu, 22 Apr 2021 21:39:24 -0700 (PDT) [thread overview] Message-ID: <mhng-12e52134-80b2-409c-bf30-1300875c54a2@palmerdabbelt-glaptop> (raw) In-Reply-To: <18d198ac-7bc0-934d-e1e9-eca01b790d61@ghiti.fr> On Wed, 14 Apr 2021 00:03:13 PDT (-0700), alex@ghiti.fr 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. Yes. In fact, I gave the opposite of a RB: we shouldn't have this, at least without some demonstration of a meaningful performance improvement and likely with a host of other changes to change the whole port over to avoid relying on traps to handle new mappings. I really don't think that's a sane way to go, as the theory is that reasonable microarchitectures won't have big windows over which these faults can occur so there won't be that many of them. If it ends up being an issue on real hardware we can try and sort something out, but it's going to be a lot of work as we'll need to avoid hurting performance on implementations that don't make invalid mappings visible often. >> >>> --- >> >> 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? > > And regarding flush_cache_vunmap, from Jisheng call stack, it seems also > not necessary. FWIW: I still think that flush_cache_vunmap() is necessary -- we don't have any other way to guarantee that mapping isn't visible. Implementing flush_cache_vmap() could work around the real bug of lacking flush_cache_vunmap(), as we'd see stale mappings. That said, it could just be covering up some other bug. Wouldn't be surprised if it's a bug in our port, but this is the sort of thing that could also be a hardware bug of some sort. > > @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 >> _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-04-23 4:39 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-12 0:05 [PATCH] implement flush_cache_vmap for RISC-V Jiuyang Liu 2021-04-12 0:05 ` Jiuyang Liu 2021-04-12 7:08 ` Jisheng Zhang 2021-04-12 7:08 ` Jisheng Zhang 2021-04-14 7:03 ` Alex Ghiti 2021-04-14 7:03 ` Alex Ghiti 2021-04-14 9:16 ` Jiuyang Liu 2021-04-14 9:16 ` Jiuyang Liu 2021-04-23 4:39 ` Palmer Dabbelt [this message] 2021-04-23 4:39 ` Palmer Dabbelt
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=mhng-12e52134-80b2-409c-bf30-1300875c54a2@palmerdabbelt-glaptop \ --to=palmer@dabbelt.com \ --cc=Jisheng.Zhang@synaptics.com \ --cc=akpm@linux-foundation.org \ --cc=alex@ghiti.fr \ --cc=aou@eecs.berkeley.edu \ --cc=geert@linux-m68k.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=liu@jiuyang.me \ --cc=paul.walmsley@sifive.com \ --cc=waterman@eecs.berkeley.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.