linux-kernel - Re: [PATCH v2 2/2] 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=C2Xt2RYm90wFBiYg+8Wpui1e3h8U72OkG5J5u2UFS=AfWg@mail.gmail.com>
Date: Tue, 19 Aug 2025 16:31:03 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Yao Zi <ziyao@...root.org>
Cc: Mark Rutland <mark.rutland@....com>, Alexandre Ghiti <alex@...ti.fr>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Anup Patel <anup@...infault.org>, 
	Atish Patra <atish.patra@...ux.dev>, linux-kernel@...r.kernel.org, 
	linux-acpi@...r.kernel.org, Palmer Dabbelt <palmer@...belt.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, linux-riscv@...ts.infradead.org, 
	Andrew Jones <ajones@...tanamicro.com>, Will Deacon <will@...nel.org>, Len Brown <lenb@...nel.org>
Subject: Re: [PATCH v2 2/2] RISC-V: Add common csr_read_num() and
 csr_write_num() functions

On Tue, Aug 19, 2025 at 9:43 AM Yao Zi <ziyao@...root.org> wrote:
>
> On Tue, Aug 19, 2025 at 09:00:03AM +0530, Anup Patel wrote:
> > On Tue, Aug 19, 2025 at 8:56 AM Yao Zi <ziyao@...root.org> wrote:
> > >
> > > On Mon, Aug 18, 2025 at 08:06:00PM +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>
> > > > Reviewed-by: Sunil V L <sunilvl@...tanamicro.com>
> > > > ---
> > > >  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 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);
> > >
> > > I think it's more consistent to directly return the error code, and for
> > > csr_read_num, we could pass out the read value by a pointer. e.g.
> > >
> > >         int csr_read_num(unsigned long csr_num, unsigned long *val);
> > >         int csr_write_num(unsigned long csr_num, unsigned long val);
> > >
> > > This allows the caller to eliminate a variable for temporarily storing
> > > the error code if they use it just after the invokation, and fits the
> > > common convention of Linux better.
> >
> > Drew had similar comments so see my response in the previous
> > patch revision. (Refer, https://www.spinics.net/lists/kernel/msg5808113.html)
>
> Thanks for the reference.
>
> > 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.
>
> You could return a structure smaller than or equal to 2 * XLEN from
> csr_read_num(), according to the ABI it could be passed in a0 and a1 and
> thus should require no memory operation.
>
> Let's assume we have
>
>         struct __csr_read_ret {
>                 long error;
>                 unsigned long value;
>         };
>
>         struct __csr_read_ret __csr_read_num(unsigned long csr_num);
>
> Then a wrapper like
>
>         /* piece of untested code */
>         static inline int csr_read_num(unsigned long csr_num,
>                                        unsigned long *val)
>         {
>                 struct __csr_read_ret ret = __csr_read_num(csr_num);
>                 *val = ret.value;
>                 return ret.error;
>         }
>
> could provide an interface that I've talked about earlier, and it
> follows the kernel's convention.

Like I mentioned previously, the current implementation tries to
minimize instructions for each switch case and avoid unnecessary
memory load/store. Your alternate suggestion is no better in this
respect.

>
> > 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.
>
> But csr_read() and csr_write() never directly raise errors that is
> expected to be handled by the caller. I don't think it's a fair
> comparison.

csr_read_num() and csr_write_num() are different because
these functions take CSR number as parameter so caller can
pass an unsupported value to these functions which needs to
be reported back as an error.

Regards,
Anup