[PATCH] arm: Extend the check for RAM in /dev/mem

[PATCH] arm: Extend the check for RAM in /dev/mem

[KarimAllah Ahmed]

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(-)

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);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Anshuman Khandual]

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 ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Raslan, KarimAllah]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Raslan, KarimAllah]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Raslan, KarimAllah]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: Extend the check for RAM in /dev/mem

[Raslan, KarimAllah]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel