[PATCH] riscv: enable TIME CSR in U mode

[Gary Guo]

I don't think we should let the firmware to decide which performance counter CSRs can be used by the userspace. No counter should be allowed unless there is a reason to allow it, like the timer CSR. Also, OpenSBI is not the only SBI implementation,; there could be no SBIs at all (i.e. M mode) or have other SBIs that have different defaults.

Best,
Gary

-----Original Message-----
From: Nick Kossifidis <mick at ics.forth.gr> 
Sent: Thursday, March 4, 2021 5:35 PM
To: Gary Guo <gary at garyguo.net>
Cc: linux-riscv at lists.infradead.org
Subject: Re: [PATCH] riscv: enable TIME CSR in U mode

Στις 2021-02-17 00:12, Gary Guo έγραψε:
> After ad5d112 we let the user mode to use rdtime directly for time 
> access. This works if the hardware does not implement the TIME CSR and 
> traps to the firmware. The spec however does allow a hardware 
> implementation to redirect the CSR access to the memory-mapped MTIME 
> CSR, and if this is performed it will check {M,S}COUNTEREN CSR to see 
> if it is allowed.
> 
> To prevent unhandled illegal instruction fault on these platforms, we 
> can request these CSRs to be enabled in U-mode. For platforms that 
> does not support the TIME CSR to MTIME CSR conversion, they will 
> hardwire corresponding bit in COUNTEREN to zero and this would be a 
> no-op.
> 
> Signed-off-by: Gary Guo <gary at garyguo.net>
> ---
>  arch/riscv/kernel/head.S | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S index 
> 0a4e81b8dc79..5a7a62190342 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -190,6 +190,9 @@ ENTRY(_start_kernel)  .align 2
>  pmp_done:
> 
> +	/* Allow user-mode to access time CSR */
> +	csrw, CSR_MCOUNTEREN, 2
> +
>  	/*
>  	 * The hartid in a0 is expected later on, and we have no firmware
>  	 * to hand it to us.
> @@ -197,6 +200,9 @@ pmp_done:
>  	csrr a0, CSR_MHARTID
>  #endif /* CONFIG_RISCV_M_MODE */
> 
> +	/* Allow user-mode to access time CSR */
> +	csrw CSR_SCOUNTEREN, 2
> +
>  	/* Load the global pointer */
>  .option push
>  .option norelax

Shouldn't we use csrrs here to preserve current settings from the firmware ? Also OpenSBI at this point sets both MCOUNTEREN/SCOUNTEREN to -1, does anyone else modify that value ?

Regards,
Nick