Message-ID: <5016896.Mh6RI2rZIc@diego>
Date:   Thu, 27 Apr 2023 19:15:58 +0200
From:   Heiko Stübner <heiko@...ech.de>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     palmer@...belt.com, linux-riscv@...ts.infradead.org,
        paul.walmsley@...ive.com, kito.cheng@...ive.com, jrtc27@...c27.com,
        matthias.bgg@...il.com, heinrich.schuchardt@...onical.com,
        greentime.hu@...ive.com, nick.knight@...ive.com,
        christoph.muellner@...ll.eu, philipp.tomsich@...ll.eu,
        richard.henderson@...aro.org, arnd@...db.de,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] RISC-V: add support for vendor-extensions via
 AT_BASE_PLATFORM and xthead

Hey Conor,

Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley:
> On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@...ll.eu>
> > 
> > T-Head cores support a number of own ISA extensions that also include
> > optimized instructions which could benefit userspace to improve
> > performance.
> > 
> > Extensions supported by current T-Head cores are:
> > * XTheadBa - bitmanipulation instructions for address calculation
> > * XTheadBb - conditional basic bit-manipulation instructions
> > * XTheadBs - instructions to access a single bit in a register
> > * XTheadCmo - cache management operations
> > * XTheadCondMov - conditional move instructions
> > * XTheadFMemIdx - indexed memory operations for floating-point registers
> > * XTheadFmv - double-precision floating-point high-bit data transmission
> >               intructions for RV32
> > * XTheadInt - instructions to reduce the code size of ISRs and/or the
> >               interrupt latencies that are caused by ISR entry/exit code
> > * XTheadMac - multiply-accumulate instructions
> > * XTheadMemIdx - indexed memory operations for GP registers
> > * XTheadMemPair - two-GPR memory operations
> > * XTheadSync - multi-core synchronization instructions
> > 
> > In-depth descriptions of these extensions can be found on
> >     https://github.com/T-head-Semi/thead-extension-spec
> > 
> > Support for those extensions was merged into the relevant toolchains
> > so userspace programs can select necessary optimizations when needed.
> > 
> > So a mechanism to the isa-string generation to export vendor-extension
> > lists via the errata mechanism and implement it for T-Head C9xx cores.
> > 
> > This exposes these vendor extensions then both in AT_BASE_PLATFORM
> > and /proc/cpuinfo.
> 
> I'm not entirely sure if this patch is meant to be a demo, but I don't
> like the idea of using these registers to determine what extensions are
> reported.

It took me a while to grasp the above, but I think you mean determining
extensions based on mvendor etc, right?


> riscv,isa in a devicetree (for as much as I might dislike it at this
> point in time), or the ACPI equivalent, should be the mechanism for
> enabling/disabling these kinds of things.

> Otherwise, we are just going to end up causing problems for ourselves
> with various lists of this that and the other extension for different
> combinations of hardware.
> The open source c906 has the same archid/impid too right? Assuming this is
> a serious proposal, how would you intend dealing with modified versions
> of those cores?
> 
> I am pretty sure that you intended this to be a demo though, particularly
> given the wording of the below quote from your cover,

yeah, this one was more following a train of thought. Thinking about the
issues, this was more of an addon thought, as I wasn't really sure which
way to go.

So you're right, vendor isa-extensions should also come from the ISA
string from firmware, similar to the base extensions. Not based on the
mvendor-id and friends.



> but in case you did
> not:
> 
> NAK to this way of sourcing the information.
> 
> Anyways, since your cover's considerations section seems partly aimed at
> me, given my discussion with head-the-ball last week:
> 
> > Things to still consider:
> > -------------------------
> > Right now both hwprobe and this approach will only pass through
> > extensions the kernel actually knows about itself. This should not
> > necessarily be needed (but could be an optional feature for e.g. virtualization).
> 
> What do you mean by virtualisation here? It's the job of the hypervisor
> etc to make sure that what it passes to its guest contains only what it
> wants the guest to see, right?
> IIUC, that's another point against doing what this patch does.

I guess I'm still seeing Zbb and friends - with just computational
instructions as always good to have. But I guess you're right that the
hypervisor should be able to control itself which extensions.


> > Most extensions don’t introduce new user-mode state that the kernel needs to
> > manage (e.g. new registers). Extension that do introduce new user-mode state
> > are usually disabled by default and have to be enabled by S mode or M mode
> > (e.g. FS[1:0] for the +floating-point extension). So there should not be a
> > reason to filter any extensions that are unknown.
> 
> I think in general this can be safely assumed, but I don't think it is
> unreasonable to expect someone may make, for example, XConorGigaVector
> that gets turned on by the same bits as regular old vector but has some
> extra registers.
> Not saying that I think that that is a good idea, but it is a distinct
> possibility that this will happen, and I don't think forwarding it to
> userspace is a good idea.

The thead-vector (0.7.1) would probably fit this description. Though in
that case, userspace definitly needs to know about it, to use it :-) .

But of course this should only be forwarded when relevant support
is available in the kernel.


> > So it might make more sense to just pass through a curated list (common
> > set) created from the core's isa strings and remove state-handling
> > extensions when they are not enabled in the kernel-side (sort of
> > blacklisting extensions that need actual kernel support).
> 
> Yeah, as discussed with Christoph the other day I don't think we can
> really avoid such a blacklist. I don't think it'd require any sort of
> vendor specific handling, since, as you point out, a vendor may well
> implement extensions that were created by other companies.
> 
> It's easy, right?? "Just" parse the dt, tokenise the extensions & delete
> whatever is in the blacklist, right?

And then reality happens ;-)


> Hyperbole aside, I think that doing something like this increases the
> need for a system like Evan's, as userspace may need a way to
> differentiate between what the hardware is capable of (as reported by
> the isa string in /proc/cpuinfo or the content of 3/4) and what the
> kernel actually supports.
>
> > However, this is a very related, but still independent discussion.
> 
> Aye, this discussion and the first two patches are relevant whether 3/4
> is accepted or not IMO.

I guess I'll poke this some more in the meantime, taking into account
all the comments from above :-) .


Thanks
Heiko

Powered by blists - more mailing lists