[v3,2/5] arm64: use fixmap region for permanent FDT mapping

Currently, the FDT blob needs to be in the same naturally aligned
512 MB region as the kernel, so that it can be mapped into the
kernel virtual memory space very early on using a minimal set of
statically allocated translation tables.

Now that we have early fixmap support, we can relax this restriction,
by moving the permanent FDT mapping to the fixmap region instead.
This way, the FDT blob may be anywhere in memory.

This also moves the vetting of the FDT to setup.c, since the early
init code in head.S does not handle mapping of the FDT anymore.
At the same time, fix up some comments in head.S that have gone stale.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/arm64/booting.txt | 10 +++---
 arch/arm64/include/asm/fixmap.h |  9 ++++++
 arch/arm64/kernel/Makefile      |  1 +
 arch/arm64/kernel/head.S        | 38 +---------------------
 arch/arm64/kernel/setup.c       | 72 +++++++++++++++++++++++++++++------------
 arch/arm64/mm/init.c            |  1 -
 6 files changed, 69 insertions(+), 62 deletions(-)

On 9 April 2015 at 13:49, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> This looks good to me (with some minor comments below).
>
> As a heads-up, this clashes with other changes in the arm64
> for-next/core branch due to some unrelated changes in head.S and
> setup.c.
>
>> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
>> index f3c05b5f9f08..ab5a90adece3 100644
>> --- a/Documentation/arm64/booting.txt
>> +++ b/Documentation/arm64/booting.txt
>> @@ -45,11 +45,13 @@ sees fit.)
>>
>>  Requirement: MANDATORY
>>
>> -The device tree blob (dtb) must be placed on an 8-byte boundary within
>> -the first 512 megabytes from the start of the kernel image and must not
>> -cross a 2-megabyte boundary. This is to allow the kernel to map the
>> -blob using a single section mapping in the initial page tables.
>> +The device tree blob (dtb) must be placed on an 8-byte boundary and must
>> +not cross a 2-megabyte boundary. This is to allow the kernel to map the
>> +blob using a single section mapping in the fixmap region.
>
> Please drop the mention of the fixmap. If we ever move this out of the
> fixmap I don't want to have to update booting.txt again, and it
> shouldn't matter to bootloader authors either way.
>

OK.

>> +NOTE: versions prior to v4.1 require, in addition to the requirements
>> +listed above, that the dtb be placed above the kernel Image inside the
>> +same naturally aligned 512 MB region.
>
> Minor nit: This would read a little better if we just said "versions
> prior to v4.1 also require that ...", dropping "in addition to the
> requirements listed above".
>

OK

Actually, I realized that this is incorrect anyway: it is not the same
naturally aligned 512 MB region, it is actually 'within 512 MB of the
start of the kernel Image' since that itself is naturally aligned to
512 MB in the virtual address space.

>>  enum fixed_addresses {
>>       FIX_HOLE,
>> +
>> +     /*
>> +      * Reserve 2 MB of virtual space for the FDT at the top of the fixmap
>> +      * region. Keep this at the top so it remains 2 MB aligned.
>> +      */
>> +#define FIX_FDT_SIZE         SZ_2M
>> +     FIX_FDT_END,
>> +     FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
>> +
>>       FIX_EARLYCON_MEM_BASE,
>>       FIX_TEXT_POKE0,
>>       __end_of_permanent_fixed_addresses,
>
> [...]
>
>> +static void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
>> +{
>> +     const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
>> +     phys_addr_t dt_phys_base = dt_phys & ~(FIX_FDT_SIZE - 1);
>> +
>> +     /*
>> +      * Make sure that the FDT region can be mapped without the need to
>> +      * allocate additional translation table pages, so that it is safe
>> +      * to call create_pgd_mapping() this early.
>> +      * On 4k pages, we'll use section mappings for the region so we
>> +      * only have to be in the same PUD as the rest of the fixmap.
>> +      * On 64k pages, we need to be in the same PMD as well, as the region
>> +      * will be mapped using PTEs.
>> +      */
>> +     BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));
>
> s/SZ_2M/FIX_FDT_SIZE/
>
> Perhaps we should also have:
>
> BUILD_BUG_ON_NOT_POWER_OF_2(FIX_FDT_SIZE);
>
> ... given it's necessary for the address masking we do here.
>
> [...]
>

I was going to suggest to use SZ_2M for the masking, but map a 4 MB
window. That way, we can relax the requirement from "should not cross
a 2 MB boundary' to 'should not exceed 2 MB in size', which is
arguable easier to adhere to, since the latter is a property of the
DTB itself, whereas the former is a property of whatever your malloc()
gives you. That means the mask would remain SZ_2M, and the size would
become SZ_4M


>> +     /*
>> +      * Before passing the dt_virt pointer to early_init_dt_scan(), we have
>> +      * to ensure that the FDT size as reported in the FDT itself does not
>> +      * exceed the 2 MB window we just mapped for it.
>> +      */
>> +     if (!dt_virt ||
>> +         fdt_check_header(dt_virt) != 0 ||
>> +         (dt_phys & (SZ_2M - 1)) + fdt_totalsize(dt_virt) > SZ_2M ||
>> +         !early_init_dt_scan(dt_virt)) {
>> +             pr_crit("\n"
>> +                     "Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
>> +                     "The dtb must be 8-byte aligned and must not cross a 2 MB alignment boundary\n"
>>                       "\nPlease check your bootloader.\n",
>> -                     dt_phys, phys_to_virt(dt_phys));
>> +                     &dt_phys, dt_virt);
>
> Nit: drop the leading '\n' on the last line. There's no need for it.
>

OK

> I've tested this on my Juno (atop of arm64 for-next/core), and all seems
> well, so with those points addressed:
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks, but this code will likely look quite different if we are going
to cater for moving the kernel out of the linear range, so by then I
probably won't be able to retain these tags.

Cheers,
Ard.

On 9 April 2015 at 15:12, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> +NOTE: versions prior to v4.1 require, in addition to the requirements
>> >> +listed above, that the dtb be placed above the kernel Image inside the
>> >> +same naturally aligned 512 MB region.
>> >
>> > Minor nit: This would read a little better if we just said "versions
>> > prior to v4.1 also require that ...", dropping "in addition to the
>> > requirements listed above".
>> >
>>
>> OK
>>
>> Actually, I realized that this is incorrect anyway: it is not the same
>> naturally aligned 512 MB region, it is actually 'within 512 MB of the
>> start of the kernel Image' since that itself is naturally aligned to
>> 512 MB in the virtual address space.
>
> Surely starting at TEXT_OFFSET bytes below the kernel image? ;)
>

Only if you find it acceptable that people start putting their FDT in
the TEXT_OFFSET region ... :-)

>> >> +static void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
>> >> +{
>> >> +     const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
>> >> +     phys_addr_t dt_phys_base = dt_phys & ~(FIX_FDT_SIZE - 1);
>> >> +
>> >> +     /*
>> >> +      * Make sure that the FDT region can be mapped without the need to
>> >> +      * allocate additional translation table pages, so that it is safe
>> >> +      * to call create_pgd_mapping() this early.
>> >> +      * On 4k pages, we'll use section mappings for the region so we
>> >> +      * only have to be in the same PUD as the rest of the fixmap.
>> >> +      * On 64k pages, we need to be in the same PMD as well, as the region
>> >> +      * will be mapped using PTEs.
>> >> +      */
>> >> +     BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));
>> >
>> > s/SZ_2M/FIX_FDT_SIZE/
>> >
>> > Perhaps we should also have:
>> >
>> > BUILD_BUG_ON_NOT_POWER_OF_2(FIX_FDT_SIZE);
>> >
>> > ... given it's necessary for the address masking we do here.
>> >
>> > [...]
>> >
>>
>> I was going to suggest to use SZ_2M for the masking, but map a 4 MB
>> window. That way, we can relax the requirement from "should not cross
>> a 2 MB boundary' to 'should not exceed 2 MB in size', which is
>> arguable easier to adhere to, since the latter is a property of the
>> DTB itself, whereas the former is a property of whatever your malloc()
>> gives you. That means the mask would remain SZ_2M, and the size would
>> become SZ_4M
>
> That sounds good so long as it doesn't get in the way of increasing
> FIX_FDT_SIZE in future.
>

No, it shouldn't. And it actually makes it more obvious when we do
increase it later which values should be left alone (SZ_2M) and which
values should be increased (the higher ones)

>> > I've tested this on my Juno (atop of arm64 for-next/core), and all seems
>> > well, so with those points addressed:
>> >
>> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> > Tested-by: Mark Rutland <mark.rutland@arm.com>
>>
>> Thanks, but this code will likely look quite different if we are going
>> to cater for moving the kernel out of the linear range, so by then I
>> probably won't be able to retain these tags.
>
> Ok. I guess you'll merge the two series together?
>

Yes. I am currently looking into a way to make early_fixmap_init() not
rely on __va(), and if that is easily doable, I will propose something
for the FDT which is quite close to the latest submitted version. But
otherwise, it may be a bit more complicated, and the FDT mapping needs
more work.