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

[Mark Rutland]

Hi Ard,

The below is modulo Rob's comments regarding fdt_to_phys and the
associated memory reservation. I'm not too worried where those live.

On Tue, Mar 03, 2015 at 11:03:47AM +0000, Ard Biesheuvel wrote:
> 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.

Nit: s/anymore/any more/

> At the same time, fix up some comments in head.S that have gone stale.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> ---
>  Documentation/arm64/booting.txt |  7 ++---
>  arch/arm64/include/asm/fixmap.h |  9 ++++++
>  arch/arm64/kernel/Makefile      |  1 +
>  arch/arm64/kernel/head.S        | 38 +------------------------
>  arch/arm64/kernel/setup.c       | 62 +++++++++++++++++++++++++++++++++++++----
>  5 files changed, 70 insertions(+), 47 deletions(-)
> 
> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index f3c05b5f9f08..bdc35fc97ac8 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -45,10 +45,9 @@ 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.

As we do elsewhere in booting.txt I'd prefer that we kept a note
regarding the restriction expected by older kernels, so bootloader/VM
authors can do the right thing for those on a best-effort basis.

[...]

> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -45,6 +45,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/efi.h>
>  #include <linux/personality.h>
> +#include <linux/libfdt.h>

I was going to say it would be nice to keep these ordered, but I see
from the rest of the includes that's a foregone hope. Never mind :(

[...]

> +static unsigned long const dt_virt_base = __fix_to_virt(FIX_FDT);

I'd prefer "static const unsigned long".

[...]

>  static void __init setup_machine_fdt(phys_addr_t dt_phys)
>  {
> -	if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys))) {
> +	void *dt_virt = NULL;
> +
> +	if (dt_phys && (dt_phys & 7) == 0)
> +		dt_virt = fixmap_remap_fdt(dt_phys);
> +

It might be worth checking that dt_phys is sufficiently far from the end
of a 2MB boundary that we can read the totalsize field below. Trivially
that means 8 bytes below, the header is 40 bytes, and any real DTB will
be larger than that.

It's a shame the arley DTB verification functions don't take a limit
parameter or we could prevent them from making potentially bad accesses.

> +	/*
> +	 * 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)) {
>  		early_print("\n"
>  			"Error: invalid device tree blob at physical address 0x%p (virtual address 0x%p)\n"
> -			"The dtb must be 8-byte aligned and passed in the first 512MB of memory\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);

I'm surprised the toolchain doesn't scream about dt_phys being a
phys_addr_t rather than a pointer here, given that's alway been wrong. I
guess the early_print wrapper managed to hide that from us -- can we
nuke that and use pr_crit here? 

With that we'd need to use %pa for the phys_addr_t, passing &dt_phys
rather than dt_phys.

Other than those points, this looks good to me.

Thanks,
Mark.