[PATCH 0/3] Assorted fixes related to reserved memory
This series has few small assorted fixes related to reserved memory support in RISC-V and EFI.
The series is rebased on top of the following series http://patchwork.ozlabs.org/project/uboot/patch/1591767391-2669-2-git-send-e...
Changes from v1->v2: 1. Rebased on top of the Bin's series. Dropped the fix generic fdtdec code. 2. Added bootefi fix.
ption-prefix PATCH v2
Atish Patra (3): riscv: Do not return error if reserved node already exists riscv: Use optimized version of fdtdec_get_addr_size_no_parent cmd: bootefi: Honor the address & size cells properties correctly
arch/riscv/lib/fdt_fixup.c | 4 ++-- cmd/bootefi.c | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-)
-- 2.24.0
Not all errors are fatal. If a reserved memory node already exists in the destination device tree, we can continue to boot without failing.
Signed-off-by: Atish Patra atish.patra@wdc.com --- arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 6db48ad04a56..91524d9a5ae9 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) pmp_mem.end = addr + size - 1; err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem, &phandle); - if (err < 0) { + if (err < 0 && err != FDT_ERR_EXISTS) { printf("failed to add reserved memory: %d\n", err); return err; }
Hi Atish,
On Fri, Jun 19, 2020 at 9:52 AM Atish Patra atish.patra@wdc.com wrote:
Not all errors are fatal. If a reserved memory node already exists in the destination device tree, we can continue to boot without failing.
Signed-off-by: Atish Patra atish.patra@wdc.com
arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 6db48ad04a56..91524d9a5ae9 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) pmp_mem.end = addr + size - 1; err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem, &phandle);
if (err < 0) {
if (err < 0 && err != FDT_ERR_EXISTS) {
This FDT_ERR_EXISTS error code is possibly returned by:
node = fdt_add_subnode(blob, parent, name); if (node < 0) return node;
But if it exists, I believe fdtdec_add_reserved_memory() already returns 0 because they are likely to have the same range of memory block start address and size, no?
printf("failed to add reserved memory: %d\n", err); return err; }--
Regards, Bin
On Tue, Jun 23, 2020 at 12:24 AM Bin Meng bmeng.cn@gmail.com wrote:
Hi Atish,
On Fri, Jun 19, 2020 at 9:52 AM Atish Patra atish.patra@wdc.com wrote:
Not all errors are fatal. If a reserved memory node already exists in the destination device tree, we can continue to boot without failing.
Signed-off-by: Atish Patra atish.patra@wdc.com
arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 6db48ad04a56..91524d9a5ae9 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) pmp_mem.end = addr + size - 1; err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem, &phandle);
if (err < 0) {
if (err < 0 && err != FDT_ERR_EXISTS) {This FDT_ERR_EXISTS error code is possibly returned by:
node = fdt_add_subnode(blob, parent, name); if (node < 0) return node;But if it exists, I believe fdtdec_add_reserved_memory() already returns 0 because they are likely to have the same range of memory block start address and size, no?
Currently, yes. As libfdt and fdtdec_add_reserved_memory external to this code, functionality can change without modifying fdt_fixup.c knowingly or unknowingly. FDT_ERR_EXISTS is harmless error in this context and we shouldn't cause boot failure because of that. That's why I add that exclusion.
printf("failed to add reserved memory: %d\n", err); return err; }--
Regards, Bin
Hi Atish,
On Wed, Jun 24, 2020 at 2:17 AM Atish Patra atishp@atishpatra.org wrote:
On Tue, Jun 23, 2020 at 12:24 AM Bin Meng bmeng.cn@gmail.com wrote:
Hi Atish,
On Fri, Jun 19, 2020 at 9:52 AM Atish Patra atish.patra@wdc.com wrote:
Not all errors are fatal. If a reserved memory node already exists in the destination device tree, we can continue to boot without failing.
Signed-off-by: Atish Patra atish.patra@wdc.com
arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 6db48ad04a56..91524d9a5ae9 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) pmp_mem.end = addr + size - 1; err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem, &phandle);
if (err < 0) {
if (err < 0 && err != FDT_ERR_EXISTS) {This FDT_ERR_EXISTS error code is possibly returned by:
node = fdt_add_subnode(blob, parent, name); if (node < 0) return node;But if it exists, I believe fdtdec_add_reserved_memory() already returns 0 because they are likely to have the same range of memory block start address and size, no?
Currently, yes. As libfdt and fdtdec_add_reserved_memory external to this code, functionality can change without modifying fdt_fixup.c knowingly or unknowingly. FDT_ERR_EXISTS is harmless error in this context and we shouldn't cause boot failure because of that. That's why I add that exclusion.
Okay. But the error should be checked against -FDT_ERR_EXISTS.
Regards, Bin
On Tue, Jun 23, 2020 at 5:51 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Atish,
On Wed, Jun 24, 2020 at 2:17 AM Atish Patra atishp@atishpatra.org wrote:
On Tue, Jun 23, 2020 at 12:24 AM Bin Meng bmeng.cn@gmail.com wrote:
Hi Atish,
On Fri, Jun 19, 2020 at 9:52 AM Atish Patra atish.patra@wdc.com wrote:
Not all errors are fatal. If a reserved memory node already exists in the destination device tree, we can continue to boot without failing.
Signed-off-by: Atish Patra atish.patra@wdc.com
arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 6db48ad04a56..91524d9a5ae9 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) pmp_mem.end = addr + size - 1; err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem, &phandle);
if (err < 0) {
if (err < 0 && err != FDT_ERR_EXISTS) {This FDT_ERR_EXISTS error code is possibly returned by:
node = fdt_add_subnode(blob, parent, name); if (node < 0) return node;But if it exists, I believe fdtdec_add_reserved_memory() already returns 0 because they are likely to have the same range of memory block start address and size, no?
Currently, yes. As libfdt and fdtdec_add_reserved_memory external to this code, functionality can change without modifying fdt_fixup.c knowingly or unknowingly. FDT_ERR_EXISTS is harmless error in this context and we shouldn't cause boot failure because of that. That's why I add that exclusion.
Okay. But the error should be checked against -FDT_ERR_EXISTS.
My bad. I will fix it and send the next version.
Regards, Bin
fdtdec_get_addr_size_no_parent is not an optimized version if parent node is already available with the caller.
Use fdtdec_get_addr_size_auto_parent to read the "reg" property
Signed-off-by: Atish Patra atish.patra@wdc.com --- arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 91524d9a5ae9..00b84dccbef0 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) fdt_for_each_subnode(node, src, offset) { name = fdt_get_name(src, node, NULL);
- addr = fdtdec_get_addr_size_auto_noparent(src, node, + addr = fdtdec_get_addr_size_auto_parent(src, offset, node, "reg", 0, &size, false); if (addr == FDT_ADDR_T_NONE) {
On Fri, Jun 19, 2020 at 9:52 AM Atish Patra atish.patra@wdc.com wrote:
fdtdec_get_addr_size_no_parent is not an optimized version if parent node is already available with the caller.
Use fdtdec_get_addr_size_auto_parent to read the "reg" property
Signed-off-by: Atish Patra atish.patra@wdc.com
arch/riscv/lib/fdt_fixup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 91524d9a5ae9..00b84dccbef0 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) fdt_for_each_subnode(node, src, offset) { name = fdt_get_name(src, node, NULL);
addr = fdtdec_get_addr_size_auto_noparent(src, node,
addr = fdtdec_get_addr_size_auto_parent(src, offset, node, "reg", 0, &size, false);
nits: please make above 2 lines indent to the (
if (addr == FDT_ADDR_T_NONE) {--
Reviewed-by: Bin Meng bin.meng@windriver.com
fdtdec_get_addr_size reads the uses a fixed value for address & size cell properties which may not be correct always.
Use the auto variant of the function which automatically reads #address-cells & #size-cells from parent and uses to read the "reg" property.
Signed-off-by: Atish Patra atish.patra@wdc.com --- cmd/bootefi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 0f6d0f77507c..5f3fcce597de 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -190,8 +190,9 @@ static void efi_carve_out_dt_rsv(void *fdt) subnode = fdt_first_subnode(fdt, nodeoffset); while (subnode >= 0) { /* check if this subnode has a reg property */ - addr = fdtdec_get_addr_size(fdt, subnode, "reg", - (fdt_size_t *)&size); + addr = fdtdec_get_addr_size_auto_parent(fdt, nodeoffset, + subnode, "reg", 0, + (fdt_size_t *)&size, false); /* * The /reserved-memory node may have children with * a size instead of a reg property.
On 6/19/20 3:51 AM, Atish Patra wrote:
fdtdec_get_addr_size reads the uses a fixed value for address & size cell properties which may not be correct always.
Use the auto variant of the function which automatically reads #address-cells & #size-cells from parent and uses to read the "reg" property.
Signed-off-by: Atish Patra atish.patra@wdc.com
cmd/bootefi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 0f6d0f77507c..5f3fcce597de 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -190,8 +190,9 @@ static void efi_carve_out_dt_rsv(void *fdt) subnode = fdt_first_subnode(fdt, nodeoffset); while (subnode >= 0) { /* check if this subnode has a reg property */
addr = fdtdec_get_addr_size(fdt, subnode, "reg",(fdt_size_t *)&size);
addr = fdtdec_get_addr_size_auto_parent(fdt, nodeoffset,subnode, "reg", 0,(fdt_size_t *)&size, false);
On qemu_arm_defconfig: sizeof(fdt_size_t) = 4, sizeof(u64) = 8. So after the call the upper four bytes of size will be random bytes from the stack.
Best regards
Heinrich
/* * The /reserved-memory node may have children with * a size instead of a reg property.
On Thu, Jun 18, 2020 at 11:51 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 6/19/20 3:51 AM, Atish Patra wrote:
fdtdec_get_addr_size reads the uses a fixed value for address & size cell properties which may not be correct always.
Use the auto variant of the function which automatically reads #address-cells & #size-cells from parent and uses to read the "reg" property.
Signed-off-by: Atish Patra atish.patra@wdc.com
cmd/bootefi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 0f6d0f77507c..5f3fcce597de 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -190,8 +190,9 @@ static void efi_carve_out_dt_rsv(void *fdt) subnode = fdt_first_subnode(fdt, nodeoffset); while (subnode >= 0) { /* check if this subnode has a reg property */
addr = fdtdec_get_addr_size(fdt, subnode, "reg",(fdt_size_t *)&size);
addr = fdtdec_get_addr_size_auto_parent(fdt, nodeoffset,subnode, "reg", 0,(fdt_size_t *)&size, false);On qemu_arm_defconfig: sizeof(fdt_size_t) = 4, sizeof(u64) = 8. So after the call the upper four bytes of size will be random bytes from the stack.
Ah yes. Thanks for catching that and the quick fix.
Best regards
Heinrich
/* * The /reserved-memory node may have children with * a size instead of a reg property.
participants (4)
-
Atish Patra -
Atish Patra -
Bin Meng -
Heinrich Schuchardt