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 <[email protected]>
---
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 */
--
2.25.1
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 <[email protected]>
> ---
> 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.
--
Damien Le Moal
Western Digital
> -----Original Message-----
> From: Damien Le Moal <[email protected]>
> Sent: 26 September 2020 14:55
> To: [email protected]; [email protected];
> [email protected]; Anup Patel <[email protected]>;
> [email protected]
> Cc: [email protected]; [email protected]; Atish Patra
> <[email protected]>; Alistair Francis <[email protected]>; linux-
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > Sent: 26 September 2020 14:55
> > To: [email protected]; [email protected];
> > [email protected]; Anup Patel <[email protected]>;
> > [email protected]
> > Cc: [email protected]; [email protected]; Atish Patra
> > <[email protected]>; Alistair Francis <[email protected]>; linux-
> > [email protected]
> > 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 <[email protected]>
> > > ---
> > > 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)
--
Damien Le Moal
Western Digital
On Sat, Sep 26, 2020 at 3:16 PM Damien Le Moal <[email protected]> wrote:
>
> On Sat, 2020-09-26 at 09:31 +0000, Anup Patel wrote:
> > > -----Original Message-----
> > > From: Damien Le Moal <[email protected]>
> > > Sent: 26 September 2020 14:55
> > > To: [email protected]; [email protected];
> > > [email protected]; Anup Patel <[email protected]>;
> > > [email protected]
> > > Cc: [email protected]; [email protected]; Atish Patra
> > > <[email protected]>; Alistair Francis <[email protected]>; linux-
> > > [email protected]
> > > 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
> > On Sat, 2020-09-26 at 09:31 +0000, Anup Patel wrote:
> > > > -----Original Message-----
> > > > From: Damien Le Moal <[email protected]>
> > > > Sent: 26 September 2020 14:55
> > > > To: [email protected]; [email protected];
> > > > [email protected]; Anup Patel <[email protected]>;
> > > > [email protected]
> > > > Cc: [email protected]; [email protected]; Atish Patra
> > > > <[email protected]>; Alistair Francis <[email protected]>; linux-
> > > > [email protected]
> > > > 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 <[email protected]>
> > > > > ---
> > > > > 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
--
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
--
Damien Le Moal
Western Digital
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