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

["Goel, Sameer" <sgoel@codeaurora.org>]

On 2/24/2017 8:36 AM, Robin Murphy wrote:
> On 07/02/17 21:19, Jeffrey Hugo wrote:
>> From: Sameer Goel <sgoel@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.
Kdump additions to the kexec code for arm64 is assuming 8 byte resource values 
for updating the chosen node with the address and size values. 
The 32 bit kexec is not updating the chosen node to set up the crashkernel start 
property and is passing the required parameters as command line args.

Any tool making the device tree update should be checking for the cell sizes before 
assuming a 32 bit or a 64 bit value.

Sameer   

> 
>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>> Signed-off-by: Jeffrey Hugo <jhugo@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;
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.