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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
>
> 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.
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
