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 for Android: free password hash cracker in your pocket

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

Message-ID: <60f4c500-6e22-ed4b-912b-c9368bad7c9a@kernel.org>
Date: Fri, 21 Nov 2025 18:14:24 -0700 (MST)
From: Paul Walmsley <pjw@...nel.org>
To: Anup Patel <anup@...infault.org>
cc: Paul Walmsley <pjw@...nel.org>, Anup Patel <apatel@...tanamicro.com>, 
    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 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...


- Paul