linux-kernel - Re: [PATCH v3 1/1] RISC-V: Add common csr_read_num() and csr_write

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAK9=C2VdvPU6b+vbL2KwnvaVo47ayNOLb-bH0UwjRj=MHe0V9A@mail.gmail.com>
Date: Mon, 24 Nov 2025 10:19:18 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Paul Walmsley <pjw@...nel.org>
Cc: Anup Patel <anup@...infault.org>, Mark Rutland <mark.rutland@....com>, 
	Alexandre Ghiti <alex@...ti.fr>, "Rafael J . Wysocki" <rafael@...nel.org>, Atish Patra <atish.patra@...ux.dev>, 
	Atish Patra <atishp@...osinc.com>, linux-kernel@...r.kernel.org, 
	linux-acpi@...r.kernel.org, Palmer Dabbelt <palmer@...belt.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Nutty Liu <nutty.liu@...mail.com>, 
	linux-riscv@...ts.infradead.org, Andrew Jones <ajones@...tanamicro.com>, 
	Will Deacon <will@...nel.org>, Len Brown <lenb@...nel.org>
Subject: Re: [PATCH v3 1/1] RISC-V: Add common csr_read_num() and
 csr_write_num() functions

On Sat, Nov 22, 2025 at 6:44 AM Paul Walmsley <pjw@...nel.org> wrote:
>
> On Thu, 30 Oct 2025, Anup Patel wrote:
>
> > On Thu, Oct 30, 2025 at 12:35 AM Paul Walmsley <pjw@...nel.org> wrote:
> > > On Tue, 14 Oct 2025, 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.
> > >
> > > This patch also (silently) removes the CSR number filtering, e.g.
> > >
> > > > 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;
> > > > -     }
> > >
> > > ... the above code, and:
> >
> > The switch case is incomplete for cppc_ffh_csr_read().
> > Also, csr_read_num() already does appropriate filtering
> > so the switch case over here is now redundant.
> >
> > >
> > > >  /*
> > > >   * 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)) {
> > > > -             pr_err("Invalid performance counter csr %lx\n", csr);
> > > > -             return -EINVAL;
> > >
> > > ... the above code.
> > >
> > > I'm thinking that we probably want to keep the CSR number filtering code
> > > in; at least, I can't think of a good reason to remove it.  Care to add it
> > > back in?
> >
> > We can potentially have custom CSRs as hardware counters
> > hence the CSR filtering over here is already incomplete. Plus,
> > csr_read_num() already does the CSR filtering and returns
> > failure for inappropriate CSR number.
>
> OK.  To me, the most notable changes in this patch are the changes in the
> filters for both reads and writes for the two different call sites, ACPI
> FFH and PMU.  It would be good to document these filter changes directly
> in the patch description, along with the rationale.
>
> I'm also not yet completely convinced that we should allow both of these
> call sites to read and write any custom CSR.  But maybe the updated patch
> description might be convincing...

The RISC-V ACPI FFH specification allows a 12-bit CSR number in
_CPC object encoding so any CSR can be mapped as a CPPC register
(including custom CSRs).

Similarly, the RISC-V SBI PMU specification return 12-bit CSR number
in sbi_pmu_counter_get_info() so a platform can use custom CSRs for
additional HW counters.

I will update the commit description accordingly.

Regards,
Anup