| 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: <CAK9=C2UA6EF=RQ0C9qK2CGEun3GGndq_G1UpTju23GNSfRaRbw@mail.gmail.com>
Date: Sat, 16 Aug 2025 14:00:07 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Andrew Jones <ajones@...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 Sat, Aug 16, 2025 at 1:32 AM Andrew Jones <ajones@...tanamicro.com> wrote:
>
> 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.
I had considered this but the problem with this approach is that
individual switch cases in csr_read_num() become roughly 4
instructions because value read from CSR has to written to a
memory location.
The current approach results in just 2 instructions for each
switch-case. Additionally, the current prototypes of csr_read_num()
and csr_write_num() are closer to csr_read() and csr_write()
respectively.
>
> > +
> > #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>
Regards,
Anup
Powered by blists - more mailing lists