| Message ID | 20200926072750.807764-1-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Check clint_time_val before use | expand |
On Sat, 2020-09-26 at 12:57 +0530, Anup Patel wrote: > The NoMMU kernel is broken for QEMU virt machine from Linux-5.9-rc6 > because the get_cycles() and friends are called very early from > rand_initialize() before CLINT driver is probed. To fix this, we > should check clint_time_val before use in get_cycles() and friends. > > Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > for M-mode systems") > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > arch/riscv/include/asm/timex.h | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > index 7f659dda0032..52b42bb1602c 100644 > --- a/arch/riscv/include/asm/timex.h > +++ b/arch/riscv/include/asm/timex.h > @@ -17,18 +17,24 @@ typedef unsigned long cycles_t; > #ifdef CONFIG_64BIT > static inline cycles_t get_cycles(void) > { > - return readq_relaxed(clint_time_val); > + if (clint_time_val) > + return readq_relaxed(clint_time_val); > + return 0; > } > #else /* !CONFIG_64BIT */ > static inline u32 get_cycles(void) > { > - return readl_relaxed(((u32 *)clint_time_val)); > + if (clint_time_val) > + return readl_relaxed(((u32 *)clint_time_val)); > + return 0; > } > #define get_cycles get_cycles > > static inline u32 get_cycles_hi(void) > { > - return readl_relaxed(((u32 *)clint_time_val) + 1); > + if (clint_time_val) > + return readl_relaxed(((u32 *)clint_time_val) + 1); > + return 0 > } > #define get_cycles_hi get_cycles_hi > #endif /* CONFIG_64BIT */ Applying this on top of rc6, I now get a hang on Kendryte boot... No problems without the patch on the other hand.
> -----Original Message----- > From: Damien Le Moal <Damien.LeMoal@wdc.com> > Sent: 26 September 2020 14:55 > To: paul.walmsley@sifive.com; palmer@dabbelt.com; > palmerdabbelt@google.com; Anup Patel <Anup.Patel@wdc.com>; > aou@eecs.berkeley.edu > Cc: anup@brainfault.org; linux-riscv@lists.infradead.org; Atish Patra > <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] RISC-V: Check clint_time_val before use > > On Sat, 2020-09-26 at 12:57 +0530, Anup Patel wrote: > > The NoMMU kernel is broken for QEMU virt machine from Linux-5.9-rc6 > > because the get_cycles() and friends are called very early from > > rand_initialize() before CLINT driver is probed. To fix this, we > > should check clint_time_val before use in get_cycles() and friends. > > > > Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > > for M-mode systems") > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > --- > > arch/riscv/include/asm/timex.h | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/include/asm/timex.h > > b/arch/riscv/include/asm/timex.h index 7f659dda0032..52b42bb1602c > > 100644 > > --- a/arch/riscv/include/asm/timex.h > > +++ b/arch/riscv/include/asm/timex.h > > @@ -17,18 +17,24 @@ typedef unsigned long cycles_t; #ifdef > > CONFIG_64BIT static inline cycles_t get_cycles(void) { > > - return readq_relaxed(clint_time_val); > > + if (clint_time_val) > > + return readq_relaxed(clint_time_val); > > + return 0; > > } > > #else /* !CONFIG_64BIT */ > > static inline u32 get_cycles(void) > > { > > - return readl_relaxed(((u32 *)clint_time_val)); > > + if (clint_time_val) > > + return readl_relaxed(((u32 *)clint_time_val)); > > + return 0; > > } > > #define get_cycles get_cycles > > > > static inline u32 get_cycles_hi(void) { > > - return readl_relaxed(((u32 *)clint_time_val) + 1); > > + if (clint_time_val) > > + return readl_relaxed(((u32 *)clint_time_val) + 1); > > + return 0 > > } > > #define get_cycles_hi get_cycles_hi > > #endif /* CONFIG_64BIT */ > > Applying this on top of rc6, I now get a hang on Kendryte boot... > No problems without the patch on the other hand. Not sure about the issue with Kendryte but I get a crash on QEMU virt machine without this patch. Regards, Anup
On Sat, 2020-09-26 at 09:31 +0000, Anup Patel wrote: > > -----Original Message----- > > From: Damien Le Moal <Damien.LeMoal@wdc.com> > > Sent: 26 September 2020 14:55 > > To: paul.walmsley@sifive.com; palmer@dabbelt.com; > > palmerdabbelt@google.com; Anup Patel <Anup.Patel@wdc.com>; > > aou@eecs.berkeley.edu > > Cc: anup@brainfault.org; linux-riscv@lists.infradead.org; Atish Patra > > <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH] RISC-V: Check clint_time_val before use > > > > On Sat, 2020-09-26 at 12:57 +0530, Anup Patel wrote: > > > The NoMMU kernel is broken for QEMU virt machine from Linux-5.9-rc6 > > > because the get_cycles() and friends are called very early from > > > rand_initialize() before CLINT driver is probed. To fix this, we > > > should check clint_time_val before use in get_cycles() and friends. > > > > > > Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > > > for M-mode systems") > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > > --- > > > arch/riscv/include/asm/timex.h | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/timex.h > > > b/arch/riscv/include/asm/timex.h index 7f659dda0032..52b42bb1602c > > > 100644 > > > --- a/arch/riscv/include/asm/timex.h > > > +++ b/arch/riscv/include/asm/timex.h > > > @@ -17,18 +17,24 @@ typedef unsigned long cycles_t; #ifdef > > > CONFIG_64BIT static inline cycles_t get_cycles(void) { > > > - return readq_relaxed(clint_time_val); > > > + if (clint_time_val) > > > + return readq_relaxed(clint_time_val); > > > + return 0; > > > } > > > #else /* !CONFIG_64BIT */ > > > static inline u32 get_cycles(void) > > > { > > > - return readl_relaxed(((u32 *)clint_time_val)); > > > + if (clint_time_val) > > > + return readl_relaxed(((u32 *)clint_time_val)); > > > + return 0; > > > } > > > #define get_cycles get_cycles > > > > > > static inline u32 get_cycles_hi(void) { > > > - return readl_relaxed(((u32 *)clint_time_val) + 1); > > > + if (clint_time_val) > > > + return readl_relaxed(((u32 *)clint_time_val) + 1); > > > + return 0 > > > } > > > #define get_cycles_hi get_cycles_hi > > > #endif /* CONFIG_64BIT */ > > > > Applying this on top of rc6, I now get a hang on Kendryte boot... > > No problems without the patch on the other hand. > > Not sure about the issue with Kendryte but I get a crash on > QEMU virt machine without this patch. With this applied in addition to your patch, it works. diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer- clint.c index d17367dee02c..8dbec85979fd 100644 --- a/drivers/clocksource/timer-clint.c +++ b/drivers/clocksource/timer-clint.c @@ -37,7 +37,7 @@ static unsigned long clint_timer_freq; static unsigned int clint_timer_irq; #ifdef CONFIG_RISCV_M_MODE -u64 __iomem *clint_time_val; +u64 __iomem *clint_time_val = NULL; #endif static void clint_send_ipi(const struct cpumask *target)
On Sat, Sep 26, 2020 at 3:16 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On Sat, 2020-09-26 at 09:31 +0000, Anup Patel wrote: > > > -----Original Message----- > > > From: Damien Le Moal <Damien.LeMoal@wdc.com> > > > Sent: 26 September 2020 14:55 > > > To: paul.walmsley@sifive.com; palmer@dabbelt.com; > > > palmerdabbelt@google.com; Anup Patel <Anup.Patel@wdc.com>; > > > aou@eecs.berkeley.edu > > > Cc: anup@brainfault.org; linux-riscv@lists.infradead.org; Atish Patra > > > <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH] RISC-V: Check clint_time_val before use > > > > > > On Sat, 2020-09-26 at 12:57 +0530, Anup Patel wrote: > > > > The NoMMU kernel is broken for QEMU virt machine from Linux-5.9-rc6 > > > > because the get_cycles() and friends are called very early from > > > > rand_initialize() before CLINT driver is probed. To fix this, we > > > > should check clint_time_val before use in get_cycles() and friends. > > > > > > > > Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > > > > for M-mode systems") > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > > > --- > > > > arch/riscv/include/asm/timex.h | 12 +++++++++--- > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/riscv/include/asm/timex.h > > > > b/arch/riscv/include/asm/timex.h index 7f659dda0032..52b42bb1602c > > > > 100644 > > > > --- a/arch/riscv/include/asm/timex.h > > > > +++ b/arch/riscv/include/asm/timex.h > > > > @@ -17,18 +17,24 @@ typedef unsigned long cycles_t; #ifdef > > > > CONFIG_64BIT static inline cycles_t get_cycles(void) { > > > > - return readq_relaxed(clint_time_val); > > > > + if (clint_time_val) > > > > + return readq_relaxed(clint_time_val); > > > > + return 0; > > > > } > > > > #else /* !CONFIG_64BIT */ > > > > static inline u32 get_cycles(void) > > > > { > > > > - return readl_relaxed(((u32 *)clint_time_val)); > > > > + if (clint_time_val) > > > > + return readl_relaxed(((u32 *)clint_time_val)); > > > > + return 0; > > > > } > > > > #define get_cycles get_cycles > > > > > > > > static inline u32 get_cycles_hi(void) { > > > > - return readl_relaxed(((u32 *)clint_time_val) + 1); > > > > + if (clint_time_val) > > > > + return readl_relaxed(((u32 *)clint_time_val) + 1); > > > > + return 0 > > > > } > > > > #define get_cycles_hi get_cycles_hi > > > > #endif /* CONFIG_64BIT */ > > > > > > Applying this on top of rc6, I now get a hang on Kendryte boot... > > > No problems without the patch on the other hand. > > > > Not sure about the issue with Kendryte but I get a crash on > > QEMU virt machine without this patch. > > With this applied in addition to your patch, it works. > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer- > clint.c > index d17367dee02c..8dbec85979fd 100644 > --- a/drivers/clocksource/timer-clint.c > +++ b/drivers/clocksource/timer-clint.c > @@ -37,7 +37,7 @@ static unsigned long clint_timer_freq; > static unsigned int clint_timer_irq; > > #ifdef CONFIG_RISCV_M_MODE > -u64 __iomem *clint_time_val; > +u64 __iomem *clint_time_val = NULL; > #endif > > static void clint_send_ipi(const struct cpumask *target) Ahh, clint_time_val is an uninitialized variable. I will send a v2 with your SoB. Regards, Anup > > -- > Damien Le Moal > Western Digital
On Sat, 2020-09-26 at 15:27 +0530, Anup Patel wrote: > On Sat, Sep 26, 2020 at 3:16 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On Sat, 2020-09-26 at 09:31 +0000, Anup Patel wrote: > > > > -----Original Message----- > > > > From: Damien Le Moal <Damien.LeMoal@wdc.com> > > > > Sent: 26 September 2020 14:55 > > > > To: paul.walmsley@sifive.com; palmer@dabbelt.com; > > > > palmerdabbelt@google.com; Anup Patel <Anup.Patel@wdc.com>; > > > > aou@eecs.berkeley.edu > > > > Cc: anup@brainfault.org; linux-riscv@lists.infradead.org; Atish Patra > > > > <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; linux- > > > > kernel@vger.kernel.org > > > > Subject: Re: [PATCH] RISC-V: Check clint_time_val before use > > > > > > > > On Sat, 2020-09-26 at 12:57 +0530, Anup Patel wrote: > > > > > The NoMMU kernel is broken for QEMU virt machine from Linux-5.9-rc6 > > > > > because the get_cycles() and friends are called very early from > > > > > rand_initialize() before CLINT driver is probed. To fix this, we > > > > > should check clint_time_val before use in get_cycles() and friends. > > > > > > > > > > Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > > > > > for M-mode systems") > > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > > > > --- > > > > > arch/riscv/include/asm/timex.h | 12 +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/include/asm/timex.h > > > > > b/arch/riscv/include/asm/timex.h index 7f659dda0032..52b42bb1602c > > > > > 100644 > > > > > --- a/arch/riscv/include/asm/timex.h > > > > > +++ b/arch/riscv/include/asm/timex.h > > > > > @@ -17,18 +17,24 @@ typedef unsigned long cycles_t; #ifdef > > > > > CONFIG_64BIT static inline cycles_t get_cycles(void) { > > > > > - return readq_relaxed(clint_time_val); > > > > > + if (clint_time_val) > > > > > + return readq_relaxed(clint_time_val); > > > > > + return 0; > > > > > } > > > > > #else /* !CONFIG_64BIT */ > > > > > static inline u32 get_cycles(void) > > > > > { > > > > > - return readl_relaxed(((u32 *)clint_time_val)); > > > > > + if (clint_time_val) > > > > > + return readl_relaxed(((u32 *)clint_time_val)); > > > > > + return 0; > > > > > } > > > > > #define get_cycles get_cycles > > > > > > > > > > static inline u32 get_cycles_hi(void) { > > > > > - return readl_relaxed(((u32 *)clint_time_val) + 1); > > > > > + if (clint_time_val) > > > > > + return readl_relaxed(((u32 *)clint_time_val) + 1); > > > > > + return 0 > > > > > } > > > > > #define get_cycles_hi get_cycles_hi > > > > > #endif /* CONFIG_64BIT */ > > > > > > > > Applying this on top of rc6, I now get a hang on Kendryte boot... > > > > No problems without the patch on the other hand. > > > > > > Not sure about the issue with Kendryte but I get a crash on > > > QEMU virt machine without this patch. > > > > With this applied in addition to your patch, it works. > > > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer- > > clint.c > > index d17367dee02c..8dbec85979fd 100644 > > --- a/drivers/clocksource/timer-clint.c > > +++ b/drivers/clocksource/timer-clint.c > > @@ -37,7 +37,7 @@ static unsigned long clint_timer_freq; > > static unsigned int clint_timer_irq; > > > > #ifdef CONFIG_RISCV_M_MODE > > -u64 __iomem *clint_time_val; > > +u64 __iomem *clint_time_val = NULL; > > #endif > > > > static void clint_send_ipi(const struct cpumask *target) > > Ahh, clint_time_val is an uninitialized variable. > > I will send a v2 with your SoB. No need for my sob. Just merge that in your patch. > > Regards, > Anup > > > -- > > Damien Le Moal > > Western Digital
On Sat, 26 Sep 2020, Damien Le Moal wrote: > > > Applying this on top of rc6, I now get a hang on Kendryte boot... > > > No problems without the patch on the other hand. > > > > Not sure about the issue with Kendryte but I get a crash on > > QEMU virt machine without this patch. > > With this applied in addition to your patch, it works. > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer- > clint.c > index d17367dee02c..8dbec85979fd 100644 > --- a/drivers/clocksource/timer-clint.c > +++ b/drivers/clocksource/timer-clint.c > @@ -37,7 +37,7 @@ static unsigned long clint_timer_freq; > static unsigned int clint_timer_irq; > > #ifdef CONFIG_RISCV_M_MODE > -u64 __iomem *clint_time_val; > +u64 __iomem *clint_time_val = NULL; > #endif Hmm, BSS initialisation issue? Maciej
On Sat, 2020-09-26 at 11:09 +0100, Maciej W. Rozycki wrote: > On Sat, 26 Sep 2020, Damien Le Moal wrote: > > > > > Applying this on top of rc6, I now get a hang on Kendryte boot... > > > > No problems without the patch on the other hand. > > > > > > Not sure about the issue with Kendryte but I get a crash on > > > QEMU virt machine without this patch. > > > > With this applied in addition to your patch, it works. > > > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer- > > clint.c > > index d17367dee02c..8dbec85979fd 100644 > > --- a/drivers/clocksource/timer-clint.c > > +++ b/drivers/clocksource/timer-clint.c > > @@ -37,7 +37,7 @@ static unsigned long clint_timer_freq; > > static unsigned int clint_timer_irq; > > > > #ifdef CONFIG_RISCV_M_MODE > > -u64 __iomem *clint_time_val; > > +u64 __iomem *clint_time_val = NULL; > > #endif > > Hmm, BSS initialisation issue? Not a static variable, so it is not in BSS, no ? > > Maciej
On Sat, 26 Sep 2020, Damien Le Moal wrote: > > > With this applied in addition to your patch, it works. > > > > > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer- > > > clint.c > > > index d17367dee02c..8dbec85979fd 100644 > > > --- a/drivers/clocksource/timer-clint.c > > > +++ b/drivers/clocksource/timer-clint.c > > > @@ -37,7 +37,7 @@ static unsigned long clint_timer_freq; > > > static unsigned int clint_timer_irq; > > > > > > #ifdef CONFIG_RISCV_M_MODE > > > -u64 __iomem *clint_time_val; > > > +u64 __iomem *clint_time_val = NULL; > > > #endif > > > > Hmm, BSS initialisation issue? > > Not a static variable, so it is not in BSS, no ? Maybe it has a weird declaration elsewhere which messes up things (I haven't checked), but it looks to me like it does have static storage (rather than automatic or thread one), so if uninitialised it goes to BSS, and it is supposed to be all-zeros whether explicitly assigned a NULL value or not. It does have external rather than internal linkage (as it would if it had the `static' keyword), but it does not matter. Best check with `objdump'/`readelf'. Maciej
diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h index 7f659dda0032..52b42bb1602c 100644 --- a/arch/riscv/include/asm/timex.h +++ b/arch/riscv/include/asm/timex.h @@ -17,18 +17,24 @@ typedef unsigned long cycles_t; #ifdef CONFIG_64BIT static inline cycles_t get_cycles(void) { - return readq_relaxed(clint_time_val); + if (clint_time_val) + return readq_relaxed(clint_time_val); + return 0; } #else /* !CONFIG_64BIT */ static inline u32 get_cycles(void) { - return readl_relaxed(((u32 *)clint_time_val)); + if (clint_time_val) + return readl_relaxed(((u32 *)clint_time_val)); + return 0; } #define get_cycles get_cycles static inline u32 get_cycles_hi(void) { - return readl_relaxed(((u32 *)clint_time_val) + 1); + if (clint_time_val) + return readl_relaxed(((u32 *)clint_time_val) + 1); + return 0 } #define get_cycles_hi get_cycles_hi #endif /* CONFIG_64BIT */
The NoMMU kernel is broken for QEMU virt machine from Linux-5.9-rc6 because the get_cycles() and friends are called very early from rand_initialize() before CLINT driver is probed. To fix this, we should check clint_time_val before use in get_cycles() and friends. Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation for M-mode systems") Signed-off-by: Anup Patel <anup.patel@wdc.com> --- arch/riscv/include/asm/timex.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)