| Message ID | tencent_D656D683705F41324657ED3393C3384C7408@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: move memblock_allow_resize() after lm is ready | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD d5e45e810e0e |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 6 and now 6 |
| 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: 8 this patch: 8 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| 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, 16 lines checked |
| 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 Sat, Jun 10, 2023 at 07:43:20AM +0800, Woody Zhang wrote: $subject: "riscv: move memblock_allow_resize() after lm is ready" s/lm/linear map/ if you resubmit, please. > The initial memblock metadata is accessed from kernel image mapping. The > regions arrays need to "reallocated" from memblock and accessed through > linear mapping to cover more memblock regions. So the resizing should > not be allowed until linear mapping is ready. Note that there are > memblock allocations when building linear mapping. > > Signed-off-by: Woody Zhang <woodylab@foxmail.com> What commit does this fix? Cheers, Conor.
On Sat, Jun 10, 2023 at 04:44:19PM +0100, Conor Dooley wrote: >On Sat, Jun 10, 2023 at 07:43:20AM +0800, Woody Zhang wrote: >$subject: "riscv: move memblock_allow_resize() after lm is ready" > >s/lm/linear map/ if you resubmit, please. Thanks, will send a v2. > >> The initial memblock metadata is accessed from kernel image mapping. The >> regions arrays need to "reallocated" from memblock and accessed through >> linear mapping to cover more memblock regions. So the resizing should >> not be allowed until linear mapping is ready. Note that there are >> memblock allocations when building linear mapping. >> >> Signed-off-by: Woody Zhang <woodylab@foxmail.com> > >What commit does this fix? Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") Will add this tag in v2. Woody
On Sun, Jun 11, 2023 at 07:31:03AM +0800, Woody Zhang wrote: > On Sat, Jun 10, 2023 at 04:44:19PM +0100, Conor Dooley wrote: > >On Sat, Jun 10, 2023 at 07:43:20AM +0800, Woody Zhang wrote: > >$subject: "riscv: move memblock_allow_resize() after lm is ready" > > > >s/lm/linear map/ if you resubmit, please. > > Thanks, will send a v2. > > > > >> The initial memblock metadata is accessed from kernel image mapping. The > >> regions arrays need to "reallocated" from memblock and accessed through > >> linear mapping to cover more memblock regions. So the resizing should > >> not be allowed until linear mapping is ready. Note that there are > >> memblock allocations when building linear mapping. > >> > >> Signed-off-by: Woody Zhang <woodylab@foxmail.com> > > > >What commit does this fix? > > Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") > > Will add this tag in v2. b4 should pick up fixes tags from replies, you shouldn't need to resubmit for that alone. Cheers, Conor.
Hi Woody, On Sat, Jun 10, 2023 at 1:49 AM Woody Zhang <woodylab@foxmail.com> wrote: > > The initial memblock metadata is accessed from kernel image mapping. The > regions arrays need to "reallocated" from memblock and accessed through > linear mapping to cover more memblock regions. So the resizing should > not be allowed until linear mapping is ready. Note that there are > memblock allocations when building linear mapping. > > Signed-off-by: Woody Zhang <woodylab@foxmail.com> > --- > arch/riscv/mm/init.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > index 9e9da69720ce..8a33ecbb4d0f 100644 > --- a/arch/riscv/mm/init.c > +++ b/arch/riscv/mm/init.c > @@ -258,7 +258,6 @@ static void __init setup_bootmem(void) > dma_contiguous_reserve(dma32_phys_limit); > if (IS_ENABLED(CONFIG_64BIT)) > hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT); > - memblock_allow_resize(); > } > > #ifdef CONFIG_MMU > @@ -1250,6 +1249,9 @@ static void __init setup_vm_final(void) > csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | satp_mode); > local_flush_tlb_all(); > > + /* Depend on that Linear Mapping is ready */ > + memblock_allow_resize(); > + > pt_ops_set_late(); > } > #else > -- > 2.39.2 > The commit log does not describe the issue thoroughly enough to me, maybe you could point to the arm64 commit that did the same? I mean commit 24cc61d8cb5a ("arm64: memblock: don't permit memblock resizing until linear mapping is up"). Another point is that I would not put this call into setup_vm_final(), I'd rather add it in paging_init() as it does not seem like a good fit for setup_vm_final(). But that's a nit so up to you of course. Anyway, that's a good catch, thanks! Alex
在 2023/6/12 15:15, Alexandre Ghiti 写道: > Hi Woody, > > On Sat, Jun 10, 2023 at 1:49 AM Woody Zhang <woodylab@foxmail.com> wrote: >> >> The initial memblock metadata is accessed from kernel image mapping. The >> regions arrays need to "reallocated" from memblock and accessed through >> linear mapping to cover more memblock regions. So the resizing should >> not be allowed until linear mapping is ready. Note that there are >> memblock allocations when building linear mapping. >> >> Signed-off-by: Woody Zhang <woodylab@foxmail.com> >> --- >> arch/riscv/mm/init.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c >> index 9e9da69720ce..8a33ecbb4d0f 100644 >> --- a/arch/riscv/mm/init.c >> +++ b/arch/riscv/mm/init.c >> @@ -258,7 +258,6 @@ static void __init setup_bootmem(void) >> dma_contiguous_reserve(dma32_phys_limit); >> if (IS_ENABLED(CONFIG_64BIT)) >> hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT); >> - memblock_allow_resize(); >> } >> >> #ifdef CONFIG_MMU >> @@ -1250,6 +1249,9 @@ static void __init setup_vm_final(void) >> csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | satp_mode); >> local_flush_tlb_all(); >> >> + /* Depend on that Linear Mapping is ready */ >> + memblock_allow_resize(); >> + >> pt_ops_set_late(); >> } >> #else >> -- >> 2.39.2 >> > > The commit log does not describe the issue thoroughly enough to me, > maybe you could point to the arm64 commit that did the same? I mean > commit 24cc61d8cb5a ("arm64: memblock: don't permit memblock resizing > until linear mapping is up"). @Alex I reproduced the problem as the arm64 commit describes. You can find the complete log via this link: https://termbin.com/bx0o I constructed the dtb with numerous discrete /memreserve/ regions (the numbers of these regions approximate INIT_MEMBLOCK_REGIONS) which full the reserved regions up. When memblock_allow_resize was set, the calling of memblock_reserve() would double/resize the reserved regions and do the __memcopy() from the old regions ( mapped by kernel) to the new ones (provided by __va()). But before the linear mapping was ready (like: during the creating of linear mapping), memblock_reserve() was called and the memcopy would trigger a Store/AMO page fault. > > Another point is that I would not put this call into setup_vm_final(), > I'd rather add it in paging_init() as it does not seem like a good fit > for setup_vm_final(). But that's a nit so up to you of course. > I agree. @Woody I noticed your V2 [1] didn't take this suggestion, maybe you can take it at V3. And it will be more sound if you supplement the commit-msg with the panic info from the log. [1]: https://lore.kernel.org/linux-riscv/tencent_97F6C19BAF7C99B22BB1199FE16B266C2B07@qq.com/ > Anyway, that's a good catch, thanks! > > Alex > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Jun 14, 2023 at 05:51:23PM +0800, Song Shuai wrote: > > >在 2023/6/12 15:15, Alexandre Ghiti 写道: >> Hi Woody, >> >> On Sat, Jun 10, 2023 at 1:49 AM Woody Zhang <woodylab@foxmail.com> wrote: >> > >> > The initial memblock metadata is accessed from kernel image mapping. The >> > regions arrays need to "reallocated" from memblock and accessed through >> > linear mapping to cover more memblock regions. So the resizing should >> > not be allowed until linear mapping is ready. Note that there are >> > memblock allocations when building linear mapping. >> > >> > Signed-off-by: Woody Zhang <woodylab@foxmail.com> >> > --- >> > arch/riscv/mm/init.c | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c >> > index 9e9da69720ce..8a33ecbb4d0f 100644 >> > --- a/arch/riscv/mm/init.c >> > +++ b/arch/riscv/mm/init.c >> > @@ -258,7 +258,6 @@ static void __init setup_bootmem(void) >> > dma_contiguous_reserve(dma32_phys_limit); >> > if (IS_ENABLED(CONFIG_64BIT)) >> > hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT); >> > - memblock_allow_resize(); >> > } >> > >> > #ifdef CONFIG_MMU >> > @@ -1250,6 +1249,9 @@ static void __init setup_vm_final(void) >> > csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | satp_mode); >> > local_flush_tlb_all(); >> > >> > + /* Depend on that Linear Mapping is ready */ >> > + memblock_allow_resize(); >> > + >> > pt_ops_set_late(); >> > } >> > #else >> > -- >> > 2.39.2 >> > >> >> The commit log does not describe the issue thoroughly enough to me, >> maybe you could point to the arm64 commit that did the same? I mean >> commit 24cc61d8cb5a ("arm64: memblock: don't permit memblock resizing >> until linear mapping is up"). >@Alex > >I reproduced the problem as the arm64 commit describes. >You can find the complete log via this link: https://termbin.com/bx0o > >I constructed the dtb with numerous discrete /memreserve/ regions >(the numbers of these regions approximate INIT_MEMBLOCK_REGIONS) which full >the reserved regions up. > >When memblock_allow_resize was set, the calling of memblock_reserve() would >double/resize the reserved regions and do the __memcopy() from the old >regions ( mapped by kernel) >to the new ones (provided by __va()). >But before the linear mapping was ready (like: during the creating of linear >mapping), >memblock_reserve() was called and the memcopy would trigger a Store/AMO page >fault. > >> >> Another point is that I would not put this call into setup_vm_final(), >> I'd rather add it in paging_init() as it does not seem like a good fit >> for setup_vm_final(). But that's a nit so up to you of course. >> >I agree. > >@Woody > >I noticed your V2 [1] didn't take this suggestion, maybe you can take it at >V3. >And it will be more sound if you supplement the commit-msg with the panic >info from the log. > Thanks for your comments. I will update it with your panic log in next version. Woody
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index 9e9da69720ce..8a33ecbb4d0f 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -258,7 +258,6 @@ static void __init setup_bootmem(void) dma_contiguous_reserve(dma32_phys_limit); if (IS_ENABLED(CONFIG_64BIT)) hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT); - memblock_allow_resize(); } #ifdef CONFIG_MMU @@ -1250,6 +1249,9 @@ static void __init setup_vm_final(void) csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | satp_mode); local_flush_tlb_all(); + /* Depend on that Linear Mapping is ready */ + memblock_allow_resize(); + pt_ops_set_late(); } #else
The initial memblock metadata is accessed from kernel image mapping. The regions arrays need to "reallocated" from memblock and accessed through linear mapping to cover more memblock regions. So the resizing should not be allowed until linear mapping is ready. Note that there are memblock allocations when building linear mapping. Signed-off-by: Woody Zhang <woodylab@foxmail.com> --- arch/riscv/mm/init.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)