| Message ID | 1562883681-18659-1-git-send-email-karahmed@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: Extend the check for RAM in /dev/mem | expand |
On 07/12/2019 03:51 AM, KarimAllah Ahmed wrote: > Some valid RAM can live outside kernel control (e.g. using mem= kernel > command-line). For these regions, pfn_valid would return "false" causing > system RAM to be mapped as uncached. Use memblock instead to identify RAM. Once the remaining memory is outside of the kernel (as the admin would have intended with mem= command line) what is the particular concern regarding the way those get mapped (cached or not) ? It is not to be used any way. > > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mike Rapoport <rppt@linux.ibm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Anders Roxell <anders.roxell@linaro.org> > Cc: Enrico Weigelt <info@metux.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: KarimAllah Ahmed <karahmed@amazon.de> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > Cc: Jun Yao <yaojun8558363@gmail.com> > Cc: Yu Zhao <yuzhao@google.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de> > --- > arch/arm/mm/mmu.c | 2 +- > arch/arm64/mm/mmu.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c > index 1aa2586..492774b 100644 > --- a/arch/arm/mm/mmu.c > +++ b/arch/arm/mm/mmu.c > @@ -705,7 +705,7 @@ static void __init build_mem_type_table(void) > pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, > unsigned long size, pgprot_t vma_prot) > { > - if (!pfn_valid(pfn)) > + if (!memblock_is_memory(__pfn_to_phys(pfn))) > return pgprot_noncached(vma_prot); > else if (file->f_flags & O_SYNC) > return pgprot_writecombine(vma_prot); > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 3645f29..cdc3e8e 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) pfn_valid() on arm64 checks if the memblock region is mapped i.e does it have a linear mapping or not. If a segment of RAM is outside linear mapping due to mem= directive and lacks a linear mapping then why should it be mapped similarly like system RAM on this path ?
On Fri, 2019-07-12 at 08:06 +0530, Anshuman Khandual wrote: > > On 07/12/2019 03:51 AM, KarimAllah Ahmed wrote: > > > > Some valid RAM can live outside kernel control (e.g. using mem= kernel > > command-line). For these regions, pfn_valid would return "false" causing > > system RAM to be mapped as uncached. Use memblock instead to identify RAM. > > Once the remaining memory is outside of the kernel (as the admin would have > intended with mem= command line) what is the particular concern regarding > the way those get mapped (cached or not) ? It is not to be used any way. They can be used by user-space which might lead to them being used by the kernel. One use-case would be using them as guest memory for KVM as I detailed here: https://lwn.net/Articles/778240/ > > > > > > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Mike Rapoport <rppt@linux.ibm.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Anders Roxell <anders.roxell@linaro.org> > > Cc: Enrico Weigelt <info@metux.net> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: KarimAllah Ahmed <karahmed@amazon.de> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: James Morse <james.morse@arm.com> > > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > > Cc: Jun Yao <yaojun8558363@gmail.com> > > Cc: Yu Zhao <yuzhao@google.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de> > > --- > > arch/arm/mm/mmu.c | 2 +- > > arch/arm64/mm/mmu.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c > > index 1aa2586..492774b 100644 > > --- a/arch/arm/mm/mmu.c > > +++ b/arch/arm/mm/mmu.c > > @@ -705,7 +705,7 @@ static void __init build_mem_type_table(void) > > pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, > > unsigned long size, pgprot_t vma_prot) > > { > > - if (!pfn_valid(pfn)) > > + if (!memblock_is_memory(__pfn_to_phys(pfn))) > > return pgprot_noncached(vma_prot); > > else if (file->f_flags & O_SYNC) > > return pgprot_writecombine(vma_prot); > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > index 3645f29..cdc3e8e 100644 > > --- a/arch/arm64/mm/mmu.c > > +++ b/arch/arm64/mm/mmu.c > > @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) > > pfn_valid() on arm64 checks if the memblock region is mapped i.e does it have > a linear mapping or not. If a segment of RAM is outside linear mapping due to > mem= directive and lacks a linear mapping then why should it be mapped similarly > like system RAM on this path ? I actually struggled a bit here because there is really no *explicit* documentation of what is the expected behavior here, so for me it was open to interpretation. It seems like for you the deciding factor between cached and uncached is the existence of linear mapping. However, for me the deciding factor is whether it is RAM or not. I choose this interpretation because it helps in the KVM scenario that I mentioned above :) Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Fri, Jul 12, 2019 at 02:58:18AM +0000, Raslan, KarimAllah wrote: > On Fri, 2019-07-12 at 08:06 +0530, Anshuman Khandual wrote: > > > > On 07/12/2019 03:51 AM, KarimAllah Ahmed wrote: > > > > > > Some valid RAM can live outside kernel control (e.g. using mem= kernel > > > command-line). For these regions, pfn_valid would return "false" causing > > > system RAM to be mapped as uncached. Use memblock instead to identify RAM. > > > > Once the remaining memory is outside of the kernel (as the admin would have > > intended with mem= command line) what is the particular concern regarding > > the way those get mapped (cached or not) ? It is not to be used any way. > > They can be used by user-space which might lead to them being used by the > kernel. One use-case would be using them as guest memory for KVM as I detailed > here: > > https://lwn.net/Articles/778240/ From the 32-bit ARM point of view... What if someone's already doing something similar with a non-coherent DSP and is relying on the current behaviour? This change is a user visible behavioural change that could end up breaking userspace. In other words, it isn't something we should rush into.
On Fri, 2019-07-12 at 09:56 +0100, Russell King - ARM Linux admin wrote: > On Fri, Jul 12, 2019 at 02:58:18AM +0000, Raslan, KarimAllah wrote: > > > > On Fri, 2019-07-12 at 08:06 +0530, Anshuman Khandual wrote: > > > > > > > > > On 07/12/2019 03:51 AM, KarimAllah Ahmed wrote: > > > > > > > > > > > > Some valid RAM can live outside kernel control (e.g. using mem= kernel > > > > command-line). For these regions, pfn_valid would return "false" causing > > > > system RAM to be mapped as uncached. Use memblock instead to identify RAM. > > > > > > Once the remaining memory is outside of the kernel (as the admin would have > > > intended with mem= command line) what is the particular concern regarding > > > the way those get mapped (cached or not) ? It is not to be used any way. > > > > They can be used by user-space which might lead to them being used by the > > kernel. One use-case would be using them as guest memory for KVM as I detailed > > here: > > > > https://lwn.net/Articles/778240/ > > From the 32-bit ARM point of view... > > What if someone's already doing something similar with a non-coherent > DSP and is relying on the current behaviour? This change is a user > visible behavioural change that could end up breaking userspace. > > In other words, it isn't something we should rush into. Yes, that makes sense. How about adding a command-line option for this new behavior instead? Would this be more reasonable? Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Fri, Jul 12, 2019 at 12:21:21AM +0200, KarimAllah Ahmed wrote: > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 3645f29..cdc3e8e 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) This looks broken to me, since it will end up returning 'true' for nomap memory and we really don't want to map that using writeback attributes. Will
On Fri, 2019-07-12 at 15:57 +0100, Will Deacon wrote: > On Fri, Jul 12, 2019 at 12:21:21AM +0200, KarimAllah Ahmed wrote: > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > index 3645f29..cdc3e8e 100644 > > --- a/arch/arm64/mm/mmu.c > > +++ b/arch/arm64/mm/mmu.c > > @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) > > This looks broken to me, since it will end up returning 'true' for nomap > memory and we really don't want to map that using writeback attributes. True, I will fix this by using memblock_is_map_memory instead. That said, do you have any concerns about this approach in general? > > Will Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Fri, Jul 12, 2019 at 03:13:38PM +0000, Raslan, KarimAllah wrote: > On Fri, 2019-07-12 at 15:57 +0100, Will Deacon wrote: > > On Fri, Jul 12, 2019 at 12:21:21AM +0200, KarimAllah Ahmed wrote: > > > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > > index 3645f29..cdc3e8e 100644 > > > --- a/arch/arm64/mm/mmu.c > > > +++ b/arch/arm64/mm/mmu.c > > > @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) > > > > This looks broken to me, since it will end up returning 'true' for nomap > > memory and we really don't want to map that using writeback attributes. > > True, I will fix this by using memblock_is_map_memory instead. That said, do > you have any concerns about this approach in general? If you do that, I don't understand why you need the patch at all given our implementation of pfn_valid() in arch/arm64/mm/init.c. Will
On Fri, 2019-07-12 at 16:34 +0100, Will Deacon wrote: > On Fri, Jul 12, 2019 at 03:13:38PM +0000, Raslan, KarimAllah wrote: > > > > On Fri, 2019-07-12 at 15:57 +0100, Will Deacon wrote: > > > > > > On Fri, Jul 12, 2019 at 12:21:21AM +0200, KarimAllah Ahmed wrote: > > > > > > > > > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > > > index 3645f29..cdc3e8e 100644 > > > > --- a/arch/arm64/mm/mmu.c > > > > +++ b/arch/arm64/mm/mmu.c > > > > @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) > > > > > > This looks broken to me, since it will end up returning 'true' for nomap > > > memory and we really don't want to map that using writeback attributes. > > > > True, I will fix this by using memblock_is_map_memory instead. That said, do > > you have any concerns about this approach in general? > > If you do that, I don't understand why you need the patch at all given our > implementation of pfn_valid() in arch/arm64/mm/init.c. Oops! Right, I guess that would not work either. Let me dig into a better way to do that. > > Will Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 1aa2586..492774b 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -705,7 +705,7 @@ static void __init build_mem_type_table(void) pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, unsigned long size, pgprot_t vma_prot) { - if (!pfn_valid(pfn)) + if (!memblock_is_memory(__pfn_to_phys(pfn))) return pgprot_noncached(vma_prot); else if (file->f_flags & O_SYNC) return pgprot_writecombine(vma_prot); diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 3645f29..cdc3e8e 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -78,7 +78,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 (!memblock_is_memory(__pfn_to_phys(pfn))) return pgprot_noncached(vma_prot); else if (file->f_flags & O_SYNC) return pgprot_writecombine(vma_prot);
Some valid RAM can live outside kernel control (e.g. using mem= kernel command-line). For these regions, pfn_valid would return "false" causing system RAM to be mapped as uncached. Use memblock instead to identify RAM. Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mike Rapoport <rppt@linux.ibm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Anders Roxell <anders.roxell@linaro.org> Cc: Enrico Weigelt <info@metux.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: KarimAllah Ahmed <karahmed@amazon.de> Cc: Mark Rutland <mark.rutland@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Anshuman Khandual <anshuman.khandual@arm.com> Cc: Jun Yao <yaojun8558363@gmail.com> Cc: Yu Zhao <yuzhao@google.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de> --- arch/arm/mm/mmu.c | 2 +- arch/arm64/mm/mmu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)