On Mon, Mar 23, 2020 at 11:14 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 3/24/20 5:16 AM, Atish Patra wrote:

...

In RISC-V, M-mode software can reserve physical memory regions by setting appropriate physical memory protection (PMP) csr. As the PMP csr are accessible only in M-mode, S-mode U-Boot can not read this configuration directly. However, M-mode software can pass this information via reserved-memory node in device tree so that S-mode software can access this information.

This patch provides a framework to copy to the reserved-memory node from one DT to another. This will be used to update the DT used by U-Boot and the DT passed to the next stage OS.

Signed-off-by: Atish Patra atish.patra@wdc.com

arch/riscv/cpu/start.S | 1 + arch/riscv/include/asm/global_data.h | 1 + arch/riscv/include/asm/u-boot-riscv.h | 2 + arch/riscv/lib/Makefile | 1 + arch/riscv/lib/asm-offsets.c | 1 + arch/riscv/lib/fdt_fixup.c | 80 +++++++++++++++++++++++++++ 6 files changed, 86 insertions(+) create mode 100644 arch/riscv/lib/fdt_fixup.c

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index 6b3ff99c3882..0282685c2906 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -121,6 +121,7 @@ call_board_init_f_0:

  jal     board_init_f_init_reserve

• SREG    s1, GD_FIRMWARE_FDT_ADDR(gp)
  /* save the boot hart id to global_data */
  SREG    tp, GD_BOOT_HART(gp)
  
  

diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index b74bd7e738bb..51ac8d1c98e2 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -15,6 +15,7 @@ /* Architecture-specific global data */ struct arch_global_data { long boot_hart; /* boot hart id */

• phys_addr_t firmware_fdt_addr;
  

  #ifdef CONFIG_SIFIVE_CLINT void __iomem *clint; /* clint base address */ #endif

diff --git a/arch/riscv/include/asm/u-boot-riscv.h b/arch/riscv/include/asm/u-boot-riscv.h index 49febd588102..543a1688db8f 100644 --- a/arch/riscv/include/asm/u-boot-riscv.h +++ b/arch/riscv/include/asm/u-boot-riscv.h @@ -17,5 +17,7 @@ int cleanup_before_linux(void); /* board/.../... */ int board_init(void); void board_quiesce_devices(void); +int riscv_board_reserved_mem_fixup(void *fdt); +int riscv_fdt_copy_resv_mem_node(const void *src_fdt, void *dest_fdt);

#endif /* _U_BOOT_RISCV_H_ */ diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index adadbf4bcbef..d132b59ce32c 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -24,6 +24,7 @@ obj-y += reset.o obj-y += setjmp.o obj-$(CONFIG_SMP) += smp.o obj-$(CONFIG_SPL_BUILD) += spl.o +obj-y += fdt_fixup.o

# For building EFI apps CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI) diff --git a/arch/riscv/lib/asm-offsets.c b/arch/riscv/lib/asm-offsets.c index 4fa4fd371473..7301c1b98e23 100644 --- a/arch/riscv/lib/asm-offsets.c +++ b/arch/riscv/lib/asm-offsets.c @@ -14,6 +14,7 @@ int main(void) { DEFINE(GD_BOOT_HART, offsetof(gd_t, arch.boot_hart));

• DEFINE(GD_FIRMWARE_FDT_ADDR, offsetof(gd_t, arch.firmware_fdt_addr));
  

  #ifndef CONFIG_XIP DEFINE(GD_AVAILABLE_HARTS, offsetof(gd_t, arch.available_harts)); #endif

diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c new file mode 100644 index 000000000000..f3d1ec5c5d02 --- /dev/null +++ b/arch/riscv/lib/fdt_fixup.c @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2020 Western Digital Corporation or its affiliates
• */

+#include <common.h> +#include <fdt_support.h> +#include <mapmem.h>

+DECLARE_GLOBAL_DATA_PTR;

Please, provide Sphinx style comments for new functions. Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html.

Sure. I will do that.

...

+int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) +{

• u32 phandle;
  

• struct fdt_memory pmp_mem;
  

• fdt_addr_t addr;
  

• fdt_size_t size;
  

• int offset, node, err, rmem_offset;
  

• bool nomap = true;
  

• char basename[32] = {0};
  

• int bname_len;
  

• int max_len = sizeof(basename);
  

• const char *name;
  

• char *temp;
  

• offset = fdt_path_offset(src, "/reserved-memory");
  

• if (offset < 0) {
  

•         printf("No reserved memory region found in source FDT\n");
  

•         return 0;
  

• }
  

• fdt_for_each_subnode(node, src, offset) {
  

•         name = fdt_get_name(src, node, NULL);
  

•         addr = fdtdec_get_addr_size_auto_noparent(src, node,
  

•                                                   "reg", 0, &size,
  

•                                                   false);
  

•         if (addr == FDT_ADDR_T_NONE) {
  

•                 debug("failed to read address/size for %s\n", name);
  

•                 continue;
  

•         }
  

•         strncpy(basename, name, max_len);
  

•         temp = strchr(basename, '@');
  

•         if (temp) {
  

•                 bname_len = strnlen(basename, max_len) - strnlen(temp,
  

•                                                                max_len);
  

•                 *(basename + bname_len) = '\0';
  

•         }
  

•         pmp_mem.start = addr;
  

•         pmp_mem.end = addr + size - 1;
  

•         err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
  

•                                          &phandle);
  

•         if (err < 0) {
  

•                 printf("failed to add reserved memory: %d\n", err);
  

•                 return err;
  

•         }
  

•         if (!fdt_getprop(src, node, "no-map", NULL))
  

•                 nomap = false;
  

•         if (nomap) {
  

•                 rmem_offset = fdt_node_offset_by_phandle(dst, phandle);
  

•                 fdt_setprop_empty(dst, rmem_offset, "no-map");
  

•         }
  

• }
  

