[PATCH v2] efi/libstub/arm*: Set default address and size cells values for an empty dtb

[Robin Murphy]

On 07/02/17 21:19, Jeffrey Hugo wrote:
> From: Sameer Goel <sgoel at codeaurora.org>
> 
> In cases where a device tree is not provided (ie ACPI based system), an
> empty fdt is generated by efistub.  #address-cells and #size-cells are not
> set in the empty fdt, so they default to 1 (4 byte wide).  This can be an
> issue on 64-bit systems where values representing addresses, etc may be
> 8 bytes wide as the default value does not align with the general
> requirements for an empty DTB, and is fragile when passed to other agents
> as extra care is required to read the entire width of a value.
> 
> This issue is observed on Qualcomm Technologies QDF24XX platforms when
> kexec-tools inserts 64-bit addresses into the "linux,elfcorehdr" and
> "linux,usable-memory-range" properties of the fdt.  When the values are
> later consumed, they are truncated to 32-bit.
> 
> Setting #address-cells and #size-cells to 2 at creation of the empty fdt
> resolves the observed issue, and makes the fdt less fragile.

Hang on, if this code is shared with 32-bit ARM, doesn't this just move
the problem around? If arm64 kexec is blindly punting reg properties
into a DT assuming *-cells == 2, then wouldn't ARM kexec likely be doing
the same relying on *-cells == 1?

Robin.

> Signed-off-by: Sameer Goel <sgoel at codeaurora.org>
> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
> ---
> 
> [v2]
> -Add braces to an if when the corresponding else has braces
> -Remove print statements
> -Reword commit text
> -Removed gerrit tag
> 
>  drivers/firmware/efi/libstub/fdt.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa0..22ea73b 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -16,6 +16,22 @@
>  
>  #include "efistub.h"
>  
> +#define EFI_DT_ADDR_CELLS_DEFAULT 2
> +#define EFI_DT_SIZE_CELLS_DEFAULT 2
> +
> +static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt)
> +{
> +	int offset;
> +
> +	offset = fdt_path_offset(fdt, "/");
> +	/* Set the #address-cells and #size-cells values for an empty tree */
> +
> +	fdt_setprop_u32(fdt, offset, "#address-cells",
> +			EFI_DT_ADDR_CELLS_DEFAULT);
> +
> +	fdt_setprop_u32(fdt, offset, "#size-cells", EFI_DT_SIZE_CELLS_DEFAULT);
> +}
> +
>  static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  			       unsigned long orig_fdt_size,
>  			       void *fdt, int new_fdt_size, char *cmdline_ptr,
> @@ -42,10 +58,18 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  		}
>  	}
>  
> -	if (orig_fdt)
> +	if (orig_fdt) {
>  		status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
> -	else
> +	} else {
>  		status = fdt_create_empty_tree(fdt, new_fdt_size);
> +		if (status == 0) {
> +			/*
> +			 * Any failure from the following function is non
> +			 * critical
> +			 */
> +			fdt_update_cell_size(sys_table, fdt);
> +		}
> +	}
>  
>  	if (status != 0)
>  		goto fdt_set_fail;
>