linux-kernel - Re: [PATCH v2 1/3] riscv: Add csr_read/write_hi

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>] [day] [month] [year] [list]
Message-ID: <CAKddAkBupSkxWF2hmTDZOvD+-cXeQK8NrUFAz6t3_tXaZZ=fZA@mail.gmail.com>
Date: Wed, 2 Oct 2024 09:57:39 +0800
From: Nick Hu <nick.hu@...ive.com>
To: Andrew Jones <ajones@...tanamicro.com>
Cc: greentime.hu@...ive.com, zong.li@...ive.com, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@....cz>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Anup Patel <anup@...infault.org>, Mayuresh Chitale <mchitale@...tanamicro.com>, 
	Conor Dooley <conor.dooley@...rochip.com>, Atish Patra <atishp@...osinc.com>, 
	Samuel Ortiz <sameo@...osinc.com>, Samuel Holland <samuel.holland@...ive.com>, 
	Sunil V L <sunilvl@...tanamicro.com>, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 1/3] riscv: Add csr_read/write_hi_lo support

It seems like my last mail didn't send out successfully.

On Wed, Oct 2, 2024 at 9:52 AM Nick Hu <nick.hu@...ive.com> wrote:
>
> Hi Andrew
>
> On Thu, Sep 26, 2024 at 3:59 PM Andrew Jones <ajones@...tanamicro.com> wrote:
>>
>> On Thu, Sep 26, 2024 at 02:54:16PM GMT, Nick Hu wrote:
>> > In RV32, we may have the need to read both low 32 bit and high 32 bit of
>> > the CSR. Therefore adding the csr_read_hi_lo() and csr_write_hi_lo() to
>> > support such case.
>> >
>> > Suggested-by: Andrew Jones <ajones@...tanamicro.com>
>> > Suggested-by: Zong Li <zong.li@...ive.com>
>> > Signed-off-by: Nick Hu <nick.hu@...ive.com>
>> > ---
>> >  arch/riscv/include/asm/csr.h | 22 ++++++++++++++++++++++
>> >  1 file changed, 22 insertions(+)
>> >
>> > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>> > index 25966995da04..54198284eb22 100644
>> > --- a/arch/riscv/include/asm/csr.h
>> > +++ b/arch/riscv/include/asm/csr.h
>> > @@ -501,6 +501,17 @@
>> >       __v;                                                    \
>> >  })
>> >
>> > +#if __riscv_xlen < 64
>> > +#define csr_read_hi_lo(csr)                                  \
>> > +({                                                           \
>> > +     u32 hi = csr_read(csr##H);                              \
>> > +     u32 lo = csr_read(csr);                                 \
>> > +     lo | ((u64)hi << 32);                                   \
>> > +})
>> > +#else
>> > +#define csr_read_hi_lo       csr_read
>> > +#endif
>> > +
>> >  #define csr_write(csr, val)                                  \
>> >  ({                                                           \
>> >       unsigned long __v = (unsigned long)(val);               \
>> > @@ -509,6 +520,17 @@
>> >                             : "memory");                      \
>> >  })
>> >
>> > +#if __riscv_xlen < 64
>> > +#define csr_write_hi_lo(csr, val)                            \
>> > +({                                                           \
>> > +     u64 _v = (u64)(val);                                    \
>> > +     csr_write(csr##H, (_v) >> 32);                          \
>> > +     csr_write(csr, (_v));                                   \
>> > +})
>> > +#else
>> > +#define csr_write_hi_lo      csr_write
>> > +#endif
>> > +
>> >  #define csr_read_set(csr, val)                                       \
>> >  ({                                                           \
>> >       unsigned long __v = (unsigned long)(val);               \
>> > --
>> > 2.34.1
>> >
>>
>> I know I suggested this, but I'm having second thoughts. The nice thing
>> about the
>>
>>  csr_write(CSR, ...);
>>  if (__riscv_xlen < 64)
>>     csr_write(CSRH, ...);
>>
>> pattern is that it matches the spec. With this helper we'll have
>>
>>  csr_write_hi_lo(CSR, ...);
>>
>> for both rv32 and rv64. That looks odd for rv64 and hides the hi register
>> access for rv32.
>>
>> We could avoid the oddness of the helper's name for rv64 if we instead
>> added csr_write32 and csr_write64 which do the right things, but that
>> still hides the hi register access for rv32. Hiding the hi register
>> access is probably fine, though, since we can be pretty certain that
>> the spec will rarely, if ever, deviate from naming high registers with
>> the H suffix and/or not keep the upper bits compatible.
>>
>> In summary, I think I'm in favor of just dropping this patch, keeping
>> the noisy, but explicit, pattern. Or, if the consensus is to add
>> helpers, then I'd rather have all csr_<op>32/64 helpers. Then, code
>> would match the spec by choosing the right helper based on the width
>> of the CSR being accessed, when the CSR has an explicit width, or still
>> use the current helpers for xlen-wide CSRs.
>>
>> Thanks,
>> drew
>
Sure I'll drop the patch in the next version


Regards,
Nick