| 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
| ||
|
Message-ID: <20250815-e69bdc695cf78e6ce07e580c@orel>
Date: Fri, 15 Aug 2025 15:02:02 -0500
From: Andrew Jones <ajones@...tanamicro.com>
To: Anup Patel <apatel@...tanamicro.com>
Cc: Sunil V L <sunilvl@...tanamicro.com>,
"Rafael J . Wysocki" <rafael@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>, Alexandre Ghiti <alex@...ti.fr>, Len Brown <lenb@...nel.org>,
Atish Patra <atish.patra@...ux.dev>, Anup Patel <anup@...infault.org>, Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>, linux-acpi@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] RISC-V: Add common csr_read_num() and
csr_write_num() functions
On Fri, Aug 15, 2025 at 09:44:06PM +0530, Anup Patel wrote:
> 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.
>
> Signed-off-by: Anup Patel <apatel@...tanamicro.com>
> ---
> arch/riscv/include/asm/csr.h | 3 +
> arch/riscv/kernel/Makefile | 1 +
> arch/riscv/kernel/csr.c | 177 +++++++++++++++++++++++++++++++++++
> drivers/acpi/riscv/cppc.c | 17 ++--
> drivers/perf/riscv_pmu.c | 43 +--------
> 5 files changed, 189 insertions(+), 52 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 6fed42e37705..1540626b3540 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -575,6 +575,9 @@
> : "memory"); \
> })
>
> +extern unsigned long csr_read_num(unsigned long csr_num, int *out_err);
> +extern void csr_write_num(unsigned long csr_num, unsigned long val, int *out_err);
My preference would be for an interface with the return/err parameters the
other way around, i.e.
int csr_read_num(unsigned long csr_num, unsigned long *val);
int csr_write_num(unsigned long csr_num, unsigned long val);
and then ensure all callers always check that the return value is zero
before proceeding to use val or assume val was written.
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_RISCV_CSR_H */
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index c7b542573407..0a75e20bde18 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -50,6 +50,7 @@ obj-y += soc.o
> obj-$(CONFIG_RISCV_ALTERNATIVE) += alternative.o
> obj-y += cpu.o
> obj-y += cpufeature.o
> +obj-y += csr.o
> obj-y += entry.o
> obj-y += irq.o
> obj-y += process.o
> diff --git a/arch/riscv/kernel/csr.c b/arch/riscv/kernel/csr.c
> new file mode 100644
> index 000000000000..f7de45bb597c
> --- /dev/null
> +++ b/arch/riscv/kernel/csr.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Ventana Micro Systems Inc.
> + */
> +
> +#define pr_fmt(fmt) "riscv: " fmt
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/printk.h>
> +#include <linux/types.h>
> +#include <asm/csr.h>
> +
> +#define CSR_CUSTOM0_U_RW_BASE 0x800
> +#define CSR_CUSTOM0_U_RW_COUNT 0x100
> +
> +#define CSR_CUSTOM1_U_RO_BASE 0xCC0
> +#define CSR_CUSTOM1_U_RO_COUNT 0x040
> +
> +#define CSR_CUSTOM2_S_RW_BASE 0x5C0
> +#define CSR_CUSTOM2_S_RW_COUNT 0x040
> +
> +#define CSR_CUSTOM3_S_RW_BASE 0x9C0
> +#define CSR_CUSTOM3_S_RW_COUNT 0x040
> +
> +#define CSR_CUSTOM4_S_RO_BASE 0xDC0
> +#define CSR_CUSTOM4_S_RO_COUNT 0x040
> +
> +#define CSR_CUSTOM5_HS_RW_BASE 0x6C0
> +#define CSR_CUSTOM5_HS_RW_COUNT 0x040
> +
> +#define CSR_CUSTOM6_HS_RW_BASE 0xAC0
> +#define CSR_CUSTOM6_HS_RW_COUNT 0x040
> +
> +#define CSR_CUSTOM7_HS_RO_BASE 0xEC0
> +#define CSR_CUSTOM7_HS_RO_COUNT 0x040
> +
> +#define CSR_CUSTOM8_M_RW_BASE 0x7C0
> +#define CSR_CUSTOM8_M_RW_COUNT 0x040
> +
> +#define CSR_CUSTOM9_M_RW_BASE 0xBC0
> +#define CSR_CUSTOM9_M_RW_COUNT 0x040
> +
> +#define CSR_CUSTOM10_M_RO_BASE 0xFC0
> +#define CSR_CUSTOM10_M_RO_COUNT 0x040
> +
> +unsigned long csr_read_num(unsigned long csr_num, int *out_err)
> +{
> +#define switchcase_csr_read(__csr_num) \
> + case (__csr_num): \
> + return csr_read(__csr_num)
> +#define switchcase_csr_read_2(__csr_num) \
> + switchcase_csr_read(__csr_num + 0); \
> + switchcase_csr_read(__csr_num + 1)
> +#define switchcase_csr_read_4(__csr_num) \
> + switchcase_csr_read_2(__csr_num + 0); \
> + switchcase_csr_read_2(__csr_num + 2)
> +#define switchcase_csr_read_8(__csr_num) \
> + switchcase_csr_read_4(__csr_num + 0); \
> + switchcase_csr_read_4(__csr_num + 4)
> +#define switchcase_csr_read_16(__csr_num) \
> + switchcase_csr_read_8(__csr_num + 0); \
> + switchcase_csr_read_8(__csr_num + 8)
> +#define switchcase_csr_read_32(__csr_num) \
> + switchcase_csr_read_16(__csr_num + 0); \
> + switchcase_csr_read_16(__csr_num + 16)
> +#define switchcase_csr_read_64(__csr_num) \
> + switchcase_csr_read_32(__csr_num + 0); \
> + switchcase_csr_read_32(__csr_num + 32)
> +#define switchcase_csr_read_128(__csr_num) \
> + switchcase_csr_read_64(__csr_num + 0); \
> + switchcase_csr_read_64(__csr_num + 64)
> +#define switchcase_csr_read_256(__csr_num) \
> + switchcase_csr_read_128(__csr_num + 0); \
> + switchcase_csr_read_128(__csr_num + 128)
> +
> + if (out_err)
> + *out_err = 0;
> +
> + switch (csr_num) {
> + switchcase_csr_read_32(CSR_CYCLE);
> + switchcase_csr_read_32(CSR_CYCLEH);
> + switchcase_csr_read_256(CSR_CUSTOM0_U_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM1_U_RO_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM2_S_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM3_S_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM4_S_RO_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM5_HS_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM6_HS_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM7_HS_RO_BASE);
> +#ifdef CONFIG_RISCV_M_MODE
> + switchcase_csr_read_64(CSR_CUSTOM8_M_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM9_M_RW_BASE);
> + switchcase_csr_read_64(CSR_CUSTOM10_M_RO_BASE);
> +#endif
> + default:
> + if (out_err)
> + *out_err = -EINVAL;
> + else
> + pr_err("%s: csr 0x%lx not supported\n", __func__, csr_num);
> + break;
> + }
> +
> + return 0;
> +#undef switchcase_csr_read_256
> +#undef switchcase_csr_read_128
> +#undef switchcase_csr_read_64
> +#undef switchcase_csr_read_32
> +#undef switchcase_csr_read_16
> +#undef switchcase_csr_read_8
> +#undef switchcase_csr_read_4
> +#undef switchcase_csr_read_2
> +#undef switchcase_csr_read
> +}
> +EXPORT_SYMBOL_GPL(csr_read_num);
> +
> +void csr_write_num(unsigned long csr_num, unsigned long val, int *out_err)
> +{
> +#define switchcase_csr_write(__csr_num, __val) \
> + case (__csr_num): \
> + csr_write(__csr_num, __val); \
> + break
> +#define switchcase_csr_write_2(__csr_num, __val) \
> + switchcase_csr_write(__csr_num + 0, __val); \
> + switchcase_csr_write(__csr_num + 1, __val)
> +#define switchcase_csr_write_4(__csr_num, __val) \
> + switchcase_csr_write_2(__csr_num + 0, __val); \
> + switchcase_csr_write_2(__csr_num + 2, __val)
> +#define switchcase_csr_write_8(__csr_num, __val) \
> + switchcase_csr_write_4(__csr_num + 0, __val); \
> + switchcase_csr_write_4(__csr_num + 4, __val)
> +#define switchcase_csr_write_16(__csr_num, __val) \
> + switchcase_csr_write_8(__csr_num + 0, __val); \
> + switchcase_csr_write_8(__csr_num + 8, __val)
> +#define switchcase_csr_write_32(__csr_num, __val) \
> + switchcase_csr_write_16(__csr_num + 0, __val); \
> + switchcase_csr_write_16(__csr_num + 16, __val)
> +#define switchcase_csr_write_64(__csr_num, __val) \
> + switchcase_csr_write_32(__csr_num + 0, __val); \
> + switchcase_csr_write_32(__csr_num + 32, __val)
> +#define switchcase_csr_write_128(__csr_num, __val) \
> + switchcase_csr_write_64(__csr_num + 0, __val); \
> + switchcase_csr_write_64(__csr_num + 64, __val)
> +#define switchcase_csr_write_256(__csr_num, __val) \
> + switchcase_csr_write_128(__csr_num + 0, __val); \
> + switchcase_csr_write_128(__csr_num + 128, __val)
> +
> + if (out_err)
> + *out_err = 0;
> +
> + switch (csr_num) {
> + switchcase_csr_write_256(CSR_CUSTOM0_U_RW_BASE, val);
> + switchcase_csr_write_64(CSR_CUSTOM2_S_RW_BASE, val);
> + switchcase_csr_write_64(CSR_CUSTOM3_S_RW_BASE, val);
> + switchcase_csr_write_64(CSR_CUSTOM5_HS_RW_BASE, val);
> + switchcase_csr_write_64(CSR_CUSTOM6_HS_RW_BASE, val);
> +#ifdef CONFIG_RISCV_M_MODE
> + switchcase_csr_write_64(CSR_CUSTOM8_M_RW_BASE, val);
> + switchcase_csr_write_64(CSR_CUSTOM9_M_RW_BASE, val);
> +#endif
> + default:
> + if (out_err)
> + *out_err = -EINVAL;
> + else
> + pr_err("%s: csr 0x%lx not supported\n", __func__, csr_num);
> + break;
> + }
> +#undef switchcase_csr_write_256
> +#undef switchcase_csr_write_128
> +#undef switchcase_csr_write_64
> +#undef switchcase_csr_write_32
> +#undef switchcase_csr_write_16
> +#undef switchcase_csr_write_8
> +#undef switchcase_csr_write_4
> +#undef switchcase_csr_write_2
> +#undef switchcase_csr_write
> +}
> +EXPORT_SYMBOL_GPL(csr_write_num);
> diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> index 42c1a9052470..fe491937ed25 100644
> --- a/drivers/acpi/riscv/cppc.c
> +++ b/drivers/acpi/riscv/cppc.c
> @@ -65,24 +65,19 @@ static void sbi_cppc_write(void *write_data)
> static void cppc_ffh_csr_read(void *read_data)
> {
> struct sbi_cppc_data *data = (struct sbi_cppc_data *)read_data;
> + int err;
>
> - switch (data->reg) {
> - /* Support only TIME CSR for now */
> - case CSR_TIME:
> - data->ret.value = csr_read(CSR_TIME);
> - data->ret.error = 0;
> - break;
> - default:
> - data->ret.error = -EINVAL;
> - break;
> - }
> + data->ret.value = csr_read_num(data->reg, &err);
> + data->ret.error = err;
> }
>
> static void cppc_ffh_csr_write(void *write_data)
> {
> struct sbi_cppc_data *data = (struct sbi_cppc_data *)write_data;
> + int err;
>
> - data->ret.error = -EINVAL;
> + csr_write_num(data->reg, data->val, &err);
> + data->ret.error = err;
> }
>
> /*
> diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c
> index 7644147d50b4..aa053254448d 100644
> --- a/drivers/perf/riscv_pmu.c
> +++ b/drivers/perf/riscv_pmu.c
> @@ -16,6 +16,7 @@
> #include <linux/smp.h>
> #include <linux/sched_clock.h>
>
> +#include <asm/csr.h>
> #include <asm/sbi.h>
>
> static bool riscv_perf_user_access(struct perf_event *event)
> @@ -88,46 +89,6 @@ void arch_perf_update_userpage(struct perf_event *event,
> userpg->cap_user_time_short = 1;
> }
>
> -static unsigned long csr_read_num(int csr_num)
> -{
> -#define switchcase_csr_read(__csr_num, __val) {\
> - case __csr_num: \
> - __val = csr_read(__csr_num); \
> - break; }
> -#define switchcase_csr_read_2(__csr_num, __val) {\
> - switchcase_csr_read(__csr_num + 0, __val) \
> - switchcase_csr_read(__csr_num + 1, __val)}
> -#define switchcase_csr_read_4(__csr_num, __val) {\
> - switchcase_csr_read_2(__csr_num + 0, __val) \
> - switchcase_csr_read_2(__csr_num + 2, __val)}
> -#define switchcase_csr_read_8(__csr_num, __val) {\
> - switchcase_csr_read_4(__csr_num + 0, __val) \
> - switchcase_csr_read_4(__csr_num + 4, __val)}
> -#define switchcase_csr_read_16(__csr_num, __val) {\
> - switchcase_csr_read_8(__csr_num + 0, __val) \
> - switchcase_csr_read_8(__csr_num + 8, __val)}
> -#define switchcase_csr_read_32(__csr_num, __val) {\
> - switchcase_csr_read_16(__csr_num + 0, __val) \
> - switchcase_csr_read_16(__csr_num + 16, __val)}
> -
> - unsigned long ret = 0;
> -
> - switch (csr_num) {
> - switchcase_csr_read_32(CSR_CYCLE, ret)
> - switchcase_csr_read_32(CSR_CYCLEH, ret)
> - default :
> - break;
> - }
> -
> - return ret;
> -#undef switchcase_csr_read_32
> -#undef switchcase_csr_read_16
> -#undef switchcase_csr_read_8
> -#undef switchcase_csr_read_4
> -#undef switchcase_csr_read_2
> -#undef switchcase_csr_read
> -}
> -
> /*
> * Read the CSR of a corresponding counter.
> */
> @@ -139,7 +100,7 @@ unsigned long riscv_pmu_ctr_read_csr(unsigned long csr)
> return -EINVAL;
> }
>
> - return csr_read_num(csr);
> + return csr_read_num(csr, NULL);
> }
>
> u64 riscv_pmu_ctr_get_width_mask(struct perf_event *event)
> --
> 2.43.0
>
Other than the suggestion to flip the interface,
Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
Powered by blists - more mailing lists