• return 0;
  

+}

Same here.

Best regards

Heinrich

...

+int riscv_board_reserved_mem_fixup(void *fdt) +{

• int err;
  

• void *src_fdt_addr;
  

• src_fdt_addr = map_sysmem(gd->arch.firmware_fdt_addr, 0);
  

• err = riscv_fdt_copy_resv_mem_node(src_fdt_addr, fdt);
  

• if (err < 0)
  

•         return err;
  

• return 0;
  

+}

On Mon, Mar 23, 2020 at 11:23 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 3/24/20 5:16 AM, Atish Patra wrote:

...

The DT used by U-Boot may be different from the DT being passed to the OS if the DT is loaded from external media such as network or mmc. In that case, the reserved-memory node needs to be copied to the DT passed to the OS.

The bootefi command works in the following sequence:

• copy the passed fdt of if none is passed the internal fdt
• call image_setup_libfdt() for the copy
• create memory reservations in the UEFI memory map

Cf. fef907b2e440 ("efi_loader: create reservations after ft_board_setup") 7be64b885a36 ("cmd: bootefi: Parse reserved-memory node from DT")

image_setup_libfdt() executes among others:

• fdt_chosen()
• arch_fixup_fdt()

Please, explain why you need to copy memory reservations, if arch_fixup_fdt() is executed inside the bootefi command:

As per my understanding, there can be two different DTs in U-Boot. 1. Internal device tree or DT passed from previous stage (gd->fdt_blob). This device tree is being used to boot OS as well. 2. User can load the DT from MMC/network to any valid address and use that address in bootefi/booti command line to boot OS.

efi_install_fdt copies DT from the given user address (via cmdline) and copies to the efi memory.

There is no need to 2nd copy for case 1 as the DT already contains the reserved-memory node. In case 2, the DT loaded from external media may not have the reserved memory regions set by the firmware. That's why we need to copy the reserved-memory nodes. Now, arch_fixup_fdt() is the last place to edit the DT passed to OS. The copy operation has to be done in arch_fixup_fdt. For case 1, fdtdec_add_reserved_memory will simply return without modifying the DT.

What would be the source of memory reservation that you would otherwise miss in the final device tree? Do you expect that the Linux device tree would lack reservation that are generated on the fly before U-Boot?

Best regards

Heinrich

...

Signed-off-by: Atish Patra atish.patra@wdc.com Reviewed-by: Bin Meng bmeng.cn@gmail.com

---

arch/riscv/lib/bootm.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c index 87cadad5016d..8ff8db6bf533 100644 --- a/arch/riscv/lib/bootm.c +++ b/arch/riscv/lib/bootm.c @@ -28,8 +28,8 @@ __weak void board_quiesce_devices(void)

