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

[Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2/7/2017 2:19 PM, Jeffrey Hugo wrote:
> From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.
>
> Signed-off-by: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---

Ping?

>
> [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;
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.