On Thu, Mar 11, 2021 at 09:33:02AM +0000, Will Deacon wrote:
> On Thu, Mar 11, 2021 at 01:22:53PM +0530, Anshuman Khandual wrote:
> > On 3/8/21 2:25 PM, Mike Rapoport wrote:
> > > On Mon, Mar 08, 2021 at 08:57:53AM +0530, Anshuman Khandual wrote:
> > >> Platforms like arm and arm64 have redefined pfn_valid() because their early
> > >> memory sections might have contained memmap holes caused by memblock areas
> > >> tagged with MEMBLOCK_NOMAP, which should be skipped while validating a pfn
> > >> for struct page backing. This scenario could be captured with a new option
> > >> CONFIG_HAVE_EARLY_SECTION_MEMMAP_HOLES and then generic pfn_valid() can be
> > >> improved to accommodate such platforms. This reduces overall code footprint
> > >> and also improves maintainability.
> > >
> > > I wonder whether arm64 would still need to free parts of its memmap after
> >
> > free_unused_memmap() is applicable when CONFIG_SPARSEMEM_VMEMMAP is not enabled.
> > I am not sure whether there still might be some platforms or boards which would
> > benefit from this. Hence lets just keep this unchanged for now.
>
> In my opinion, unless there's a compelling reason for us to offer all of
> these different implementations of the memmap on arm64 then we shouldn't
> bother -- it's not like it's fun to maintain! Just use sparsemem vmemmap
> and be done with it. Is there some reason we can't do that?
Regardless if the decision to stop supporting other memory models, I think
it's a long due for arm64 to stop using pfn_valid() for anything except to
check whether there is a valid struct page for a pfn.
Something like the completely untested patch below:
>From 3a753a56c2d87711f937ba09e4e14e4ad4926c38 Mon Sep 17 00:00:00 2001
From: Mike Rapoport <rppt@xxxxxxxxxxxxx>
Date: Thu, 11 Mar 2021 12:28:29 +0200
Subject: [PATCH] arm64: decouple check whether pfn is normal memory from
pfn_valid()
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_memory() to perform such check and use it
where appropriate.
Signed-off-by: Mike Rapoport <rppt@xxxxxxxxxxxxx>
---
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 | 6 ++++++
arch/arm64/mm/ioremap.c | 4 ++--
arch/arm64/mm/mmu.c | 2 +-
6 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index c759faf7a1ff..778dbfe95d0e 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -346,7 +346,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_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..32b485bcc6ff 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_memory(unsigned long);
#include <asm/memory.h>
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 77cb2d28f2a4..a60069604361 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_memory(pfn);
}
/*
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 0ace5e68efba..77c08853bafc 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -235,6 +235,12 @@ int pfn_valid(unsigned long pfn)
}
EXPORT_SYMBOL(pfn_valid);
+int pfn_is_memory(unsigned long pfn)
+{
+ return memblock_is_map_memory(PFN_PHYS(pfn));
+}
+EXPORT_SYMBOL(pfn_is_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..82a369b22ef5 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_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_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 3802cfbdd20d..ee66f2f21b6f 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_memory(pfn))
return pgprot_noncached(vma_prot);
else if (file->f_flags & O_SYNC)
return pgprot_writecombine(vma_prot);
--
2.28.0
--
Sincerely yours,
Mike.
