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

[Conor Dooley]

On Thu, Apr 27, 2023 at 07:28:49PM +0100, Conor Dooley wrote:
> 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?

I think what I missed is that for riscv,isa containing `_xtheadvector`
but a kernel without support for vector then we'd hit Andy's
trap-on-first-use and that's one of the cases that don't need to be
considered here.

> 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