Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region

From: Ard Biesheuvel <ardb@kernel.org>
To: Nicolas Pitre <nico@fluxnic.net>
Cc: Rob Herring <robh+dt@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 2/2] ARM: move device tree mapping out of linear region
Date: Wed, 7 Oct 2020 17:32:54 +0200	[thread overview]
Message-ID: <CAMj1kXFjJFDCHVZO1hTWNkZCtXVW29v2aQv7T+-FnYB4Jv99Og@mail.gmail.com> (raw)
In-Reply-To: <nycvar.YSQ.7.78.906.2010071026420.2184@knanqh.ubzr>

On Wed, 7 Oct 2020 at 17:28, Nicolas Pitre <nico@fluxnic.net> wrote:
>
> On Wed, 7 Oct 2020, Ard Biesheuvel wrote:
>
> > @@ -1078,13 +1078,18 @@ void __init hyp_mode_check(void)
> >  void __init setup_arch(char **cmdline_p)
> >  {
> >       const struct machine_desc *mdesc = NULL;
> > +     void *atags_vaddr;
> >
> >       if (__atags_pointer)
> > -             atags_vaddr = phys_to_virt(__atags_pointer);
> > +             atags_vaddr = FDT_VIRT_ADDR(__atags_pointer);
> >
> >       setup_processor();
> > -     if (atags_vaddr)
> > +     if (atags_vaddr) {
> >               mdesc = setup_machine_fdt(atags_vaddr);
>
> If __atags_pointer is zero, you'll end up with atags_vaddr being
> undefined here. I'm surprised the compiler didn't warn about that.
>

Ah yes, well spotted. I'll add a NULL initializer.

> > @@ -947,7 +949,7 @@ static void __init create_mapping(struct map_desc *md)
> >               return;
> >       }
> >
> > -     if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
> > +     if (md->type == MT_DEVICE &&
> >           md->virtual >= PAGE_OFFSET && md->virtual < FIXADDR_START &&
> >           (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) {
> >               pr_warn("BUG: mapping for 0x%08llx at 0x%08lx out of vmalloc space\n",
>
> Hmmm... I wonder why MT_ROM was there in the first place.
>
> Digging into the history of this particular test (which has seen many
> slight variations over the years and could probably be simplified
> further nowdays) I finally found where that MT_ROM condition was
> introduced:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=457450ab3b94
>
> Oh well...
>

I didn't dig quite that far, but I did notice that the test predates
get Git era.

My reasoning to remove it was that MT_ROM is only used locally in the
same source file, whereas MT_DEVICE is used all over the place by
machine files, and so MT_DEVICE obviously requires this kind of
policing to ensure that the static device mappings do not overlap. The
XIP kernel mapping and now the DTB mapping were carefully placed in
the memory map, and so MT_ROM regions can be disregarded here.

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