linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] riscv: force hart_lottery to put in .sdata section
@ 2020-02-04 11:19 Zong Li
  2020-02-04 11:39 ` Anup Patel
  2020-02-10  6:42 ` Alex Ghiti
  0 siblings, 2 replies; 6+ messages in thread
From: Zong Li @ 2020-02-04 11:19 UTC (permalink / raw)
  To: paul.walmsley, palmer, aou, linux-riscv, linux-kernel; +Cc: Zong Li

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: force hart_lottery to put in .sdata section
  2020-02-04 11:19 [PATCH] riscv: force hart_lottery to put in .sdata section Zong Li
@ 2020-02-04 11:39 ` Anup Patel
  2020-02-05 11:07   ` Zong Li
  2020-02-10  6:42 ` Alex Ghiti
  1 sibling, 1 reply; 6+ messages in thread
From: Anup Patel @ 2020-02-04 11:39 UTC (permalink / raw)
  To: Zong Li
  Cc: linux-riscv, Albert Ou, Palmer Dabbelt,
	linux-kernel@vger.kernel.org List, Paul Walmsley

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: force hart_lottery to put in .sdata section
  2020-02-04 11:39 ` Anup Patel
@ 2020-02-05 11:07   ` Zong Li
  2020-02-18 21:26     ` Palmer Dabbelt
  0 siblings, 1 reply; 6+ messages in thread
From: Zong Li @ 2020-02-05 11:07 UTC (permalink / raw)
  To: Anup Patel
  Cc: linux-riscv, Albert Ou, Palmer Dabbelt,
	linux-kernel@vger.kernel.org List, Paul Walmsley

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: force hart_lottery to put in .sdata section
  2020-02-04 11:19 [PATCH] riscv: force hart_lottery to put in .sdata section Zong Li
  2020-02-04 11:39 ` Anup Patel
@ 2020-02-10  6:42 ` Alex Ghiti
  1 sibling, 0 replies; 6+ messages in thread
From: Alex Ghiti @ 2020-02-10  6:42 UTC (permalink / raw)
  To: Zong Li, paul.walmsley, palmer, aou, linux-riscv, linux-kernel

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>


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: force hart_lottery to put in .sdata section
  2020-02-05 11:07   ` Zong Li
@ 2020-02-18 21:26     ` Palmer Dabbelt
  2020-02-19  8:48       ` Zong Li
  0 siblings, 1 reply; 6+ messages in thread
From: Palmer Dabbelt @ 2020-02-18 21:26 UTC (permalink / raw)
  To: zong.li; +Cc: anup, linux-riscv, aou, linux-kernel, Paul Walmsley

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: force hart_lottery to put in .sdata section
  2020-02-18 21:26     ` Palmer Dabbelt
@ 2020-02-19  8:48       ` Zong Li
  0 siblings, 0 replies; 6+ messages in thread
From: Zong Li @ 2020-02-19  8:48 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Anup Patel, linux-riscv, Albert Ou,
	linux-kernel@vger.kernel.org List, Paul Walmsley

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-19  8:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 11:19 [PATCH] riscv: force hart_lottery to put in .sdata section Zong Li
2020-02-04 11:39 ` Anup Patel
2020-02-05 11:07   ` Zong Li
2020-02-18 21:26     ` Palmer Dabbelt
2020-02-19  8:48       ` Zong Li
2020-02-10  6:42 ` Alex Ghiti

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).