int arch_fixup_fdt(void *blob) { -#ifdef CONFIG_EFI_LOADER int err; +#ifdef CONFIG_EFI_LOADER u32 size; int chosen_offset;

@@ -50,6 +50,12 @@ int arch_fixup_fdt(void *blob) /* Overwrite the boot-hartid as U-Boot is the last stage BL */ fdt_setprop_u32(blob, chosen_offset, "boot-hartid", gd->arch.boot_hart); #endif

• /* Copy the reserved-memory node to the DT used by OS */
  

• err = riscv_fdt_copy_resv_mem_node(gd->fdt_blob, blob);
  

• if (err < 0)
  

•         return err;
  

• return 0;
  

  }

On Tue, Mar 24, 2020 at 3:11 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 3/24/20 8:15 AM, Atish Patra wrote:

...

On Mon, Mar 23, 2020 at 11:23 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 3/24/20 5:16 AM, Atish Patra wrote:

...

The DT used by U-Boot may be different from the DT being passed to the OS if the DT is loaded from external media such as network or mmc. In that case, the reserved-memory node needs to be copied to the DT passed to the OS.

The bootefi command works in the following sequence:

• copy the passed fdt of if none is passed the internal fdt
• call image_setup_libfdt() for the copy
• create memory reservations in the UEFI memory map

Cf. fef907b2e440 ("efi_loader: create reservations after ft_board_setup") 7be64b885a36 ("cmd: bootefi: Parse reserved-memory node from DT")

image_setup_libfdt() executes among others:

• fdt_chosen()
• arch_fixup_fdt()

Please, explain why you need to copy memory reservations, if arch_fixup_fdt() is executed inside the bootefi command:

As per my understanding, there can be two different DTs in U-Boot.

1. Internal device tree or DT passed from previous stage (gd->fdt_blob). This device tree is being used to boot OS as well.
2. User can load the DT from MMC/network to any valid address and use

that address in bootefi/booti command line to boot OS.

efi_install_fdt copies DT from the given user address (via cmdline) and copies to the efi memory.

There is no need to 2nd copy for case 1 as the DT already contains the reserved-memory node. In case 2, the DT loaded from external media may not have the reserved memory regions set by the firmware. That's why we need to copy the reserved-memory nodes. Now, arch_fixup_fdt() is the last place to edit the DT passed to OS. The copy operation has to be done in arch_fixup_fdt. For case 1, fdtdec_add_reserved_memory will simply return without modifying the DT.

There are three cases of device trees:

• U-Boot's internal device trees
• device trees provided by a earlier stage firmware to U-Boot (e.g. on QEMU)
• device trees supplied by the user

Yes. I combined 1st & 2nd case into one.

We cannot use U-Boot's internal device tree directly because a program called via bootefi may manipulate the device tree and return afterwards.

As arch_fixup_fdt is called by all boot commands there is no need to insert reserved memory nodes at an earlier stage.

The copy operation is only needed for bootefi as the other boot commands cannot return. So it does not make to me to move the device tree copying to arch_fixup_fdt().

This series is aimed at providing reserved memory support all boot commands in RISC-V. All other boot commands booti/bootm also load the device tree supplied by the user. Hence, we need to copy the reserved memory nodes so that proper kernel sees those DT nodes.

The code path for booti/bootm is image_setup_linux->image_setup_libfdt->arch_fixup_fdt

This is executed while executing booti/bootm and fixes the DT that passed to the OS.

Best regards

Heinrich

...

...

What would be the source of memory reservation that you would otherwise miss in the final device tree? Do you expect that the Linux device tree would lack reservation that are generated on the fly before U-Boot?

Best regards

Heinrich

...

Signed-off-by: Atish Patra atish.patra@wdc.com Reviewed-by: Bin Meng bmeng.cn@gmail.com

---

arch/riscv/lib/bootm.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c index 87cadad5016d..8ff8db6bf533 100644 --- a/arch/riscv/lib/bootm.c +++ b/arch/riscv/lib/bootm.c @@ -28,8 +28,8 @@ __weak void board_quiesce_devices(void)

int arch_fixup_fdt(void *blob) { -#ifdef CONFIG_EFI_LOADER int err; +#ifdef CONFIG_EFI_LOADER u32 size; int chosen_offset;

@@ -50,6 +50,12 @@ int arch_fixup_fdt(void *blob) /* Overwrite the boot-hartid as U-Boot is the last stage BL */ fdt_setprop_u32(blob, chosen_offset, "boot-hartid", gd->arch.boot_hart); #endif

• /* Copy the reserved-memory node to the DT used by OS */
  

• err = riscv_fdt_copy_resv_mem_node(gd->fdt_blob, blob);
  

• if (err < 0)
  

•         return err;
  

•  return 0;
  

  }