[3/8] arm64: memory: Rewrite default page_to_virt()/virt_to_page()
diff mbox series

Message ID 20190813170149.26037-4-will@kernel.org
State New
Headers show
Series
  • Fix issues with 52-bit kernel virtual addressing
Related show

Commit Message

Will Deacon Aug. 13, 2019, 5:01 p.m. UTC
The default implementations of page_to_virt() and virt_to_page() are
fairly confusing to read and the former evaluates its 'page' parameter
twice in the macro

Rewrite them so that the computation is expressed as 'base + index' in
both cases and the parameter is always evaluated exactly once.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/memory.h | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Steve Capper Aug. 13, 2019, 6:54 p.m. UTC | #1
On Tue, Aug 13, 2019 at 06:01:44PM +0100, Will Deacon wrote:
> The default implementations of page_to_virt() and virt_to_page() are
> fairly confusing to read and the former evaluates its 'page' parameter
> twice in the macro
> 
> Rewrite them so that the computation is expressed as 'base + index' in
> both cases and the parameter is always evaluated exactly once.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

Reviewed-by: Steve Capper <steve.capper@arm.com>

> ---
>  arch/arm64/include/asm/memory.h | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 47b4dc73b8bf..77074b3a1025 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -313,19 +313,18 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
>  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>  #else
> -#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
> -#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> -
> -#define page_to_virt(page)	({					\
> -	unsigned long __addr =						\
> -		((__page_to_voff(page)) + PAGE_OFFSET);			\
> -	const void *__addr_tag =					\
> -		__tag_set((void *)__addr, page_kasan_tag(page));	\
> -	((void *)__addr_tag);						\
> +#define page_to_virt(x)	({						\
> +	__typeof__(x) __page = x;					\
> +	u64 __idx = ((u64)__page - VMEMMAP_START) / sizeof(struct page);\
> +	u64 __addr = PAGE_OFFSET + (__idx * PAGE_SIZE);			\
> +	(void *)__tag_set((const void *)__addr, page_kasan_tag(__page));\
>  })
>  
> -#define virt_to_page(vaddr)	\
> -	((struct page *)((__virt_to_pgoff(__tag_reset(vaddr))) + VMEMMAP_START))
> +#define virt_to_page(x)	({						\
> +	u64 __idx = (__tag_reset((u64)x) - PAGE_OFFSET) / PAGE_SIZE;	\
> +	u64 __addr = VMEMMAP_START + (__idx * sizeof(struct page));	\
> +	(struct page *)__addr;						\
> +})
>  #endif
>  
>  #define virt_addr_valid(addr)	({					\
> -- 
> 2.11.0
>
Catalin Marinas Aug. 14, 2019, 9:30 a.m. UTC | #2
On Tue, Aug 13, 2019 at 06:01:44PM +0100, Will Deacon wrote:
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 47b4dc73b8bf..77074b3a1025 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -313,19 +313,18 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
>  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>  #else
> -#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
> -#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> -
> -#define page_to_virt(page)	({					\
> -	unsigned long __addr =						\
> -		((__page_to_voff(page)) + PAGE_OFFSET);			\
> -	const void *__addr_tag =					\
> -		__tag_set((void *)__addr, page_kasan_tag(page));	\
> -	((void *)__addr_tag);						\
> +#define page_to_virt(x)	({						\
> +	__typeof__(x) __page = x;					\

Why not struct page * directly here?

Either way:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Will Deacon Aug. 14, 2019, 9:41 a.m. UTC | #3
On Wed, Aug 14, 2019 at 10:30:19AM +0100, Catalin Marinas wrote:
> On Tue, Aug 13, 2019 at 06:01:44PM +0100, Will Deacon wrote:
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index 47b4dc73b8bf..77074b3a1025 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -313,19 +313,18 @@ static inline void *phys_to_virt(phys_addr_t x)
> >  #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
> >  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> >  #else
> > -#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
> > -#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> > -
> > -#define page_to_virt(page)	({					\
> > -	unsigned long __addr =						\
> > -		((__page_to_voff(page)) + PAGE_OFFSET);			\
> > -	const void *__addr_tag =					\
> > -		__tag_set((void *)__addr, page_kasan_tag(page));	\
> > -	((void *)__addr_tag);						\
> > +#define page_to_virt(x)	({						\
> > +	__typeof__(x) __page = x;					\
> 
> Why not struct page * directly here?

I started out with that, but then you have to deal with const struct page *
as well and it gets pretty messy.

> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks,

Will
Mark Rutland Aug. 14, 2019, 10:56 a.m. UTC | #4
On Wed, Aug 14, 2019 at 10:41:19AM +0100, Will Deacon wrote:
> On Wed, Aug 14, 2019 at 10:30:19AM +0100, Catalin Marinas wrote:
> > On Tue, Aug 13, 2019 at 06:01:44PM +0100, Will Deacon wrote:
> > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > index 47b4dc73b8bf..77074b3a1025 100644
> > > --- a/arch/arm64/include/asm/memory.h
> > > +++ b/arch/arm64/include/asm/memory.h
> > > @@ -313,19 +313,18 @@ static inline void *phys_to_virt(phys_addr_t x)
> > >  #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
> > >  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> > >  #else
> > > -#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
> > > -#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> > > -
> > > -#define page_to_virt(page)	({					\
> > > -	unsigned long __addr =						\
> > > -		((__page_to_voff(page)) + PAGE_OFFSET);			\
> > > -	const void *__addr_tag =					\
> > > -		__tag_set((void *)__addr, page_kasan_tag(page));	\
> > > -	((void *)__addr_tag);						\
> > > +#define page_to_virt(x)	({						\
> > > +	__typeof__(x) __page = x;					\
> > 
> > Why not struct page * directly here?
> 
> I started out with that, but then you have to deal with const struct page *
> as well and it gets pretty messy.

What goes wrong if you always use const struct page *__page?

Otherwise this looks sound to me.

Thanks,
Mark.
Will Deacon Aug. 14, 2019, 11:17 a.m. UTC | #5
On Wed, Aug 14, 2019 at 11:56:39AM +0100, Mark Rutland wrote:
> On Wed, Aug 14, 2019 at 10:41:19AM +0100, Will Deacon wrote:
> > On Wed, Aug 14, 2019 at 10:30:19AM +0100, Catalin Marinas wrote:
> > > On Tue, Aug 13, 2019 at 06:01:44PM +0100, Will Deacon wrote:
> > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > > index 47b4dc73b8bf..77074b3a1025 100644
> > > > --- a/arch/arm64/include/asm/memory.h
> > > > +++ b/arch/arm64/include/asm/memory.h
> > > > @@ -313,19 +313,18 @@ static inline void *phys_to_virt(phys_addr_t x)
> > > >  #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
> > > >  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> > > >  #else
> > > > -#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
> > > > -#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> > > > -
> > > > -#define page_to_virt(page)	({					\
> > > > -	unsigned long __addr =						\
> > > > -		((__page_to_voff(page)) + PAGE_OFFSET);			\
> > > > -	const void *__addr_tag =					\
> > > > -		__tag_set((void *)__addr, page_kasan_tag(page));	\
> > > > -	((void *)__addr_tag);						\
> > > > +#define page_to_virt(x)	({						\
> > > > +	__typeof__(x) __page = x;					\
> > > 
> > > Why not struct page * directly here?
> > 
> > I started out with that, but then you have to deal with const struct page *
> > as well and it gets pretty messy.
> 
> What goes wrong if you always use const struct page *__page?

It would probably work, but then I wondered about the possibility of
volatile and decided that __typeof__ was cleaner.

Will
Mark Rutland Aug. 14, 2019, 11:26 a.m. UTC | #6
On Wed, Aug 14, 2019 at 12:17:22PM +0100, Will Deacon wrote:
> On Wed, Aug 14, 2019 at 11:56:39AM +0100, Mark Rutland wrote:
> > On Wed, Aug 14, 2019 at 10:41:19AM +0100, Will Deacon wrote:
> > > On Wed, Aug 14, 2019 at 10:30:19AM +0100, Catalin Marinas wrote:
> > > > On Tue, Aug 13, 2019 at 06:01:44PM +0100, Will Deacon wrote:
> > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > > > index 47b4dc73b8bf..77074b3a1025 100644
> > > > > --- a/arch/arm64/include/asm/memory.h
> > > > > +++ b/arch/arm64/include/asm/memory.h
> > > > > @@ -313,19 +313,18 @@ static inline void *phys_to_virt(phys_addr_t x)
> > > > >  #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
> > > > >  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> > > > >  #else
> > > > > -#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
> > > > > -#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> > > > > -
> > > > > -#define page_to_virt(page)	({					\
> > > > > -	unsigned long __addr =						\
> > > > > -		((__page_to_voff(page)) + PAGE_OFFSET);			\
> > > > > -	const void *__addr_tag =					\
> > > > > -		__tag_set((void *)__addr, page_kasan_tag(page));	\
> > > > > -	((void *)__addr_tag);						\
> > > > > +#define page_to_virt(x)	({						\
> > > > > +	__typeof__(x) __page = x;					\
> > > > 
> > > > Why not struct page * directly here?
> > > 
> > > I started out with that, but then you have to deal with const struct page *
> > > as well and it gets pretty messy.
> > 
> > What goes wrong if you always use const struct page *__page?
> 
> It would probably work, but then I wondered about the possibility of
> volatile and decided that __typeof__ was cleaner.

Ok, but I'd suggest that volatile struct page * that's never sane to use
in the first place.

If you don't want to change this, then no worries -- I just can't
follow.

Thanks,
Mark.

Patch
diff mbox series

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 47b4dc73b8bf..77074b3a1025 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -313,19 +313,18 @@  static inline void *phys_to_virt(phys_addr_t x)
 #if !defined(CONFIG_SPARSEMEM_VMEMMAP) || defined(CONFIG_DEBUG_VIRTUAL)
 #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
 #else
-#define __virt_to_pgoff(kaddr)	(((u64)(kaddr) - PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
-#define __page_to_voff(kaddr)	(((u64)(kaddr) - VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
-
-#define page_to_virt(page)	({					\
-	unsigned long __addr =						\
-		((__page_to_voff(page)) + PAGE_OFFSET);			\
-	const void *__addr_tag =					\
-		__tag_set((void *)__addr, page_kasan_tag(page));	\
-	((void *)__addr_tag);						\
+#define page_to_virt(x)	({						\
+	__typeof__(x) __page = x;					\
+	u64 __idx = ((u64)__page - VMEMMAP_START) / sizeof(struct page);\
+	u64 __addr = PAGE_OFFSET + (__idx * PAGE_SIZE);			\
+	(void *)__tag_set((const void *)__addr, page_kasan_tag(__page));\
 })
 
-#define virt_to_page(vaddr)	\
-	((struct page *)((__virt_to_pgoff(__tag_reset(vaddr))) + VMEMMAP_START))
+#define virt_to_page(x)	({						\
+	u64 __idx = (__tag_reset((u64)x) - PAGE_OFFSET) / PAGE_SIZE;	\
+	u64 __addr = VMEMMAP_START + (__idx * sizeof(struct page));	\
+	(struct page *)__addr;						\
+})
 #endif
 
 #define virt_addr_valid(addr)	({					\