diff mbox

ARM: fix bug which lowmem size is limited to 760MB

Message ID alpine.LFD.2.20.1509071152510.22877@knanqh.ubzr
State New
Headers show

Commit Message

Nicolas Pitre Sept. 7, 2015, 7:40 p.m. UTC 
On Mon, 7 Sep 2015, Arnd Bergmann wrote:

> On Thursday 03 September 2015 21:24:00 Nicolas Pitre wrote:
> > If 768MB targets were common place then it could be worth changing the 
> > default vmalloc size to accommodate this memory size and testing all the 
> > other targets to make sure no regressions are introduced.  But given it 
> > is easy to change the default via the kernel cmdline, and that you lose 
> > only 8 MB otherwise, I don't think it is worth the trouble and/or the 
> > risk.
> 
> Agreed.

Well... I think there is a better solution.

Instead of shrinking the vmalloc size, we should instead move 
VMALLOC_END and keep the vmalloc size unchanged.  That also makes for 
nicer address alignment in a subsequent CONFIG_VMSPLIT_3G_OPT patch.

----- >8
ARM: move VMALLOC_END up from 0xff000000 to 0xff800000

There is a 12MB unused region in our memory map between the vmalloc and
fixmap areas. This became unused with commit e9da6e9905e6, confirmed with
commit 64d3b6a3f480.

We also have a 8MB guard area before the vmalloc area.  With the default
240MB vmalloc area size, that puts the end of low memory at 0xef800000
which is unfortunate for 768MB machines where 8MB of RAM is lost to
himem.

Let's move VMALLOC_END to 0xff800000 so the guard area won't chop the
top of the 768MB lomem area while keeping the default vmalloc area size
unchanged and also preserving a gap between the vmalloc and fixmap
areas.

Signed-off-by: Nicolas Pitre <nico@linaro.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Russell King - ARM Linux Sept. 8, 2015, 12:10 a.m. UTC | #1 
On Mon, Sep 07, 2015 at 03:40:36PM -0400, Nicolas Pitre wrote:
> On Mon, 7 Sep 2015, Arnd Bergmann wrote:
> 
> > On Thursday 03 September 2015 21:24:00 Nicolas Pitre wrote:
> > > If 768MB targets were common place then it could be worth changing the 
> > > default vmalloc size to accommodate this memory size and testing all the 
> > > other targets to make sure no regressions are introduced.  But given it 
> > > is easy to change the default via the kernel cmdline, and that you lose 
> > > only 8 MB otherwise, I don't think it is worth the trouble and/or the 
> > > risk.
> > 
> > Agreed.
> 
> Well... I think there is a better solution.

Doesn't this clash with things like:

#define UNCACHEABLE_ADDR        0xff000000      /* IRQ_STAT */

?
Nicolas Pitre Sept. 8, 2015, 2:01 a.m. UTC | #2 
On Tue, 8 Sep 2015, Russell King - ARM Linux wrote:

> On Mon, Sep 07, 2015 at 03:40:36PM -0400, Nicolas Pitre wrote:
> > On Mon, 7 Sep 2015, Arnd Bergmann wrote:
> > 
> > > On Thursday 03 September 2015 21:24:00 Nicolas Pitre wrote:
> > > > If 768MB targets were common place then it could be worth changing the 
> > > > default vmalloc size to accommodate this memory size and testing all the 
> > > > other targets to make sure no regressions are introduced.  But given it 
> > > > is easy to change the default via the kernel cmdline, and that you lose 
> > > > only 8 MB otherwise, I don't think it is worth the trouble and/or the 
> > > > risk.
> > > 
> > > Agreed.
> > 
> > Well... I think there is a better solution.
> 
> Doesn't this clash with things like:
> 
> #define UNCACHEABLE_ADDR        0xff000000      /* IRQ_STAT */
> 
> ?

It looks like the move might actually "fix" it.  That UNCACHEABLE_ADDR 
is mapped with:

static struct map_desc ebsa110_io_desc[] __initdata = {
        /*
         * sparse external-decode ISAIO space
         */
        {       /* IRQ_STAT/IRQ_MCLR */
                .virtual        = (unsigned long)IRQ_STAT,
                .pfn            = __phys_to_pfn(TRICK4_PHYS),
                .length         = TRICK4_SIZE,
                .type           = MT_DEVICE
        },
[...]
};

This is passed to iotable_init(), then to create_mapping(). There you 
have:

        if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
            md->virtual >= PAGE_OFFSET &&
            (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
                pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
                        (long long)__pfn_to_phys((u64)md->pfn), md->virtual);
        }

