MIME-Version: 1.0
References: <20230314183220.513101-1-evan@rivosinc.com> <20230314183220.513101-5-evan@rivosinc.com>
 <1846748.tdWV9SEqCh@diego>
In-Reply-To: <1846748.tdWV9SEqCh@diego>
From:   Evan Green <evan@rivosinc.com>
Date:   Tue, 21 Mar 2023 08:35:01 -0700
Message-ID: <CALs-Hssz-GJ7zY5pnSjbXHzyu-uy3+UYo54rWA8nAjYBaeNk1g@mail.gmail.com>
Subject: Re: [PATCH v4 4/6] RISC-V: hwprobe: Support probing of misaligned
 access performance
To:     =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>
Cc:     Palmer Dabbelt <palmer@rivosinc.com>, slewis@rivosinc.com,
        Conor Dooley <conor@kernel.org>, vineetg@rivosinc.com,
        Albert Ou <aou@eecs.berkeley.edu>,
        Andrew Bresticker <abrestic@rivosinc.com>,
        Andrew Jones <ajones@ventanamicro.com>,
        Anup Patel <apatel@ventanamicro.com>,
        Atish Patra <atishp@rivosinc.com>,
        Celeste Liu <coelacanthus@outlook.com>,
        Conor Dooley <conor.dooley@microchip.com>,
        Guo Ren <guoren@kernel.org>,
        Jisheng Zhang <jszhang@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Philipp Tomsich <philipp.tomsich@vrull.eu>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Sunil V L <sunilvl@ventanamicro.com>,
        Tsukasa OI <research_trasio@irq.a4lg.com>,
        Wei Fu <wefu@redhat.com>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 17, 2023 at 3:08=E2=80=AFAM Heiko St=C3=BCbner <heiko@sntech.de=
> wrote:
>
> Hi Evan,
>
> Am Dienstag, 14. M=C3=A4rz 2023, 19:32:18 CET schrieb Evan Green:
> > This allows userspace to select various routines to use based on the
> > performance of misaligned access on the target hardware.
>
> I really like this implementation.
>
> Also interesting that T-Head actually has a fast unaligned access.
> Maybe that should be part of the commit message (including were
> this information comes from)

Thanks Heiko (and Conor)! Yep, you both noticed that, I'll add a descriptio=
n.

>
>
> > Co-developed-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Signed-off-by: Evan Green <evan@rivosinc.com>
> >
> > ---
> >
> > Changes in v4:
> >  - Add newlines to CPUPERF_0 documentation (Conor)
> >  - Add UNSUPPORTED value (Conor)
> >  - Switched from DT to alternatives-based probing (Rob)
> >  - Crispen up cpu index type to always be int (Conor)
> >
> > Changes in v3:
> >  - Have hwprobe_misaligned return int instead of long.
> >  - Constify cpumask pointer in hwprobe_misaligned()
> >  - Fix warnings in _PERF_O list documentation, use :c:macro:.
> >  - Move include cpufeature.h to misaligned patch.
> >  - Fix documentation mismatch for RISCV_HWPROBE_KEY_CPUPERF_0 (Conor)
> >  - Use for_each_possible_cpu() instead of NR_CPUS (Conor)
> >  - Break early in misaligned access iteration (Conor)
> >  - Increase MISALIGNED_MASK from 2 bits to 3 for possible UNSUPPORTED f=
uture
> >    value (Conor)
> >
> > Changes in v2:
> >  - Fixed logic error in if(of_property_read_string...) that caused cras=
h
> >  - Include cpufeature.h in cpufeature.h to avoid undeclared variable
> >    warning.
> >  - Added a _MASK define
> >  - Fix random checkpatch complaints
> >
> >  Documentation/riscv/hwprobe.rst       | 21 ++++++++++++++++++++
> >  arch/riscv/errata/thead/errata.c      |  9 +++++++++
> >  arch/riscv/include/asm/alternative.h  |  5 +++++
> >  arch/riscv/include/asm/cpufeature.h   |  2 ++
> >  arch/riscv/include/asm/hwprobe.h      |  2 +-
> >  arch/riscv/include/uapi/asm/hwprobe.h |  7 +++++++
> >  arch/riscv/kernel/alternative.c       | 19 ++++++++++++++++++
> >  arch/riscv/kernel/cpufeature.c        |  3 +++
> >  arch/riscv/kernel/smpboot.c           |  1 +
> >  arch/riscv/kernel/sys_riscv.c         | 28 +++++++++++++++++++++++++++
> >  10 files changed, 96 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwpr=
obe.rst
> > index 945d44683c40..9f0dd62dcb5d 100644
> > --- a/Documentation/riscv/hwprobe.rst
> > +++ b/Documentation/riscv/hwprobe.rst
> > @@ -63,3 +63,24 @@ The following keys are defined:
> >
> >    * :c:macro:`RISCV_HWPROBE_IMA_C`: The C extension is supported, as d=
efined
> >      by version 2.2 of the RISC-V ISA manual.
> > +
> > +* :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains perf=
ormance
> > +  information about the selected set of processors.
> > +
> > +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNKNOWN`: The performance of mi=
saligned
> > +    accesses is unknown.
> > +
> > +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_EMULATED`: Misaligned accesses =
are
> > +    emulated via software, either in or below the kernel.  These acces=
ses are
> > +    always extremely slow.
> > +
> > +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_SLOW`: Misaligned accesses are =
supported
> > +    in hardware, but are slower than the cooresponding aligned accesse=
s
> > +    sequences.
> > +
> > +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_FAST`: Misaligned accesses are =
supported
> > +    in hardware and are faster than the cooresponding aligned accesses
> > +    sequences.
> > +
> > +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNSUPPORTED`: Misaligned access=
es are
> > +    not supported at all and will generate a misaligned address fault.
> > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead=
/errata.c
> > index fac5742d1c1e..f41a45af5607 100644
> > --- a/arch/riscv/errata/thead/errata.c
> > +++ b/arch/riscv/errata/thead/errata.c
> > @@ -10,7 +10,9 @@
> >  #include <linux/uaccess.h>
> >  #include <asm/alternative.h>
> >  #include <asm/cacheflush.h>
> > +#include <asm/cpufeature.h>
> >  #include <asm/errata_list.h>
> > +#include <asm/hwprobe.h>
> >  #include <asm/patch.h>
> >  #include <asm/vendorid_list.h>
> >
> > @@ -108,3 +110,10 @@ void __init_or_module thead_errata_patch_func(stru=
ct alt_entry *begin, struct al
> >       if (stage =3D=3D RISCV_ALTERNATIVES_EARLY_BOOT)
> >               local_flush_icache_all();
> >  }
> > +
> > +void thead_feature_probe_func(unsigned int cpu, unsigned long archid,
> > +                           unsigned long impid)
> > +{
> > +     if ((archid =3D=3D 0) && (impid =3D=3D 0))
> > +             per_cpu(misaligned_access_speed, cpu) =3D RISCV_HWPROBE_M=
ISALIGNED_FAST;
>
> When looking at this function I 'm wondering if we also want to expose
> the active erratas somehow (not in this patch of course, just in general)

I suppose as Arnd pointed out in a different thread there's sort of a
tension between this mechanism and /proc/cpuinfo, the traditional spot
for exposing more standard cpu features/errata. Though if we think of
this mechanism as a sort of surrogate for cpuid, then it potentially
does make sense. My gut says it's a judgment call.

-Evan