Skip to contentSkip to conversation

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

PATCH Superseded Jeffrey Hugo 6 participants 9 messages Feb 7, 2017 – Mar 8, 2017 EFI

lore
Share & tools
lore
#0 lore raw

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

From: Sameer Goel <[email protected]>

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.

---

[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(-)
drivers/firmware/efi/libstub/fdt.c +26 -2
index 921dfa0..22ea73b 100644
@@ -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;
Signature · 3 lines
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.
#1 lore raw

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

Quoted 15 lines · Jeffrey Hugo

From: Sameer Goel <[email protected]>

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.

Quoted 6 lines

Setting #address-cells and #size-cells to 2 at creation of the empty fdt resolves the observed issue, and makes the fdt less fragile.

---

Ping?

Quoted 59 lines

[v2]

(quoted diff) +0 -4
-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(-)
drivers/firmware/efi/libstub/fdt.c +26 -2
index 921dfa0..22ea73b 100644
@@ -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;
Signature · 5 lines
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.
#2 lore raw

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

In reply to Jeffrey Hugo
Quoted 10 lines · Jeffrey Hugo

From: Sameer Goel <[email protected]>

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.

Quoted 7 lines

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.

Quoted 62 lines

---

[v2]

(quoted diff) +0 -4
-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(-)
drivers/firmware/efi/libstub/fdt.c +26 -2
index 921dfa0..22ea73b 100644
@@ -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;
#3 lore raw

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

Quoted 19 lines · Robin Murphy
Quoted 10 lines · Jeffrey Hugo

From: Sameer Goel <[email protected]>

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.

Quoted 7 lines

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.

Quoted 6 lines

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

Quoted 70 lines
Quoted 53 lines

---

[v2]

(quoted diff) +0 -4
-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(-)
drivers/firmware/efi/libstub/fdt.c +19 -2
index 921dfa0..22ea73b 100644
@@ -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) {

drivers/firmware/efi/libstub/fdt.c static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, · line 75

Quoted 10 lines
drivers/firmware/efi/libstub/fdt.c +7 -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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
#4 lore raw

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

Quoted 2 lines · Sameer

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

To be clear, Akashi's patches already look at the cell sizes:

+static int __init early_init_dt_scan_usablemem(unsigned long node,
+ const char *uname, int depth, void *data)
+{

+       struct memblock_region *usablemem = data;
+       const __be32 *reg;
+       int len;

+
+ if (depth != 1 || strcmp(uname, "chosen") != 0)
+ return 0;
+
+ reg = of_get_flat_dt_prop(node, "linux,usable-memory-range", &len);
+ if (!reg || (len < (dt_root_addr_cells + dt_root_size_cells)))
+ return 1;
+
+ usablemem->base = dt_mem_next_cell(dt_root_addr_cells, &reg);
+ usablemem->size = dt_mem_next_cell(dt_root_size_cells, &reg);
+
+ return 1;
+}

This function uses dt_root_addr_cells and dt_root_size_cells. With Sameer's patch, those values should be 2 and 2, respectively, on ARM64 systems.

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
#5 lore raw

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

In reply to Jeffrey Hugo

Hi,

Quoted 15 lines · Jeffrey Hugo

From: Sameer Goel <[email protected]>

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.

Quoted 5 lines

Setting #address-cells and #size-cells to 2 at creation of the empty fdt resolves the observed issue, and makes the fdt less fragile.

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.

Thanks,
Mark.

Quoted 64 lines

---

[v2]

(quoted diff) +0 -4
-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(-)
drivers/firmware/efi/libstub/fdt.c +26 -2
index 921dfa0..22ea73b 100644
@@ -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;
Signature · 3 lines
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.
#6 lore raw

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

Quoted 27 lines · Mark Rutland

Hi,

Quoted 15 lines · Jeffrey Hugo

From: Sameer Goel <[email protected]>

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.

Quoted 5 lines

Setting #address-cells and #size-cells to 2 at creation of the empty fdt resolves the observed issue, and makes the fdt less fragile.

Technically the kdump ABI isn't set in stone yet, and this isn't a problem until that goes in.

Quoted 7 lines

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.

Quoted 68 lines

Thanks,
Mark.

Quoted 64 lines

---

[v2]

(quoted diff) +0 -4
-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(-)
drivers/firmware/efi/libstub/fdt.c +26 -3
index 921dfa0..22ea73b 100644
@@ -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.

Signature · 5 lines
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.
#7 lore raw

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

In reply to Mark Rutland

Mark,

Quoted 27 lines · Mark Rutland

Hi,

Quoted 15 lines · Jeffrey Hugo

From: Sameer Goel <[email protected]>

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.

Quoted 5 lines

Setting #address-cells and #size-cells to 2 at creation of the empty fdt resolves the observed issue, and makes the fdt less fragile.

Technically the kdump ABI isn't set in stone yet, and this isn't a problem until that goes in.

Quoted 7 lines

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:

Quoted 2 lines

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

Quoted 67 lines

Thanks,
Mark.

Quoted 64 lines

---

[v2]

(quoted diff) +0 -4
-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(-)
drivers/firmware/efi/libstub/fdt.c +26 -2
index 921dfa0..22ea73b 100644
@@ -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;
Signature · 3 lines
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.
#8 lore raw

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

Quoted 2 lines · AKASHI Takahiro

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.

It's safe to set the *-cells to 2 on all platforms, as long as everyone who reads/writes from/to the device tree respects those values (which is what they're supposed to do anyway).

Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.