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

[AKASHI Takahiro]

Mark,

On Fri, Feb 24, 2017 at 03:36:44PM +0000, Mark Rutland wrote:
> Hi,
> 
> On Tue, Feb 07, 2017 at 02:19:20PM -0700, 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.
> > 
> > Signed-off-by: Sameer Goel <sgoel at codeaurora.org>
> > Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
> 
> Technically the kdump ABI isn't set in stone yet, and this isn't a
> problem until that goes in.
> 
> So while this generally looks ok, it's possible that this may be
> unnecessary, and on reflection I do symptahise with Ard's earlier
> comment that this shouldn't otherwise be necessary for the empty dt.
> 
> So I'd prefer if this were folded into the kdump series if it's
> necessary. That way this goes in if it's necessary, and we can drop it
> otherwise.

I don't have any problem in folding this patch into my kdump series,
but I don't know how we should address Robin's comment:

> Hang on, if this code is shared with 32-bit ARM, doesn't this just move
> the problem around?

Since the values of *-cells under root node should reflect the hardware,
the kernel has no way to determine whether they be 1 or 2 here.

Thinking of this circumstance, I'd prefer to always use 64-bit values
for the address and the size. This aligns with other properties under /chosen
node, like initrd-* and uefi-*, whose values are also 64-bit wide.

(I know that the current kernel cannot boot if the entire memory is
located at >4GB, but it's a different issue.)

Thanks,
-Takahiro AKASHI

> Thanks,
> Mark.
> 
> > ---
> > 
> > [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;
> > -- 
> > 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.
> >