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

[jhugo@codeaurora.org (Jeffrey Hugo)]

On 2/7/2017 11:12 AM, Ard Biesheuvel wrote:
> Hi Jeffrey,
>
> (adding Mark)
>
> On 7 February 2017 at 17:59, Jeffrey Hugo <jhugo@codeaurora.org> 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.  Sets the address and size cell values
>> in a generated fdt to support 64 bit addressing.
>>
>> This enables kexec/kdump on Qualcomm Technologies QDF24XX platforms as those
>> utilities will read the address/size values from the fdt, and such values
>> may exceed the range provided by the 32 bit default.
>>
>
> As far as I know, those properties are explicitly associated with the
> 'reg' properties of subordinate nodes. So which nodes are we talking
> about here? Are we producing an incorrect DT by not setting these? Or
> is this simply a convenience to work around bugs in the tooling?

I think we are producing an incorrect DT, in some instances.

So we are starting from the same baseline, this is specific to ACPI 
systems, as an ACPI system won't have a DT from the bootloader.  DT 
based systems will already have a DT from the bootloader which is 
assumed to be correct.  On ACPI systems without a DT, efistub generates 
a default one.

That default is assumed to be for a 32-bit system.  The cell width 
defaults to 1, which is 4 bytes.  You cannot represent a 64-bit value in 
that instance.

What happens is that kexec inserts properties into the fdt which contain 
the start address and size on the crash kernel.  On our system, the 
start address is a 64-bit value, and while its not the case today, I see 
no reason why size could not also be a 64-bit value.  However the values 
that are inserted into the fdt are governed by the address and size cell 
values already present in the fdt.

Kexec attempts to insert these values in the fdt.  The fdt only accepts 
32-bit values, so it truncates what is put in.  Then later kexec/kdump 
read the values from the fdt, and get garbage.

By changing the defaults to 2 (the proposed change), 64-bit values can 
be inserted into the fdt, so the values we put in don't get truncated, 
and thus kexec/kdump read the correct thing when they need the values.

I don't see how the tools could be fixed - fdt is truncating the values, 
and the generated fdt is already "static" at the point the tools run. 
We haven't had luck changing the cell size at the point the tools run. 
Additionally, this seems to be an issue for everything using the fdt - 
pushing the problem to every tool instead of fixing once at the top 
seems like playing a game of whack-a-mole.

Does that clarify that issue for you?  Obviously the commit text needs 
some work, but I'd like to get on the same page first.

>
>> Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5
>
> Please drop these IDs when you post
>
>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>  drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index 921dfa0..def5c9c 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -16,6 +16,34 @@
>>
>>  #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;
>> +       int status;
>> +
>> +       offset = fdt_path_offset(fdt, "/");
>> +       /* Set the #address-cells and #size-cells values for an empty tree */
>> +
>> +       status = fdt_setprop_u32(fdt, offset, "#address-cells",
>> +                                EFI_DT_ADDR_CELLS_DEFAULT);
>> +       if (status) {
>> +               pr_efi(sys_table,
>> +                      "Failed to set #address-cells for empty dtb\n");
>> +               return;
>> +       }
>> +
>> +       status = fdt_setprop_u32(fdt, offset, "#size-cells",
>> +                                EFI_DT_SIZE_CELLS_DEFAULT);
>> +       if (status) {
>> +               pr_efi(sys_table,
>> +                      "Failed to set #size-cells for empty dtb\n");
>> +               return;
>> +       }
>> +}
>> +
>>  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,
>> @@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *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.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


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