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 <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Anders Roxell <[email protected]>
Cc: Enrico Weigelt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: KarimAllah Ahmed <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: James Morse <[email protected]>
Cc: Anshuman Khandual <[email protected]>
Cc: Jun Yao <[email protected]>
Cc: Yu Zhao <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: KarimAllah Ahmed <[email protected]>
---
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
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 <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mike Rapoport <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Anders Roxell <[email protected]>
> Cc: Enrico Weigelt <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: KarimAllah Ahmed <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Anshuman Khandual <[email protected]>
> Cc: Jun Yao <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: KarimAllah Ahmed <[email protected]>
> ---
> 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 <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Mike Rapoport <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Anders Roxell <[email protected]>
> > Cc: Enrico Weigelt <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: KarimAllah Ahmed <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: James Morse <[email protected]>
> > Cc: Anshuman Khandual <[email protected]>
> > Cc: Jun Yao <[email protected]>
> > Cc: Yu Zhao <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: KarimAllah Ahmed <[email protected]>
> > ---
> > 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.
--
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
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