[PATCH] riscv: move memblock_allow_resize() after lm is ready

[PATCH] riscv: move memblock_allow_resize() after lm is ready

[Woody Zhang]

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

Re: [PATCH] riscv: move memblock_allow_resize() after lm is ready

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] riscv: move memblock_allow_resize() after lm is ready

[Woody Zhang]

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

Re: [PATCH] riscv: move memblock_allow_resize() after lm is ready

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] riscv: move memblock_allow_resize() after lm is ready

[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").

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

Re: [PATCH] riscv: move memblock_allow_resize() after lm is ready

[Song Shuai]

在 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

-- 
Thanks
Song Shuai

Re: [PATCH] riscv: move memblock_allow_resize() after lm is ready

[Woody Zhang]

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