| Message ID | 20210421065108.1987-4-rppt@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: drop pfn_valid_within() and simplify pfn_valid() | expand |
On 4/21/21 12:21 PM, Mike Rapoport wrote: > From: Mike Rapoport <rppt@linux.ibm.com> > > The intended semantics of pfn_valid() is to verify whether there is a > struct page for the pfn in question and nothing else. > > Yet, on arm64 it is used to distinguish memory areas that are mapped in the > linear map vs those that require ioremap() to access them. > > Introduce a dedicated pfn_is_map_memory() wrapper for > memblock_is_map_memory() to perform such check and use it where > appropriate. > > Using a wrapper allows to avoid cyclic include dependencies. > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > --- > arch/arm64/include/asm/memory.h | 2 +- > arch/arm64/include/asm/page.h | 1 + > arch/arm64/kvm/mmu.c | 2 +- > arch/arm64/mm/init.c | 11 +++++++++++ > arch/arm64/mm/ioremap.c | 4 ++-- > arch/arm64/mm/mmu.c | 2 +- > 6 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > index 0aabc3be9a75..194f9f993d30 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x) > > #define virt_addr_valid(addr) ({ \ > __typeof__(addr) __addr = __tag_reset(addr); \ > - __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > + __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \ > }) > > void dump_mem_limit(void); > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > index 012cffc574e8..99a6da91f870 100644 > --- a/arch/arm64/include/asm/page.h > +++ b/arch/arm64/include/asm/page.h > @@ -38,6 +38,7 @@ void copy_highpage(struct page *to, struct page *from); > typedef struct page *pgtable_t; > > extern int pfn_valid(unsigned long); > +extern int pfn_is_map_memory(unsigned long); Check patch is complaining about this. WARNING: function definition argument 'unsigned long' should also have an identifier name #50: FILE: arch/arm64/include/asm/page.h:41: +extern int pfn_is_map_memory(unsigned long); > > #include <asm/memory.h> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 8711894db8c2..23dd99e29b23 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -85,7 +85,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm) > > static bool kvm_is_device_pfn(unsigned long pfn) > { > - return !pfn_valid(pfn); > + return !pfn_is_map_memory(pfn); > } > > /* > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 3685e12aba9b..dc03bdc12c0f 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -258,6 +258,17 @@ int pfn_valid(unsigned long pfn) > } > EXPORT_SYMBOL(pfn_valid); > > +int pfn_is_map_memory(unsigned long pfn) > +{ > + phys_addr_t addr = PFN_PHYS(pfn); > + Should also bring with it, the comment regarding upper bits in the pfn from arm64 pfn_valid(). > + if (PHYS_PFN(addr) != pfn) > + return 0; > + ^^^^^ trailing spaces here. ERROR: trailing whitespace #81: FILE: arch/arm64/mm/init.c:263: +^I$ > + return memblock_is_map_memory(addr); > +} > +EXPORT_SYMBOL(pfn_is_map_memory); > + Is the EXPORT_SYMBOL() required to build drivers which will use pfn_is_map_memory() but currently use pfn_valid() ?
On Wed, Apr 21, 2021 at 04:29:48PM +0530, Anshuman Khandual wrote: > > On 4/21/21 12:21 PM, Mike Rapoport wrote: > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > The intended semantics of pfn_valid() is to verify whether there is a > > struct page for the pfn in question and nothing else. > > > > Yet, on arm64 it is used to distinguish memory areas that are mapped in the > > linear map vs those that require ioremap() to access them. > > > > Introduce a dedicated pfn_is_map_memory() wrapper for > > memblock_is_map_memory() to perform such check and use it where > > appropriate. > > > > Using a wrapper allows to avoid cyclic include dependencies. > > > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > > --- > > arch/arm64/include/asm/memory.h | 2 +- > > arch/arm64/include/asm/page.h | 1 + > > arch/arm64/kvm/mmu.c | 2 +- > > arch/arm64/mm/init.c | 11 +++++++++++ > > arch/arm64/mm/ioremap.c | 4 ++-- > > arch/arm64/mm/mmu.c | 2 +- > > 6 files changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > index 0aabc3be9a75..194f9f993d30 100644 > > --- a/arch/arm64/include/asm/memory.h > > +++ b/arch/arm64/include/asm/memory.h > > @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x) > > > > #define virt_addr_valid(addr) ({ \ > > __typeof__(addr) __addr = __tag_reset(addr); \ > > - __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > > + __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \ > > }) > > > > void dump_mem_limit(void); > > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > > index 012cffc574e8..99a6da91f870 100644 > > --- a/arch/arm64/include/asm/page.h > > +++ b/arch/arm64/include/asm/page.h > > @@ -38,6 +38,7 @@ void copy_highpage(struct page *to, struct page *from); > > typedef struct page *pgtable_t; > > > > extern int pfn_valid(unsigned long); > > +extern int pfn_is_map_memory(unsigned long); > > Check patch is complaining about this. > > WARNING: function definition argument 'unsigned long' should also have an identifier name > #50: FILE: arch/arm64/include/asm/page.h:41: > +extern int pfn_is_map_memory(unsigned long); > > > > > > #include <asm/memory.h> > > > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > > index 8711894db8c2..23dd99e29b23 100644 > > --- a/arch/arm64/kvm/mmu.c > > +++ b/arch/arm64/kvm/mmu.c > > @@ -85,7 +85,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm) > > > > static bool kvm_is_device_pfn(unsigned long pfn) > > { > > - return !pfn_valid(pfn); > > + return !pfn_is_map_memory(pfn); > > } > > > > /* > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > > index 3685e12aba9b..dc03bdc12c0f 100644 > > --- a/arch/arm64/mm/init.c > > +++ b/arch/arm64/mm/init.c > > @@ -258,6 +258,17 @@ int pfn_valid(unsigned long pfn) > > } > > EXPORT_SYMBOL(pfn_valid); > > > > +int pfn_is_map_memory(unsigned long pfn) > > +{ > > + phys_addr_t addr = PFN_PHYS(pfn); > > + > > Should also bring with it, the comment regarding upper bits in > the pfn from arm64 pfn_valid(). I think a reference to the comment in pfn_valid() will suffice. BTW, I wonder how is that other architectures do not need this check? > > + if (PHYS_PFN(addr) != pfn) > > + return 0; > > + > > ^^^^^ trailing spaces here. > > ERROR: trailing whitespace > #81: FILE: arch/arm64/mm/init.c:263: > +^I$ Oops :) > > + return memblock_is_map_memory(addr); > > +} > > +EXPORT_SYMBOL(pfn_is_map_memory); > > + > > Is the EXPORT_SYMBOL() required to build drivers which will use > pfn_is_map_memory() but currently use pfn_valid() ? Yes, this is required for virt_addr_valid() that is used by modules.
On 4/21/21 5:49 PM, Mike Rapoport wrote: > On Wed, Apr 21, 2021 at 04:29:48PM +0530, Anshuman Khandual wrote: >> >> On 4/21/21 12:21 PM, Mike Rapoport wrote: >>> From: Mike Rapoport <rppt@linux.ibm.com> >>> >>> The intended semantics of pfn_valid() is to verify whether there is a >>> struct page for the pfn in question and nothing else. >>> >>> Yet, on arm64 it is used to distinguish memory areas that are mapped in the >>> linear map vs those that require ioremap() to access them. >>> >>> Introduce a dedicated pfn_is_map_memory() wrapper for >>> memblock_is_map_memory() to perform such check and use it where >>> appropriate. >>> >>> Using a wrapper allows to avoid cyclic include dependencies. >>> >>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> >>> --- >>> arch/arm64/include/asm/memory.h | 2 +- >>> arch/arm64/include/asm/page.h | 1 + >>> arch/arm64/kvm/mmu.c | 2 +- >>> arch/arm64/mm/init.c | 11 +++++++++++ >>> arch/arm64/mm/ioremap.c | 4 ++-- >>> arch/arm64/mm/mmu.c | 2 +- >>> 6 files changed, 17 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h >>> index 0aabc3be9a75..194f9f993d30 100644 >>> --- a/arch/arm64/include/asm/memory.h >>> +++ b/arch/arm64/include/asm/memory.h >>> @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x) >>> >>> #define virt_addr_valid(addr) ({ \ >>> __typeof__(addr) __addr = __tag_reset(addr); \ >>> - __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ >>> + __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \ >>> }) >>> >>> void dump_mem_limit(void); >>> diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h >>> index 012cffc574e8..99a6da91f870 100644 >>> --- a/arch/arm64/include/asm/page.h >>> +++ b/arch/arm64/include/asm/page.h >>> @@ -38,6 +38,7 @@ void copy_highpage(struct page *to, struct page *from); >>> typedef struct page *pgtable_t; >>> >>> extern int pfn_valid(unsigned long); >>> +extern int pfn_is_map_memory(unsigned long); >> >> Check patch is complaining about this. >> >> WARNING: function definition argument 'unsigned long' should also have an identifier name >> #50: FILE: arch/arm64/include/asm/page.h:41: >> +extern int pfn_is_map_memory(unsigned long); >> >> >>> >>> #include <asm/memory.h> >>> >>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c >>> index 8711894db8c2..23dd99e29b23 100644 >>> --- a/arch/arm64/kvm/mmu.c >>> +++ b/arch/arm64/kvm/mmu.c >>> @@ -85,7 +85,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm) >>> >>> static bool kvm_is_device_pfn(unsigned long pfn) >>> { >>> - return !pfn_valid(pfn); >>> + return !pfn_is_map_memory(pfn); >>> } >>> >>> /* >>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>> index 3685e12aba9b..dc03bdc12c0f 100644 >>> --- a/arch/arm64/mm/init.c >>> +++ b/arch/arm64/mm/init.c >>> @@ -258,6 +258,17 @@ int pfn_valid(unsigned long pfn) >>> } >>> EXPORT_SYMBOL(pfn_valid); >>> >>> +int pfn_is_map_memory(unsigned long pfn) >>> +{ >>> + phys_addr_t addr = PFN_PHYS(pfn); >>> + >> >> Should also bring with it, the comment regarding upper bits in >> the pfn from arm64 pfn_valid(). > > I think a reference to the comment in pfn_valid() will suffice. Okay. > > BTW, I wonder how is that other architectures do not need this check? Trying to move that into generic pfn_valid() in mmzone.h, will resend the RFC patch after this series. https://patchwork.kernel.org/project/linux-mm/patch/1615174073-10520-1-git-send-email-anshuman.khandual@arm.com/ > >>> + if (PHYS_PFN(addr) != pfn) >>> + return 0; >>> + >> >> ^^^^^ trailing spaces here. >> >> ERROR: trailing whitespace >> #81: FILE: arch/arm64/mm/init.c:263: >> +^I$ > > Oops :) > >>> + return memblock_is_map_memory(addr); >>> +} >>> +EXPORT_SYMBOL(pfn_is_map_memory); >>> + >> >> Is the EXPORT_SYMBOL() required to build drivers which will use >> pfn_is_map_memory() but currently use pfn_valid() ? > > Yes, this is required for virt_addr_valid() that is used by modules. > There will be two adjacent EXPORT_SYMBOLS(), one for pfn_valid() and one for pfn_is_map_memory(). But its okay I guess, cant help it.
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index 0aabc3be9a75..194f9f993d30 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -351,7 +351,7 @@ static inline void *phys_to_virt(phys_addr_t x) #define virt_addr_valid(addr) ({ \ __typeof__(addr) __addr = __tag_reset(addr); \ - __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ + __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr)); \ }) void dump_mem_limit(void); diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h index 012cffc574e8..99a6da91f870 100644 --- a/arch/arm64/include/asm/page.h +++ b/arch/arm64/include/asm/page.h @@ -38,6 +38,7 @@ void copy_highpage(struct page *to, struct page *from); typedef struct page *pgtable_t; extern int pfn_valid(unsigned long); +extern int pfn_is_map_memory(unsigned long); #include <asm/memory.h> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 8711894db8c2..23dd99e29b23 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -85,7 +85,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm) static bool kvm_is_device_pfn(unsigned long pfn) { - return !pfn_valid(pfn); + return !pfn_is_map_memory(pfn); } /* diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 3685e12aba9b..dc03bdc12c0f 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -258,6 +258,17 @@ int pfn_valid(unsigned long pfn) } EXPORT_SYMBOL(pfn_valid); +int pfn_is_map_memory(unsigned long pfn) +{ + phys_addr_t addr = PFN_PHYS(pfn); + + if (PHYS_PFN(addr) != pfn) + return 0; + + return memblock_is_map_memory(addr); +} +EXPORT_SYMBOL(pfn_is_map_memory); + static phys_addr_t memory_limit = PHYS_ADDR_MAX; /* diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index b5e83c46b23e..b7c81dacabf0 100644 --- a/arch/arm64/mm/ioremap.c +++ b/arch/arm64/mm/ioremap.c @@ -43,7 +43,7 @@ static void __iomem *__ioremap_caller(phys_addr_t phys_addr, size_t size, /* * Don't allow RAM to be mapped. */ - if (WARN_ON(pfn_valid(__phys_to_pfn(phys_addr)))) + if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) return NULL; area = get_vm_area_caller(size, VM_IOREMAP, caller); @@ -84,7 +84,7 @@ EXPORT_SYMBOL(iounmap); void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size) { /* For normal memory we already have a cacheable mapping. */ - if (pfn_valid(__phys_to_pfn(phys_addr))) + if (pfn_is_map_memory(__phys_to_pfn(phys_addr))) return (void __iomem *)__phys_to_virt(phys_addr); return __ioremap_caller(phys_addr, size, __pgprot(PROT_NORMAL), diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 5d9550fdb9cf..26045e9adbd7 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -81,7 +81,7 @@ void set_swapper_pgd(pgd_t *pgdp, pgd_t pgd) pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, unsigned long size, pgprot_t vma_prot) { - if (!pfn_valid(pfn)) + if (!pfn_is_map_memory(pfn)) return pgprot_noncached(vma_prot); else if (file->f_flags & O_SYNC) return pgprot_writecombine(vma_prot);