| Message ID | 20200204111947.52013-1-zong.li@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: force hart_lottery to put in .sdata section | expand |
On Tue, Feb 4, 2020 at 4:49 PM Zong Li <zong.li@sifive.com> wrote: > > In PIC code model, the zero initialized data always be put in .bss > section, so when building kernel as PIE, the hart_lottery won't present > in small data section, and it causes more than one harts to get the > lottery, because the main hart clears the content of .bss section > immediately after it getting the lottery. > > Signed-off-by: Zong Li <zong.li@sifive.com> > --- > arch/riscv/kernel/setup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 22b671dbbcf1..45c63dc06360 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -40,7 +40,7 @@ struct screen_info screen_info = { > #endif > > /* The lucky hart to first increment this variable will boot the other cores */ > -atomic_t hart_lottery; > +atomic_t hart_lottery __section(.sdata); > unsigned long boot_cpu_hartid; > > void __init parse_dtb(void) > -- > 2.25.0 > > Looks good to me. Please ensure that it is tested with both RV32 and RV64. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup
On Tue, Feb 4, 2020 at 7:40 PM Anup Patel <anup@brainfault.org> wrote: > > On Tue, Feb 4, 2020 at 4:49 PM Zong Li <zong.li@sifive.com> wrote: > > > > In PIC code model, the zero initialized data always be put in .bss > > section, so when building kernel as PIE, the hart_lottery won't present > > in small data section, and it causes more than one harts to get the > > lottery, because the main hart clears the content of .bss section > > immediately after it getting the lottery. > > > > Signed-off-by: Zong Li <zong.li@sifive.com> > > --- > > arch/riscv/kernel/setup.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index 22b671dbbcf1..45c63dc06360 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -40,7 +40,7 @@ struct screen_info screen_info = { > > #endif > > > > /* The lucky hart to first increment this variable will boot the other cores */ > > -atomic_t hart_lottery; > > +atomic_t hart_lottery __section(.sdata); > > unsigned long boot_cpu_hartid; > > > > void __init parse_dtb(void) > > -- > > 2.25.0 > > > > > > Looks good to me. Please ensure that it is tested with both > RV32 and RV64. > I had tested it on RV32 and RV64, it works on both. > Reviewed-by: Anup Patel <anup@brainfault.org> > > Regards, > Anup
Hi Zong, On 2/4/20 6:19 AM, Zong Li wrote: > In PIC code model, the zero initialized data always be put in .bss > section, so when building kernel as PIE, the hart_lottery won't present > in small data section, and it causes more than one harts to get the > lottery, because the main hart clears the content of .bss section > immediately after it getting the lottery. > > Signed-off-by: Zong Li <zong.li@sifive.com> > --- > arch/riscv/kernel/setup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 22b671dbbcf1..45c63dc06360 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -40,7 +40,7 @@ struct screen_info screen_info = { > #endif > > /* The lucky hart to first increment this variable will boot the other cores */ > -atomic_t hart_lottery; > +atomic_t hart_lottery __section(.sdata); > unsigned long boot_cpu_hartid; > > void __init parse_dtb(void) > Thanks for testing the relocatable patch in SMP and fixing this issue. It looks good to me too as I did not find any other variable being used before bss clearing so feel free to add: Reviewed-by: Alexandre Ghiti <alex@ghiti.fr>
On Wed, 05 Feb 2020 03:07:52 PST (-0800), zong.li@sifive.com wrote: > On Tue, Feb 4, 2020 at 7:40 PM Anup Patel <anup@brainfault.org> wrote: >> >> On Tue, Feb 4, 2020 at 4:49 PM Zong Li <zong.li@sifive.com> wrote: >> > >> > In PIC code model, the zero initialized data always be put in .bss >> > section, so when building kernel as PIE, the hart_lottery won't present >> > in small data section, and it causes more than one harts to get the >> > lottery, because the main hart clears the content of .bss section >> > immediately after it getting the lottery. >> > >> > Signed-off-by: Zong Li <zong.li@sifive.com> >> > --- >> > arch/riscv/kernel/setup.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> > index 22b671dbbcf1..45c63dc06360 100644 >> > --- a/arch/riscv/kernel/setup.c >> > +++ b/arch/riscv/kernel/setup.c >> > @@ -40,7 +40,7 @@ struct screen_info screen_info = { >> > #endif >> > >> > /* The lucky hart to first increment this variable will boot the other cores */ >> > -atomic_t hart_lottery; >> > +atomic_t hart_lottery __section(.sdata); >> > unsigned long boot_cpu_hartid; >> > >> > void __init parse_dtb(void) >> > -- >> > 2.25.0 >> > >> > >> >> Looks good to me. Please ensure that it is tested with both >> RV32 and RV64. >> > > I had tested it on RV32 and RV64, it works on both. Can you be more specific about your setup? Before you patch, hart_lottery should be in .sbss, which we put inside .sdata. I'm a bit worried there's some other issue going on here that this is just masking. That said, putting sbss in sdata seems like a generally bad idea so I'd be in favor of getting rid of it. I've queued this up onto for-next, as even if it is fixing a bug related to the relocatable kernels we don't have support for that yet. I've also sent a patch that stops putting .sbss into .sdata. Thanks! >> Reviewed-by: Anup Patel <anup@brainfault.org> >> >> Regards, >> Anup
On Wed, Feb 19, 2020 at 5:26 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Wed, 05 Feb 2020 03:07:52 PST (-0800), zong.li@sifive.com wrote: > > On Tue, Feb 4, 2020 at 7:40 PM Anup Patel <anup@brainfault.org> wrote: > >> > >> On Tue, Feb 4, 2020 at 4:49 PM Zong Li <zong.li@sifive.com> wrote: > >> > > >> > In PIC code model, the zero initialized data always be put in .bss > >> > section, so when building kernel as PIE, the hart_lottery won't present > >> > in small data section, and it causes more than one harts to get the > >> > lottery, because the main hart clears the content of .bss section > >> > immediately after it getting the lottery. > >> > > >> > Signed-off-by: Zong Li <zong.li@sifive.com> > >> > --- > >> > arch/riscv/kernel/setup.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > >> > index 22b671dbbcf1..45c63dc06360 100644 > >> > --- a/arch/riscv/kernel/setup.c > >> > +++ b/arch/riscv/kernel/setup.c > >> > @@ -40,7 +40,7 @@ struct screen_info screen_info = { > >> > #endif > >> > > >> > /* The lucky hart to first increment this variable will boot the other cores */ > >> > -atomic_t hart_lottery; > >> > +atomic_t hart_lottery __section(.sdata); > >> > unsigned long boot_cpu_hartid; > >> > > >> > void __init parse_dtb(void) > >> > -- > >> > 2.25.0 > >> > > >> > > >> > >> Looks good to me. Please ensure that it is tested with both > >> RV32 and RV64. > >> > > > > I had tested it on RV32 and RV64, it works on both. > > Can you be more specific about your setup? Before you patch, hart_lottery I applied the Alex's CONFIG_RELOCATABLE patch (https://patchwork.kernel.org/patch/11349207/) on Linux 5.6-rc1, and selected the RELOCATABLE in Kconfig by manual, the firmware was using OpenSBI, then ran the Linux kernel on QEMU. (I ran the RV64 on sifive_u machine and RV32 on virt machine). > should be in .sbss, which we put inside .sdata. I'm a bit worried there's some > other issue going on here that this is just masking. That said, putting sbss > in sdata seems like a generally bad idea so I'd be in favor of getting rid of > it. It happened at the beginning, I would help to give a quick test the change you made. > > I've queued this up onto for-next, as even if it is fixing a bug related to the > relocatable kernels we don't have support for that yet. I've also sent a patch > that stops putting .sbss into .sdata. > > Thanks! > > >> Reviewed-by: Anup Patel <anup@brainfault.org> > >> > >> Regards, > >> Anup
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 22b671dbbcf1..45c63dc06360 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -40,7 +40,7 @@ struct screen_info screen_info = { #endif /* The lucky hart to first increment this variable will boot the other cores */ -atomic_t hart_lottery; +atomic_t hart_lottery __section(.sdata); unsigned long boot_cpu_hartid; void __init parse_dtb(void)
In PIC code model, the zero initialized data always be put in .bss section, so when building kernel as PIE, the hart_lottery won't present in small data section, and it causes more than one harts to get the lottery, because the main hart clears the content of .bss section immediately after it getting the lottery. Signed-off-by: Zong Li <zong.li@sifive.com> --- arch/riscv/kernel/setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)