* [PATCH v2] RISC-V: Check clint_time_val before use
@ 2020-09-26 10:21 Anup Patel
2020-09-26 10:31 ` Damien Le Moal
0 siblings, 1 reply; 7+ messages in thread
From: Anup Patel @ 2020-09-26 10:21 UTC (permalink / raw)
To: Palmer Dabbelt, Palmer Dabbelt, Paul Walmsley, Albert Ou
Cc: Atish Patra, Alistair Francis, Damien Le Moal, Anup Patel,
linux-riscv, linux-kernel, Anup Patel
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>
---
Changes since v1:
- Explicitly initialize clint_time_val to NULL in CLINT driver to
avoid hang on Kendryte K210
---
arch/riscv/include/asm/timex.h | 12 +++++++++---
drivers/clocksource/timer-clint.c | 2 +-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h
index 7f659dda0032..6e7b04874755 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 */
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)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] RISC-V: Check clint_time_val before use 2020-09-26 10:21 [PATCH v2] RISC-V: Check clint_time_val before use Anup Patel @ 2020-09-26 10:31 ` Damien Le Moal 2020-09-27 0:20 ` Palmer Dabbelt 0 siblings, 1 reply; 7+ messages in thread From: Damien Le Moal @ 2020-09-26 10:31 UTC (permalink / raw) To: paul.walmsley, palmer, palmerdabbelt, Anup Patel, aou Cc: anup, linux-riscv, Atish Patra, Alistair Francis, linux-kernel On Sat, 2020-09-26 at 15:51 +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> > --- > Changes since v1: > - Explicitly initialize clint_time_val to NULL in CLINT driver to > avoid hang on Kendryte K210 > --- > arch/riscv/include/asm/timex.h | 12 +++++++++--- > drivers/clocksource/timer-clint.c | 2 +- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > index 7f659dda0032..6e7b04874755 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 */ > 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) For Kendryte: Tested-by: Damien Le Moal <damien.lemoal@wdc.com> -- Damien Le Moal Western Digital ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] RISC-V: Check clint_time_val before use 2020-09-26 10:31 ` Damien Le Moal @ 2020-09-27 0:20 ` Palmer Dabbelt 2020-09-27 5:35 ` Anup Patel 2020-09-27 5:38 ` Anup Patel 0 siblings, 2 replies; 7+ messages in thread From: Palmer Dabbelt @ 2020-09-27 0:20 UTC (permalink / raw) To: Damien Le Moal Cc: Paul Walmsley, Anup Patel, aou, anup, linux-riscv, Atish Patra, Alistair Francis, linux-kernel On Sat, 26 Sep 2020 03:31:29 PDT (-0700), Damien Le Moal wrote: > On Sat, 2020-09-26 at 15:51 +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. I don't think this is the right way to solve that problem, as we're essentially just lying about the timer rather than informing the system we can't get timer-based entropy right now. MIPS is explicit about this, I don't see any reason why we shouldn't be as well. Does this fix the boot issue (see below for the NULL)? There's some other random-related arch functions so this might not be quite the right way to do it. diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h index 7f659dda0032..7e39b0068932 100644 --- a/arch/riscv/include/asm/timex.h +++ b/arch/riscv/include/asm/timex.h @@ -33,6 +33,18 @@ static inline u32 get_cycles_hi(void) #define get_cycles_hi get_cycles_hi #endif /* CONFIG_64BIT */ +/* + * Much like MIPS, we may not have a viable counter to use at an early point in + * the boot process. Unfortunately we don't have a fallback, so instead we + * just return 0. + */ +static inline unsigned long random_get_entropy(void) +{ + if (unlikely(clint_time_val == NULL)) + return 0; + return get_cycles(); +} + #else /* CONFIG_RISCV_M_MODE */ static inline cycles_t get_cycles(void) >> Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation >> for M-mode systems") >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> --- >> Changes since v1: >> - Explicitly initialize clint_time_val to NULL in CLINT driver to >> avoid hang on Kendryte K210 >> --- >> arch/riscv/include/asm/timex.h | 12 +++++++++--- >> drivers/clocksource/timer-clint.c | 2 +- >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h >> index 7f659dda0032..6e7b04874755 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 */ >> 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; This one I definately don't get. According the internet, the C standard says If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and every member that has pointer type were assigned a null pointer constant. so unless I'm missing something there shouldn't be any difference between these two lines. When I just apply this I get exactly the same "objdump -dt" before and after. I do see some difference in assembly, but only when I don't pass "-fno-common" and that ends up being passed during my Linux builds. >> #endif >> >> static void clint_send_ipi(const struct cpumask *target) > > For Kendryte: > > Tested-by: Damien Le Moal <damien.lemoal@wdc.com> > > -- > Damien Le Moal > Western Digital ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] RISC-V: Check clint_time_val before use 2020-09-27 0:20 ` Palmer Dabbelt @ 2020-09-27 5:35 ` Anup Patel 2020-09-27 5:37 ` Palmer Dabbelt 2020-09-27 5:38 ` Anup Patel 1 sibling, 1 reply; 7+ messages in thread From: Anup Patel @ 2020-09-27 5:35 UTC (permalink / raw) To: Palmer Dabbelt Cc: Damien Le Moal, Paul Walmsley, Anup Patel, Albert Ou, linux-riscv, Atish Patra, Alistair Francis, linux-kernel@vger.kernel.org List On Sun, Sep 27, 2020 at 5:50 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote: > > On Sat, 26 Sep 2020 03:31:29 PDT (-0700), Damien Le Moal wrote: > > On Sat, 2020-09-26 at 15:51 +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. > > I don't think this is the right way to solve that problem, as we're essentially > just lying about the timer rather than informing the system we can't get > timer-based entropy right now. MIPS is explicit about this, I don't see any > reason why we shouldn't be as well. > > Does this fix the boot issue (see below for the NULL)? There's some other > random-related arch functions so this might not be quite the right way to do > it. > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > index 7f659dda0032..7e39b0068932 100644 > --- a/arch/riscv/include/asm/timex.h > +++ b/arch/riscv/include/asm/timex.h > @@ -33,6 +33,18 @@ static inline u32 get_cycles_hi(void) > #define get_cycles_hi get_cycles_hi > #endif /* CONFIG_64BIT */ > > +/* > + * Much like MIPS, we may not have a viable counter to use at an early point in > + * the boot process. Unfortunately we don't have a fallback, so instead we > + * just return 0. > + */ > +static inline unsigned long random_get_entropy(void) > +{ > + if (unlikely(clint_time_val == NULL)) > + return 0; > + return get_cycles(); > +} > + Overall, this approach is good but this change is incomplete so does not work. The linux/timex.h expects random_get_entropy() to be macro so we need a "#define" as well. After fixing rand_initialize() with custom random_get_entropy(), we get another issue in boot_init_stack_canary() because boot_init_stack_canary() directly calls get_cycles() so we remove use of get_cycles() from boot_init_stack_canary() and this is similar to ARM, ARM64, and MIPS kernel. Regards, Anup > #else /* CONFIG_RISCV_M_MODE */ > > static inline cycles_t get_cycles(void) > > >> Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > >> for M-mode systems") > >> Signed-off-by: Anup Patel <anup.patel@wdc.com> > >> --- > >> Changes since v1: > >> - Explicitly initialize clint_time_val to NULL in CLINT driver to > >> avoid hang on Kendryte K210 > >> --- > >> arch/riscv/include/asm/timex.h | 12 +++++++++--- > >> drivers/clocksource/timer-clint.c | 2 +- > >> 2 files changed, 10 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > >> index 7f659dda0032..6e7b04874755 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 */ > >> 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; > > This one I definately don't get. According the internet, the C standard says > > If an object that has static storage duration is not initialized > explicitly, it is initialized implicitly as if every member that has > arithmetic type were assigned 0 and every member that has pointer type were > assigned a null pointer constant. > > so unless I'm missing something there shouldn't be any difference between these > two lines. When I just apply this I get exactly the same "objdump -dt" before > and after. I do see some difference in assembly, but only when I don't pass > "-fno-common" and that ends up being passed during my Linux builds. > > >> #endif > >> > >> static void clint_send_ipi(const struct cpumask *target) > > > > For Kendryte: > > > > Tested-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > -- > > Damien Le Moal > > Western Digital ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] RISC-V: Check clint_time_val before use 2020-09-27 5:35 ` Anup Patel @ 2020-09-27 5:37 ` Palmer Dabbelt 0 siblings, 0 replies; 7+ messages in thread From: Palmer Dabbelt @ 2020-09-27 5:37 UTC (permalink / raw) To: anup Cc: Damien Le Moal, Paul Walmsley, Anup Patel, aou, linux-riscv, Atish Patra, Alistair Francis, linux-kernel On Sat, 26 Sep 2020 22:35:39 PDT (-0700), anup@brainfault.org wrote: > On Sun, Sep 27, 2020 at 5:50 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote: >> >> On Sat, 26 Sep 2020 03:31:29 PDT (-0700), Damien Le Moal wrote: >> > On Sat, 2020-09-26 at 15:51 +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. >> >> I don't think this is the right way to solve that problem, as we're essentially >> just lying about the timer rather than informing the system we can't get >> timer-based entropy right now. MIPS is explicit about this, I don't see any >> reason why we shouldn't be as well. >> >> Does this fix the boot issue (see below for the NULL)? There's some other >> random-related arch functions so this might not be quite the right way to do >> it. >> >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h >> index 7f659dda0032..7e39b0068932 100644 >> --- a/arch/riscv/include/asm/timex.h >> +++ b/arch/riscv/include/asm/timex.h >> @@ -33,6 +33,18 @@ static inline u32 get_cycles_hi(void) >> #define get_cycles_hi get_cycles_hi >> #endif /* CONFIG_64BIT */ >> >> +/* >> + * Much like MIPS, we may not have a viable counter to use at an early point in >> + * the boot process. Unfortunately we don't have a fallback, so instead we >> + * just return 0. >> + */ >> +static inline unsigned long random_get_entropy(void) >> +{ >> + if (unlikely(clint_time_val == NULL)) >> + return 0; >> + return get_cycles(); >> +} >> + > > Overall, this approach is good but this change is incomplete so does not work. > > The linux/timex.h expects random_get_entropy() to be macro so we need a > "#define" as well. > > After fixing rand_initialize() with custom random_get_entropy(), we get another > issue in boot_init_stack_canary() because boot_init_stack_canary() directly > calls get_cycles() so we remove use of get_cycles() from > boot_init_stack_canary() > and this is similar to ARM, ARM64, and MIPS kernel. OK, are you going to send that patch or do you want me to? > > Regards, > Anup > >> #else /* CONFIG_RISCV_M_MODE */ >> >> static inline cycles_t get_cycles(void) >> >> >> Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation >> >> for M-mode systems") >> >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> >> --- >> >> Changes since v1: >> >> - Explicitly initialize clint_time_val to NULL in CLINT driver to >> >> avoid hang on Kendryte K210 >> >> --- >> >> arch/riscv/include/asm/timex.h | 12 +++++++++--- >> >> drivers/clocksource/timer-clint.c | 2 +- >> >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h >> >> index 7f659dda0032..6e7b04874755 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 */ >> >> 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; >> >> This one I definately don't get. According the internet, the C standard says >> >> If an object that has static storage duration is not initialized >> explicitly, it is initialized implicitly as if every member that has >> arithmetic type were assigned 0 and every member that has pointer type were >> assigned a null pointer constant. >> >> so unless I'm missing something there shouldn't be any difference between these >> two lines. When I just apply this I get exactly the same "objdump -dt" before >> and after. I do see some difference in assembly, but only when I don't pass >> "-fno-common" and that ends up being passed during my Linux builds. >> >> >> #endif >> >> >> >> static void clint_send_ipi(const struct cpumask *target) >> > >> > For Kendryte: >> > >> > Tested-by: Damien Le Moal <damien.lemoal@wdc.com> >> > >> > -- >> > Damien Le Moal >> > Western Digital ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] RISC-V: Check clint_time_val before use 2020-09-27 0:20 ` Palmer Dabbelt 2020-09-27 5:35 ` Anup Patel @ 2020-09-27 5:38 ` Anup Patel 2020-09-27 5:39 ` Palmer Dabbelt 1 sibling, 1 reply; 7+ messages in thread From: Anup Patel @ 2020-09-27 5:38 UTC (permalink / raw) To: Palmer Dabbelt Cc: Damien Le Moal, Paul Walmsley, Anup Patel, Albert Ou, linux-riscv, Atish Patra, Alistair Francis, linux-kernel@vger.kernel.org List On Sun, Sep 27, 2020 at 5:50 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote: > > On Sat, 26 Sep 2020 03:31:29 PDT (-0700), Damien Le Moal wrote: > > On Sat, 2020-09-26 at 15:51 +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. > > I don't think this is the right way to solve that problem, as we're essentially > just lying about the timer rather than informing the system we can't get > timer-based entropy right now. MIPS is explicit about this, I don't see any > reason why we shouldn't be as well. > > Does this fix the boot issue (see below for the NULL)? There's some other > random-related arch functions so this might not be quite the right way to do > it. > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > index 7f659dda0032..7e39b0068932 100644 > --- a/arch/riscv/include/asm/timex.h > +++ b/arch/riscv/include/asm/timex.h > @@ -33,6 +33,18 @@ static inline u32 get_cycles_hi(void) > #define get_cycles_hi get_cycles_hi > #endif /* CONFIG_64BIT */ > > +/* > + * Much like MIPS, we may not have a viable counter to use at an early point in > + * the boot process. Unfortunately we don't have a fallback, so instead we > + * just return 0. > + */ > +static inline unsigned long random_get_entropy(void) > +{ > + if (unlikely(clint_time_val == NULL)) > + return 0; > + return get_cycles(); > +} > + > #else /* CONFIG_RISCV_M_MODE */ > > static inline cycles_t get_cycles(void) > > >> Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation > >> for M-mode systems") > >> Signed-off-by: Anup Patel <anup.patel@wdc.com> > >> --- > >> Changes since v1: > >> - Explicitly initialize clint_time_val to NULL in CLINT driver to > >> avoid hang on Kendryte K210 > >> --- > >> arch/riscv/include/asm/timex.h | 12 +++++++++--- > >> drivers/clocksource/timer-clint.c | 2 +- > >> 2 files changed, 10 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > >> index 7f659dda0032..6e7b04874755 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 */ > >> 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; > > This one I definately don't get. According the internet, the C standard says > > If an object that has static storage duration is not initialized > explicitly, it is initialized implicitly as if every member that has > arithmetic type were assigned 0 and every member that has pointer type were > assigned a null pointer constant. > > so unless I'm missing something there shouldn't be any difference between these > two lines. When I just apply this I get exactly the same "objdump -dt" before > and after. I do see some difference in assembly, but only when I don't pass > "-fno-common" and that ends up being passed during my Linux builds. Even checkpatch complains about explicit NULL assignment to global variables. I will send v3 with a different approach to fix this so hopefully with v3 we will not require explicit NULL assignment. Regards, Anup > > >> #endif > >> > >> static void clint_send_ipi(const struct cpumask *target) > > > > For Kendryte: > > > > Tested-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > -- > > Damien Le Moal > > Western Digital ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] RISC-V: Check clint_time_val before use 2020-09-27 5:38 ` Anup Patel @ 2020-09-27 5:39 ` Palmer Dabbelt 0 siblings, 0 replies; 7+ messages in thread From: Palmer Dabbelt @ 2020-09-27 5:39 UTC (permalink / raw) To: anup Cc: Damien Le Moal, Paul Walmsley, Anup Patel, aou, linux-riscv, Atish Patra, Alistair Francis, linux-kernel On Sat, 26 Sep 2020 22:38:17 PDT (-0700), anup@brainfault.org wrote: > On Sun, Sep 27, 2020 at 5:50 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote: >> >> On Sat, 26 Sep 2020 03:31:29 PDT (-0700), Damien Le Moal wrote: >> > On Sat, 2020-09-26 at 15:51 +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. >> >> I don't think this is the right way to solve that problem, as we're essentially >> just lying about the timer rather than informing the system we can't get >> timer-based entropy right now. MIPS is explicit about this, I don't see any >> reason why we shouldn't be as well. >> >> Does this fix the boot issue (see below for the NULL)? There's some other >> random-related arch functions so this might not be quite the right way to do >> it. >> >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h >> index 7f659dda0032..7e39b0068932 100644 >> --- a/arch/riscv/include/asm/timex.h >> +++ b/arch/riscv/include/asm/timex.h >> @@ -33,6 +33,18 @@ static inline u32 get_cycles_hi(void) >> #define get_cycles_hi get_cycles_hi >> #endif /* CONFIG_64BIT */ >> >> +/* >> + * Much like MIPS, we may not have a viable counter to use at an early point in >> + * the boot process. Unfortunately we don't have a fallback, so instead we >> + * just return 0. >> + */ >> +static inline unsigned long random_get_entropy(void) >> +{ >> + if (unlikely(clint_time_val == NULL)) >> + return 0; >> + return get_cycles(); >> +} >> + >> #else /* CONFIG_RISCV_M_MODE */ >> >> static inline cycles_t get_cycles(void) >> >> >> Fixes: d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation >> >> for M-mode systems") >> >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> >> --- >> >> Changes since v1: >> >> - Explicitly initialize clint_time_val to NULL in CLINT driver to >> >> avoid hang on Kendryte K210 >> >> --- >> >> arch/riscv/include/asm/timex.h | 12 +++++++++--- >> >> drivers/clocksource/timer-clint.c | 2 +- >> >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h >> >> index 7f659dda0032..6e7b04874755 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 */ >> >> 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; >> >> This one I definately don't get. According the internet, the C standard says >> >> If an object that has static storage duration is not initialized >> explicitly, it is initialized implicitly as if every member that has >> arithmetic type were assigned 0 and every member that has pointer type were >> assigned a null pointer constant. >> >> so unless I'm missing something there shouldn't be any difference between these >> two lines. When I just apply this I get exactly the same "objdump -dt" before >> and after. I do see some difference in assembly, but only when I don't pass >> "-fno-common" and that ends up being passed during my Linux builds. > > Even checkpatch complains about explicit NULL assignment to global variables. > > I will send v3 with a different approach to fix this so hopefully with > v3 we will > not require explicit NULL assignment. Awesome, thanks! > > Regards, > Anup > >> >> >> #endif >> >> >> >> static void clint_send_ipi(const struct cpumask *target) >> > >> > For Kendryte: >> > >> > Tested-by: Damien Le Moal <damien.lemoal@wdc.com> >> > >> > -- >> > Damien Le Moal >> > Western Digital ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-09-27 5:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-26 10:21 [PATCH v2] RISC-V: Check clint_time_val before use Anup Patel 2020-09-26 10:31 ` Damien Le Moal 2020-09-27 0:20 ` Palmer Dabbelt 2020-09-27 5:35 ` Anup Patel 2020-09-27 5:37 ` Palmer Dabbelt 2020-09-27 5:38 ` Anup Patel 2020-09-27 5:39 ` Palmer Dabbelt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®