diff mbox series

[2/3] riscv: Fixup PAGE_UP in asm/page.h

Message ID 1621839068-31738-2-git-send-email-guoren@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/3] riscv: Fixup _PAGE_GLOBAL in _PAGE_KERNEL | expand

Commit Message

Guo Ren May 24, 2021, 6:51 a.m. UTC 
From: Guo Ren <guoren@linux.alibaba.com>

Current PAGE_UP implementation is wrong. PAGE_UP(0) should be
0x1000, but current implementation will give out 0.

Although the current PAGE_UP isn't used, it will soon be used in
tlb_flush optimization.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Anup Patel <anup.patel@wdc.com>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig May 25, 2021, 6:34 a.m. UTC | #1 
On Mon, May 24, 2021 at 06:51:07AM +0000, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Current PAGE_UP implementation is wrong. PAGE_UP(0) should be
> 0x1000, but current implementation will give out 0.
> 
> Although the current PAGE_UP isn't used, it will soon be used in
> tlb_flush optimization.

Nak.  Please just remove the PAGE_UP/PAGE_DOWN macros just like they
have been removed from other architectures long ago and use the
generic DIV_ROUND_UP macro for your new code like everyone else.
Guo Ren May 25, 2021, 9:28 a.m. UTC | #2 
On Tue, May 25, 2021 at 2:34 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, May 24, 2021 at 06:51:07AM +0000, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Current PAGE_UP implementation is wrong. PAGE_UP(0) should be
> > 0x1000, but current implementation will give out 0.
> >
> > Although the current PAGE_UP isn't used, it will soon be used in
> > tlb_flush optimization.
>
> Nak.  Please just remove the PAGE_UP/PAGE_DOWN macros just like they
> have been removed from other architectures long ago and use the
> generic DIV_ROUND_UP macro for your new code like everyone else.

This patch has been dropped because it's wrong, ref Anup's reply.

Remove PAGE_UP/DOWN is okay for me. How about:
 static inline void local_flush_tlb_range_asid(unsigned long start,
unsigned long size,
                                              unsigned long asid)
 {
-       unsigned long page_add = PAGE_DOWN(start);
-       unsigned long page_end = PAGE_UP(start + size);
+       unsigned long page_add = _ALIGN_DOWN(start, PAGE_SIZE);
+       unsigned long page_end = _ALIGN_UP(start + size, PAGE_SIZE);

_ALIGN_XXX are also defined in arch/riscv/include/asm/page.h.
Christoph Hellwig May 25, 2021, 9:34 a.m. UTC | #3 
On Tue, May 25, 2021 at 05:28:21PM +0800, Guo Ren wrote:
> On Tue, May 25, 2021 at 2:34 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, May 24, 2021 at 06:51:07AM +0000, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Current PAGE_UP implementation is wrong. PAGE_UP(0) should be
> > > 0x1000, but current implementation will give out 0.
> > >
> > > Although the current PAGE_UP isn't used, it will soon be used in
> > > tlb_flush optimization.
> >
> > Nak.  Please just remove the PAGE_UP/PAGE_DOWN macros just like they
> > have been removed from other architectures long ago and use the
> > generic DIV_ROUND_UP macro for your new code like everyone else.
> 
> This patch has been dropped because it's wrong, ref Anup's reply.
> 
> Remove PAGE_UP/DOWN is okay for me. How about:
>  static inline void local_flush_tlb_range_asid(unsigned long start,
> unsigned long size,
>                                               unsigned long asid)
>  {
> -       unsigned long page_add = PAGE_DOWN(start);
> -       unsigned long page_end = PAGE_UP(start + size);
> +       unsigned long page_add = _ALIGN_DOWN(start, PAGE_SIZE);
> +       unsigned long page_end = _ALIGN_UP(start + size, PAGE_SIZE);
> 
> _ALIGN_XXX are also defined in arch/riscv/include/asm/page.h.

And these also are leftovers from days gone by and should be removed.

I think this should simply be:

	
	unsigned long page_add = start & PAGE_MASK;
	unsigned long page_end = PAGE_ALIGN(start + size);

(and page_add is a pretty horrible name as well..)
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index 6a7761c..c611b20 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -37,7 +37,7 @@ 
 
 #ifndef __ASSEMBLY__
 
-#define PAGE_UP(addr)	(((addr)+((PAGE_SIZE)-1))&(~((PAGE_SIZE)-1)))
+#define PAGE_UP(addr)	(((addr)+(PAGE_SIZE))&(~((PAGE_SIZE)-1)))
 #define PAGE_DOWN(addr)	((addr)&(~((PAGE_SIZE)-1)))
 
 /* align addr on a size boundary - adjust address up/down if needed */