Re: [PATCH 4/4] RISC-V: add support for vendor-extensions via AT_BASE_PLATFORM and xthead

[Conor Dooley]

Hey Heiko,

On Thu, Apr 27, 2023 at 07:15:58PM +0200, Heiko Stübner wrote:
> 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@xxxxxxxx>

> > 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?

Yes, sorry. Apologies if that was not clear. I suppose the SBI
implementation could (as ours does!) could report something different to
the registers themselves, so using that word is probably not a good idea
anyway.

> > 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.

:)

> > > 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.

Yah, there may not be any obvious downsides to something like Zbb, but I
think that taking control away from the hypervisors etc isn't a good
idea.
Having a simple policy of blocking things that are known to misbehave
would require less maint. than a list of things that are okay to pass
through, but both are probably cans-of-worms.
I think we need to think carefully about what policy is chosen here.
Allowlist will be slower, but at least we'll not tell userspace
something that is not usable. Blocklist will be easier to manage, but
can only be reactive.

> > > 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.

Right. IIRC, the plan for that is to add `v` to riscv,isa & alternatives
will do the rest as opposed to doing an `_xtheadvector` type thing.

Assuming the latter for a moment, we'd have to blacklist `_xheadvector`
for kernels compiled without vector support even if the relevant support
is added to the kernel. Similarly, we'd have to blacklist it for kernels
with vector support, but without the erratum enabled.

I think the plan was the former though, so you'd have to block passing
`v` to userspace if vector is enabled and the erratum is not supported.
Should ERRATA_THEAD_VECTOR be mandatory then for RISCV_ISA_VECTOR &&
ERRATA_THEAD kernels? What am I missing?

Also, in a world where we do do some sort of passing, should we only
forward the vendor extensions, or should we forward the standard ones
too?
What about supervisor mode only stuff? There's a bunch of questions to
consider here, even if for some of them the answer may be obvious.

As I said, not really bothered about hwprobe, aux vector etc, but this
side of things is particularly interesting to me.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature