| Message ID | 20190813170149.26037-4-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix issues with 52-bit kernel virtual addressing | expand |
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 >
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>
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
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.
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
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.
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) ({ \
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(-)