Re: [PATCH 02/19] riscv: cpufeature: Fix thead vector hwcap removal

[Evan Green]

On Tue, Apr 16, 2024 at 9:25 PM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
>
> On Tue, Apr 16, 2024 at 08:36:33AM +0100, Conor Dooley wrote:
> > On Mon, Apr 15, 2024 at 08:34:05PM -0700, Charlie Jenkins wrote:
> > > On Sat, Apr 13, 2024 at 12:40:26AM +0100, Conor Dooley wrote:
> > > > On Fri, Apr 12, 2024 at 02:31:42PM -0700, Charlie Jenkins wrote:
> > > > > On Fri, Apr 12, 2024 at 10:27:47PM +0100, Conor Dooley wrote:
> > > > > > On Fri, Apr 12, 2024 at 01:48:46PM -0700, Charlie Jenkins wrote:
> > > > > > > On Fri, Apr 12, 2024 at 07:47:48PM +0100, Conor Dooley wrote:
> > > > > > > > On Fri, Apr 12, 2024 at 10:12:20AM -0700, Charlie Jenkins wrote:
> > > >
> > > > > > > > > This is already falling back on the boot CPU, but that is not a solution
> > > > > > > > > that scales. Even though all systems currently have homogenous
> > > > > > > > > marchid/mvendorid I am hesitant to assert that all systems are
> > > > > > > > > homogenous without providing an option to override this.
> > > > > > > >
> > > > > > > > There are already is an option. Use the non-deprecated property in your
> > > > > > > > new system for describing what extesions you support. We don't need to
> > > > > > > > add any more properties (for now at least).
> > > > > > >
> > > > > > > The issue is that it is not possible to know which vendor extensions are
> > > > > > > associated with a vendor. That requires a global namespace where each
> > > > > > > extension can be looked up in a table. I have opted to have a
> > > > > > > vendor-specific namespace so that vendors don't have to worry about
> > > > > > > stepping on other vendor's toes (or the other way around). In order to
> > > > > > > support that, the vendorid of the hart needs to be known prior.
> > > > > >
> > > > > > Nah, I think you're mixing up something like hwprobe and having
> > > > > > namespaces there with needing namespacing on the devicetree probing side
> > > > > > too. You don't need any vendor namespacing, it's perfectly fine (IMO)
> > > > > > for a vendor to implement someone else's extension and I think we should
> > > > > > allow probing any vendors extension on any CPU.
> > > > >
> > > > > I am not mixing it up. Sure a vendor can implement somebody else's
> > > > > extension, they just need to add it to their namespace too.
> > > >
> > > > I didn't mean that you were mixing up how your implementation worked, my
> > > > point was that you're mixing up the hwprobe stuff which may need
> > > > namespacing for $a{b,p}i_reason and probing from DT which does not.
> > > > I don't think that the kernel should need to be changed at all if
> > > > someone shows up and implements another vendor's extension - we already
> > > > have far too many kernel changes required to display support for
> > > > extensions and I don't welcome potential for more.
> > >
> > > Yes I understand where you are coming from. We do not want it to require
> > > very many changes to add an extension. With this framework, there are
> > > the same number of changes to add a vendor extension as there is to add
> > > a standard extension.
> >
> > No, it is actually subtly different. Even if the kernel already supports
> > the extension, it needs to be patched for each vendor
> >
> > > There is the upfront cost of creating the struct
> > > for the first vendor extension from a vendor, but after that the
> > > extension only needs to be added to the associated vendor's file (I am
> > > extracting this out to a vendor file in the next version). This is also
> > > a very easy task since the fields from a different vendor can be copied
> > > and adapted.
> > >
> > > > Another thing I just thought of was systems where the SoC vendor
> > > > implements some extension that gets communicated in the ISA string but
> > > > is not the vendor in mvendorid in their various CPUs. I wouldn't want to
> > > > see several different entries in structs (or several different hwprobe
> > > > keys, but that's another story) for this situation because you're only
> > > > allowing probing what's in the struct matching the vendorid.
> > >
> > > Since the isa string is a per-hart field, the vendor associated with the
> > > hart will be used.
> >
> > I don't know if you just didn't really read what I said or didn't
> > understand it, but this response doesn't address my comment.
>
> I read what you said! This question seemed to me as another variant of
> "what happens when one vendor implements an extension from a different
> vendor", and since we already discussed that I was trying to figure out
> what you were actually asking.
>
> > Consider SoC vendor S buys CPUs from vendors A & B and asks both of them
> > to implement Xsjam. The CPUs are have the vendorid of either A or B,
> > depending on who made it. This scenario should not result in two
> > different hwprobe keys nor two different in-kernel riscv_has_vendor_ext()
> > checks to see if the extension is supported. *If* the extension is vendor
> > namespaced, it should be to the SoC vendor whose extension it is, not
> > the individual CPU vendors that implemented it.
> >
> > Additionally, consider that CPUs from both vendors are in the same SoC
> > and all CPUs support Xsjam. Linux only supports homogeneous extensions
> > so we should be able to detect that all CPUs support the extension and
> > use it in a driver etc, but that's either not going to work (or be
> > difficult to orchestrate) with different mappings per CPU vendor. I saw
> > your v2 cover letter, in which you said:
> >   Only patch vendor extension if all harts are associated with the same
> >   vendor. This is the best chance the kernel has for working properly if
> >   there are multiple vendors.
> > I don't think that level of paranoia is required: if firmware tells us
> > that an extension is supported, then we can trust that those extensions
> > have been implemented correctly. If the fear of implementation bugs is
> > what is driving the namespacing that you've gone for, I don't think that
> > it is required and we can simplify things, with the per-vendor structs
> > being the vendor of the extension (so SoC vendor S in my example), not
> > A and B who are the vendors of the CPU IP.
> >
> > Thanks,
> > Conor.
> >
>
> Thank you for expanding upon this idea further. This solution of
> indexing the extensions based on the vendor who proposed them does make
> a lot of sense. There are some key differences here of note. When
> vendors are able to mix vendor extensions, defining a bitmask that
> contains all of the vendor extensions gets a bit messier. I see two
> possible solutions.
>
> 1. Vendor keys cannot overlap between vendors. A set bit in the bitmask
> is associated with exactly one extension.
>
> 2. Vendor keys can overlap between vendors. There is a vendor bitmask
> per vendor. When setting/checking a vendor extension, first index into
> the vendor extension bitmask with the vendor associated with the
> extension and then with the key of the vendor extension.
>
> A third option would be to use the standard extension framework. This
> causes the standard extension list to become populated with extensions
> that most harts will never implement so I am opposed to that.
>
> This problem carries over into hwprobe since the schemes proposed by
> Evan and I both rely on the mvendorid of harts associated with the
> cpumask. To have this level of support in hwprobe for SoCs with a mix of
> vendors but the same extensions I again see two options:
>
> 1. Vendor keys cannot overlap between vendors. A set bit in the bitmask
> is associated with exactly one extension. This bitmask would be returned
> by the vendor extension hwprobe key.
>
> 2. Vendor keys can overlap between vendors. There is an hwprobe key per
> vendor. Automatic resolution of the vendor doesn't work because the
> vendor-specific feature being requested (extensions in the case) may be
> of a vendor that is different than the hart's vendor, in otherwords
> there are two variables necessary: the vendor and a way to ask hwprobe
> for a list of the vendor extensions. With hwprobe there is only the
> "key" that can be used to encode these variables simultaneously. We
> could have something like a HWPROBE_THEAD_EXT_0 key that would return
> all thead vendor extensions supported by the harts corresponding to the
> cpumask.

I was a big proponent of the vendor namespacing in hwprobe, as I liked
the tidiness of it, and felt it could handle most cases (including
mix-n-matching multiple mvendorids in a single SoC). However my
balloon lost its air after chatting with Palmer, as there's one case
it really can't handle: white labeling. This is where I buy a THead
(for instance) CPU for my SoC, including all its vendor extensions,
and do nothing but change the mvendorid to my own. If this is a thing,
then the vendor extensions basically have to be a single global
namespace in hwprobe (sigh).

I do like Charlie's idea of at least letting vendors allocate a key at
a time, eg HWPROBE_THEAD_EXT_0, rather than racing to allocate a bit
at a time in a key like HWPROBE_VENDOR_EXT_0. That gives it some
semblance of organization, and still gives us a chance of a
cleanup/deprecation path for vendors that stop producing chips.
-Evan