Re: [PATCH v7 1/1] RISC-V: Add common csr_read_num() and csr_write_num() functions

[Zhanpeng Zhang]

Hi Anup,

On 11/5/26 12:15 AM, Anup Patel wrote:
> From: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
>
> In RISC-V, there is no CSR read/write instruction which takes CSR
> number via register so add common csr_read_num() and csr_write_num()
> functions which allow accessing certain CSRs by passing CSR number
> as parameter. These common functions will be first used by the
> ACPI CPPC driver and RISC-V PMU driver.
>
> Also, the RISC-V ACPI FFH specification allows arbitrary CSR number
> as CPPC register and the RISC-V SBI specification allows arbitrary
> CSR number as PMU hardware counter. This means ACPI CPPC driver and
> RISC-V PMU driver no longer need to do sanity checks on CSR number
> which are now done by the common csr_read_num() and csr_write_num()
> functions.
>
> Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> Reviewed-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx>
> Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> Reviewed-by: Atish Patra <atishp@xxxxxxxxxxxx>
> Reviewed-by: Nutty Liu <nutty.liu@xxxxxxxxxxx>
> ---
>   arch/riscv/include/asm/csr.h |   3 +
>   arch/riscv/kernel/Makefile   |   1 +
>   arch/riscv/kernel/csr.c      | 165 +++++++++++++++++++++++++++++++++++
>   drivers/acpi/riscv/cppc.c    |  17 ++--
>   drivers/perf/riscv_pmu.c     |  54 ++----------
>   5 files changed, 184 insertions(+), 56 deletions(-)
>   create mode 100644 arch/riscv/kernel/csr.c
>
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 621f84d1898f..ebd82043430b 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
>
> [...]
>   /*
>    * Read the CSR of a corresponding counter.
>    */
>   unsigned long riscv_pmu_ctr_read_csr(unsigned long csr)
>   {
> -	if (csr < CSR_CYCLE || csr > CSR_HPMCOUNTER31H ||
> -	   (csr > CSR_HPMCOUNTER31 && csr < CSR_CYCLEH)) {


Could we keep the existing CSR range check in riscv_pmu_ctr_read_csr()
when switching it to csr_read_num()?

The SBI PMU driver still assumes the architectural counter layout. For
example, if SBI assigns an event to a custom counter at CSR 0x800,
pmu_sbi_csr_index() truncates 0x800 - CSR_CYCLE to zero. The perf mmap
page then advertises index 1, so userspace reads cycle while the kernel
reads 0x800. The same index is used to set and clear scounteren.

Non-snapshot overflow handling and RV32 high-half reads also assume the
architectural layout. Could we keep the PMU conversion
behavior-preserving and handle custom counters separately?

> [...]
Thanks,
Zhanpeng