Messages in this thread | | | Date | Fri, 28 Apr 2023 08:53:49 +0100 | From | Conor Dooley <> | Subject | Re: [PATCH 4/4] RISC-V: add support for vendor-extensions via AT_BASE_PLATFORM and xthead |
| |
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@vrull.eu> > > > > 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.
[unhandled content-type:application/pgp-signature] | |