Received-SPF: pass (google.com: domain of linux-kernel+bounces-148872-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240411-dev-charlie-support_thead_vector_6_9-v1-2-4af9815ec746@rivosinc.com>
 <20240412-tuesday-resident-d9d07e75463c@wendy> <ZhlrdGXfSushUNTp@ghost>
 <20240412-eastcoast-disparity-9c9e7d178df5@spud> <ZhmeLoPS+tsfqv1T@ghost>
 <20240412-chemist-haunt-0a30a8f280ca@spud> <ZhmoPuoR00aS6qZp@ghost>
 <20240413-sharper-unlivable-5a65660b19e2@spud> <Zh3xrTfjjk3b4GHb@ghost>
 <20240416-husband-flavored-96c1dad58b6e@wendy> <Zh9PLhUZmCF5mLXc@ghost>
In-Reply-To: <Zh9PLhUZmCF5mLXc@ghost>
From: Evan Green <evan@rivosinc.com>
Date: Wed, 17 Apr 2024 09:02:05 -0700
Message-ID: <CALs-HssYAHrBy-uYKNT_zS02F_65qJZh80OD-W1RrmmYosFU=Q@mail.gmail.com>
Subject: Re: [PATCH 02/19] riscv: cpufeature: Fix thead vector hwcap removal
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: Conor Dooley <conor.dooley@microchip.com>, Conor Dooley <conor@kernel.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Albert Ou <aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>, 
	Conor Dooley <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>, 
	Jernej Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, 
	=?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>, 
	Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>, linux-riscv@lists.infradead.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Palmer Dabbelt <palmer@rivosinc.com>, linux-arm-kernel@lists.infradead.org, 
	linux-sunxi@lists.linux.dev, linux-doc@vger.kernel.org, 
	linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 16, 2024 at 9:25=E2=80=AFPM Charlie Jenkins <charlie@rivosinc.c=
om> 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 w=
rote:
> > > >
> > > > > > > > > This is already falling back on the boot CPU, but that is=
 not a solution
> > > > > > > > > that scales. Even though all systems currently have homog=
enous
> > > > > > > > > marchid/mvendorid I am hesitant to assert that all system=
s are
> > > > > > > > > homogenous without providing an option to override this.
> > > > > > > >
> > > > > > > > There are already is an option. Use the non-deprecated prop=
erty in your
> > > > > > > > new system for describing what extesions you support. We do=
n't need to
> > > > > > > > add any more properties (for now at least).
> > > > > > >
> > > > > > > The issue is that it is not possible to know which vendor ext=
ensions are
> > > > > > > associated with a vendor. That requires a global namespace wh=
ere 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 prio=
r.
> > > > > >
> > > > > > Nah, I think you're mixing up something like hwprobe and having
> > > > > > namespaces there with needing namespacing on the devicetree pro=
bing 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 worke=
d, 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 alr=
eady
> > > > 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 requ=
ire
> > > 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 a=
dd
> > > a standard extension.
> >
> > No, it is actually subtly different. Even if the kernel already support=
s
> > 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 a=
m
> > > extracting this out to a vendor file in the next version). This is al=
so
> > > a very easy task since the fields from a different vendor can be copi=
ed
> > > 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 wa=
nt to
> > > > see several different entries in structs (or several different hwpr=
obe
> > > > keys, but that's another story) for this situation because you're o=
nly
> > > > 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 the=
m
> > 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 vend=
or
> > 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 i=
f
> >   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 tha=
t
> > 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