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/24/2017 8:36 AM, Mark Rutland wrote:
> Hi,
>
> On Tue, Feb 07, 2017 at 02:19:20PM -0700, 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>
>
> 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.

Ok, I'll go coordinate with our folks looking at kdump.

>
> 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.
>>


-- 
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.