| Message ID | 20230323163347.182895-3-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for dtb mapping | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be fixes |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 1 and now 1 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 18 this patch: 18 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 19 this patch: 19 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 38 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Thu, Mar 23, 2023 at 05:33:47PM +0100, Alexandre Ghiti wrote: > Now that the dtb early mapping was moved in the fixmap region, we can > keep using this address since it is present in swapper_pg_dir, so remove > the dtb relocation which was wrong anyway since early_memremap is > restricted to 256K whereas the maximum fdt size is 2MB. I feel bad making this comment since only one of us is a native speaker, but for the future would you mind breaking up overly long sentences like the above? Say: Now that the dtb early mapping was moved in the fixmap region, we can keep using this address since it is present in swapper_pg_dir, and remove the dtb relocation. The relocation was wrong anyway since early_memremap() is restricted to 256K whereas the maximum fdt size is 2MB. > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > arch/riscv/kernel/setup.c | 5 +---- > arch/riscv/mm/init.c | 21 ++------------------- > 2 files changed, 3 insertions(+), 23 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 542eed85ad2c..a059b73f4ddb 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p) > #if IS_ENABLED(CONFIG_BUILTIN_DTB) > unflatten_and_copy_device_tree(); > #else > - if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) btw, how come it is safe now to drop this? This feels like a separate change that should be its own commit, no? Cheers, Conor. > - unflatten_device_tree(); > - else > - pr_err("No DTB found in kernel mappings\n"); > + unflatten_device_tree(); > #endif > misc_mem_init(); > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > index fb78d6bbabae..0f14f4a8d179 100644 > --- a/arch/riscv/mm/init.c > +++ b/arch/riscv/mm/init.c > @@ -249,25 +249,8 @@ static void __init setup_bootmem(void) > * early_init_fdt_reserve_self() since __pa() does > * not work for DTB pointers that are fixmap addresses > */ > - if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) { > - /* > - * In case the DTB is not located in a memory region we won't > - * be able to locate it later on via the linear mapping and > - * get a segfault when accessing it via __va(dtb_early_pa). > - * To avoid this situation copy DTB to a memory region. > - * Note that memblock_phys_alloc will also reserve DTB region. > - */ > - if (!memblock_is_memory(dtb_early_pa)) { > - size_t fdt_size = fdt_totalsize(dtb_early_va); > - phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE); > - void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size); > - > - memcpy(new_dtb_early_va, dtb_early_va, fdt_size); > - early_memunmap(new_dtb_early_va, fdt_size); > - _dtb_early_pa = new_dtb_early_pa; > - } else > - memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va)); > - } > + if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) > + memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va)); > > dma_contiguous_reserve(dma32_phys_limit); > if (IS_ENABLED(CONFIG_64BIT)) > -- > 2.37.2 > >
On 3/27/23 13:16, Conor Dooley wrote: > On Thu, Mar 23, 2023 at 05:33:47PM +0100, Alexandre Ghiti wrote: >> Now that the dtb early mapping was moved in the fixmap region, we can >> keep using this address since it is present in swapper_pg_dir, so remove >> the dtb relocation which was wrong anyway since early_memremap is >> restricted to 256K whereas the maximum fdt size is 2MB. > I feel bad making this comment since only one of us is a native speaker, > but for the future would you mind breaking up overly long sentences like > the above? Say: > > Now that the dtb early mapping was moved in the fixmap region, we can > keep using this address since it is present in swapper_pg_dir, and > remove the dtb relocation. > The relocation was wrong anyway since early_memremap() is restricted to > 256K whereas the maximum fdt size is 2MB. Sure! >> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> >> --- >> arch/riscv/kernel/setup.c | 5 +---- >> arch/riscv/mm/init.c | 21 ++------------------- >> 2 files changed, 3 insertions(+), 23 deletions(-) >> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> index 542eed85ad2c..a059b73f4ddb 100644 >> --- a/arch/riscv/kernel/setup.c >> +++ b/arch/riscv/kernel/setup.c >> @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p) >> #if IS_ENABLED(CONFIG_BUILTIN_DTB) >> unflatten_and_copy_device_tree(); >> #else >> - if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > btw, how come it is safe now to drop this? This feels like a separate > change that should be its own commit, no? It is safe because early_init_dt_verify is already called in parse_dtb and since the dtb address does not change anymore, no need to reset initial_boot_params. So I'll split this one, thanks. > > Cheers, > Conor. > >> - unflatten_device_tree(); >> - else >> - pr_err("No DTB found in kernel mappings\n"); >> + unflatten_device_tree(); >> #endif >> misc_mem_init(); >> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c >> index fb78d6bbabae..0f14f4a8d179 100644 >> --- a/arch/riscv/mm/init.c >> +++ b/arch/riscv/mm/init.c >> @@ -249,25 +249,8 @@ static void __init setup_bootmem(void) >> * early_init_fdt_reserve_self() since __pa() does >> * not work for DTB pointers that are fixmap addresses >> */ >> - if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) { >> - /* >> - * In case the DTB is not located in a memory region we won't >> - * be able to locate it later on via the linear mapping and >> - * get a segfault when accessing it via __va(dtb_early_pa). >> - * To avoid this situation copy DTB to a memory region. >> - * Note that memblock_phys_alloc will also reserve DTB region. >> - */ >> - if (!memblock_is_memory(dtb_early_pa)) { >> - size_t fdt_size = fdt_totalsize(dtb_early_va); >> - phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE); >> - void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size); >> - >> - memcpy(new_dtb_early_va, dtb_early_va, fdt_size); >> - early_memunmap(new_dtb_early_va, fdt_size); >> - _dtb_early_pa = new_dtb_early_pa; >> - } else >> - memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va)); >> - } >> + if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) >> + memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va)); >> >> dma_contiguous_reserve(dma32_phys_limit); >> if (IS_ENABLED(CONFIG_64BIT)) >> -- >> 2.37.2 >> >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Mar 29, 2023 at 10:09:13AM +0200, Alexandre Ghiti wrote: > On 3/27/23 13:16, Conor Dooley wrote: > > On Thu, Mar 23, 2023 at 05:33:47PM +0100, Alexandre Ghiti wrote: > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > > index 542eed85ad2c..a059b73f4ddb 100644 > > > --- a/arch/riscv/kernel/setup.c > > > +++ b/arch/riscv/kernel/setup.c > > > @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p) > > > #if IS_ENABLED(CONFIG_BUILTIN_DTB) > > > unflatten_and_copy_device_tree(); > > > #else > > > - if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > > > btw, how come it is safe now to drop this? This feels like a separate > > change that should be its own commit, no? > > > It is safe because early_init_dt_verify is already called in parse_dtb and Yah, that's what I figured. Cool. > since the dtb address does not change anymore, no need to reset > initial_boot_params. So I'll split this one, thanks. Worth noting the point at which this became redundant in your commit message when you do. Thanks, Conor.
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 542eed85ad2c..a059b73f4ddb 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -278,10 +278,7 @@ void __init setup_arch(char **cmdline_p) #if IS_ENABLED(CONFIG_BUILTIN_DTB) unflatten_and_copy_device_tree(); #else - if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) - unflatten_device_tree(); - else - pr_err("No DTB found in kernel mappings\n"); + unflatten_device_tree(); #endif misc_mem_init(); diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index fb78d6bbabae..0f14f4a8d179 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -249,25 +249,8 @@ static void __init setup_bootmem(void) * early_init_fdt_reserve_self() since __pa() does * not work for DTB pointers that are fixmap addresses */ - if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) { - /* - * In case the DTB is not located in a memory region we won't - * be able to locate it later on via the linear mapping and - * get a segfault when accessing it via __va(dtb_early_pa). - * To avoid this situation copy DTB to a memory region. - * Note that memblock_phys_alloc will also reserve DTB region. - */ - if (!memblock_is_memory(dtb_early_pa)) { - size_t fdt_size = fdt_totalsize(dtb_early_va); - phys_addr_t new_dtb_early_pa = memblock_phys_alloc(fdt_size, PAGE_SIZE); - void *new_dtb_early_va = early_memremap(new_dtb_early_pa, fdt_size); - - memcpy(new_dtb_early_va, dtb_early_va, fdt_size); - early_memunmap(new_dtb_early_va, fdt_size); - _dtb_early_pa = new_dtb_early_pa; - } else - memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va)); - } + if (!IS_ENABLED(CONFIG_BUILTIN_DTB)) + memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va)); dma_contiguous_reserve(dma32_phys_limit); if (IS_ENABLED(CONFIG_64BIT))
Now that the dtb early mapping was moved in the fixmap region, we can keep using this address since it is present in swapper_pg_dir, so remove the dtb relocation which was wrong anyway since early_memremap is restricted to 256K whereas the maximum fdt size is 2MB. Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- arch/riscv/kernel/setup.c | 5 +---- arch/riscv/mm/init.c | 21 ++------------------- 2 files changed, 3 insertions(+), 23 deletions(-)