So you must have hit the above warning somehow. Incidentally, this 
IRQ_STAT entry is the only one that happened to be outside the vmalloc 
area. By moving VMALLOC_END from 0xff000000 to 0xff800000 the warning 
will be gone.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Russell King - ARM Linux Sept. 8, 2015, 8:03 a.m. UTC | #3 
On Mon, Sep 07, 2015 at 10:01:41PM -0400, Nicolas Pitre wrote:
> On Tue, 8 Sep 2015, Russell King - ARM Linux wrote:
> 
> > On Mon, Sep 07, 2015 at 03:40:36PM -0400, Nicolas Pitre wrote:
> > > On Mon, 7 Sep 2015, Arnd Bergmann wrote:
> > > 
> > > > On Thursday 03 September 2015 21:24:00 Nicolas Pitre wrote:
> > > > > If 768MB targets were common place then it could be worth changing the 
> > > > > default vmalloc size to accommodate this memory size and testing all the 
> > > > > other targets to make sure no regressions are introduced.  But given it 
> > > > > is easy to change the default via the kernel cmdline, and that you lose 
> > > > > only 8 MB otherwise, I don't think it is worth the trouble and/or the 
> > > > > risk.
> > > > 
> > > > Agreed.
> > > 
> > > Well... I think there is a better solution.
> > 
> > Doesn't this clash with things like:
> > 
> > #define UNCACHEABLE_ADDR        0xff000000      /* IRQ_STAT */
> > 
> > ?
> 
> It looks like the move might actually "fix" it.  That UNCACHEABLE_ADDR 
> is mapped with:
> 
> static struct map_desc ebsa110_io_desc[] __initdata = {
>         /*
>          * sparse external-decode ISAIO space
>          */
>         {       /* IRQ_STAT/IRQ_MCLR */
>                 .virtual        = (unsigned long)IRQ_STAT,
>                 .pfn            = __phys_to_pfn(TRICK4_PHYS),
>                 .length         = TRICK4_SIZE,
>                 .type           = MT_DEVICE
>         },
> [...]
> };
> 
> This is passed to iotable_init(), then to create_mapping(). There you 
> have:
> 
>         if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
>             md->virtual >= PAGE_OFFSET &&
>             (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
>                 pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
>                         (long long)__pfn_to_phys((u64)md->pfn), md->virtual);
>         }
> 
> So you must have hit the above warning somehow. Incidentally, this 
> IRQ_STAT entry is the only one that happened to be outside the vmalloc 
> area. By moving VMALLOC_END from 0xff000000 to 0xff800000 the warning 
> will be gone.

No, there are other legacy platforms which have mappings above that.
Nicolas Pitre Sept. 8, 2015, 2:58 p.m. UTC | #4 
On Tue, 8 Sep 2015, Russell King - ARM Linux wrote:

> On Mon, Sep 07, 2015 at 10:01:41PM -0400, Nicolas Pitre wrote:
> 
[...]
> > This is passed to iotable_init(), then to create_mapping(). There you 
> > have:
> > 
> >         if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
> >             md->virtual >= PAGE_OFFSET &&
> >             (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
> >                 pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
> >                         (long long)__pfn_to_phys((u64)md->pfn), md->virtual);
> >         }
> > 
> > So you must have hit the above warning somehow. Incidentally, this 
> > IRQ_STAT entry is the only one that happened to be outside the vmalloc 
> > area. By moving VMALLOC_END from 0xff000000 to 0xff800000 the warning 
> > will be gone.
> 
> No, there are other legacy platforms which have mappings above that.

... for that particular map_desc array instance I mean.

WRT other legacy platforms: if the above warning didn't bother anyone 
for the last 4 years, then it won't be such an issue either if the 
current proposal doesn't solve them all.  That's not the primary purpose 
of this change anyway. In those cases, the appropriate fix, like it is 
today, should consist in some adjustment of those virtual addresses to 
bring them inside the vmalloc area.

Your original concern was whether extending the vmalloc area upward 
would clash with something else, and the short answer is no.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/Documentation/arm/memory.txt b/Documentation/arm/memory.txt
index 4178ebda6e..546a39048e 100644
--- a/Documentation/arm/memory.txt
+++ b/Documentation/arm/memory.txt
@@ -54,7 +54,7 @@  VMALLOC_START	VMALLOC_END-1	vmalloc() / ioremap() space.
 				located here through iotable_init().
 				VMALLOC_START is based upon the value
 				of the high_memory variable, and VMALLOC_END
-				is equal to 0xff000000.
+				is equal to 0xff800000.
 
 PAGE_OFFSET	high_memory-1	Kernel direct-mapped RAM region.
 				This maps the platforms RAM, and typically
diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index f40354198b..348caabb76 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -43,7 +43,7 @@ 
  */
 #define VMALLOC_OFFSET		(8*1024*1024)
 #define VMALLOC_START		(((unsigned long)high_memory + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
-#define VMALLOC_END		0xff000000UL
+#define VMALLOC_END		0xff800000UL
 
 #define LIBRARY_TEXT_START	0x0c000000