linux-kernel - Re: [PATCH v3 2/2] clocksource/drivers/timer-clint: Add T-Head C9xx clint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 24 Apr 2024 12:53:15 +0100
From: Conor Dooley <conor@...nel.org>
To: Jisheng Zhang <jszhang@...nel.org>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Samuel Holland <samuel.holland@...ive.com>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] clocksource/drivers/timer-clint: Add T-Head C9xx
 clint

On Wed, Apr 10, 2024 at 10:23:47PM +0800, Jisheng Zhang wrote:
> To use the T-HEAD C9xx clint in RISCV-M NOMMU env, we need to take
> care two points:
> 
> 1.The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> implement such support.
> 
> 2. As pointed out by commit ca7810aecdba ("lib: utils/timer: mtimer:
> add a quirk for lacking mtime register") of opensbi:
> 
> "T-Head developers surely have a different understanding of time CSR and
> CLINT's mtime register with SiFive ones, that they did not implement
> the mtime register at all -- as shown in openC906 source code, their
> time CSR value is just exposed at the top of their processor IP block
> and expects an external continous counter, which makes it not
> overrideable, and thus mtime register is not implemented, even not for
> reading. However, if CLINTEE is not enabled in T-Head's MXSTATUS
> extended CSR, these systems still rely on the mtimecmp registers to
> generate timer interrupts. This makes it necessary to implement T-Head
> C9xx CLINT support in OpenSBI MTIMER driver, which skips implementing
> reading mtime register and falls back to default code that reads time
> CSR."
> 
> So, we need to fall back to read time CSR instead of mtime register.
> Add riscv_csr_time_available static key for this purpose.
> 
> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> ---
>  arch/riscv/include/asm/clint.h    |  2 ++
>  arch/riscv/include/asm/timex.h    | 18 +++++++++---
>  drivers/clocksource/timer-clint.c | 48 +++++++++++++++++++++++++++----
>  3 files changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/clint.h b/arch/riscv/include/asm/clint.h
> index 0789fd37b40a..c6057a182c5d 100644
> --- a/arch/riscv/include/asm/clint.h
> +++ b/arch/riscv/include/asm/clint.h
> @@ -10,6 +10,7 @@
>  #include <asm/mmio.h>
>  
>  #ifdef CONFIG_RISCV_M_MODE
> +#include <linux/jump_label.h>
>  /*
>   * This lives in the CLINT driver, but is accessed directly by timex.h to avoid
>   * any overhead when accessing the MMIO timer.
> @@ -21,6 +22,7 @@
>   * like "riscv_mtime", to signify that these non-ISA assumptions must hold.
>   */
>  extern u64 __iomem *clint_time_val;
> +DECLARE_STATIC_KEY_FALSE(riscv_csr_time_available);
>  #endif
>  
>  #endif
> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h
> index a06697846e69..007a15482d75 100644
> --- a/arch/riscv/include/asm/timex.h
> +++ b/arch/riscv/include/asm/timex.h
> @@ -17,18 +17,27 @@ typedef unsigned long cycles_t;
>  #ifdef CONFIG_64BIT
>  static inline cycles_t get_cycles(void)
>  {
> -	return readq_relaxed(clint_time_val);
> +	if (static_branch_likely(&riscv_csr_time_available))
> +		return csr_read(CSR_TIME);
> +	else
> +		return readq_relaxed(clint_time_val);
>  }
>  #else /* !CONFIG_64BIT */
>  static inline u32 get_cycles(void)
>  {
> -	return readl_relaxed(((u32 *)clint_time_val));
> +	if (static_branch_likely(&riscv_csr_time_available))
> +		return csr_read(CSR_TIME);
> +	else
> +		return readl_relaxed(((u32 *)clint_time_val));
>  }
>  #define get_cycles get_cycles
>  
>  static inline u32 get_cycles_hi(void)
>  {
> -	return readl_relaxed(((u32 *)clint_time_val) + 1);
> +	if (static_branch_likely(&riscv_csr_time_available))
> +		return csr_read(CSR_TIMEH);
> +	else
> +		return readl_relaxed(((u32 *)clint_time_val) + 1);
>  }

None of the else branches here need to actually be an else, since the
other branch returns. Otherwise, looks aight to me, thanks for the
update.


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)