From mboxrd@z Thu Jan 1 00:00:00 1970 From: jhugo@codeaurora.org (Jeffrey Hugo) Date: Fri, 24 Feb 2017 08:54:17 -0700 Subject: [PATCH v2] efi/libstub/arm*: Set default address and size cells values for an empty dtb In-Reply-To: <20170224153644.GE417@leverpostej> References: <1486502360-18071-1-git-send-email-jhugo@codeaurora.org> <20170224153644.GE417@leverpostej> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.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 >> >> 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 >> Signed-off-by: Jeffrey Hugo > > 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.