Message-ID: <tencent_6A95637042401AD5F8BE05C7B4F11CAD7009@qq.com>
Date: Thu, 14 Nov 2024 11:26:47 +0800
From: Yangyu Chen <cyy@...self.name>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Jisheng Zhang <jszhang@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
 Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Samuel Holland <samuel.holland@...ive.com>, Jonathan Corbet
 <corbet@....net>, Shuah Khan <shuah@...nel.org>, Guo Ren
 <guoren@...nel.org>, Evan Green <evan@...osinc.com>,
 Jessica Clarke <jrtc27@...c27.com>, Andrew Jones <ajones@...tanamicro.com>,
 Andy Chiu <andybnac@...il.com>, linux-riscv@...ts.infradead.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-sunxi@...ts.linux.dev, linux-doc@...r.kernel.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v11 10/14] riscv: hwprobe: Add thead vendor extension
 probing



On 11/14/24 11:02, Charlie Jenkins wrote:
> On Thu, Nov 14, 2024 at 10:44:37AM +0800, Yangyu Chen wrote:
>>
>>
>> On 11/14/24 10:21, Charlie Jenkins wrote:
>>> Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0" which
>>> allows userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR
>>> vendor extension.
>>>
>>
>> Hi Charlie,
>>
>> How about changing the name of the key from
>> "RISCV_ISA_VENDOR_EXT_XTHEADVECTOR" to "RISCV_HWPROBE_KEY_VENDOR_EXT_0" and
>> use marchid to identify what the vendor is, each vendor will have its own
>> bit definition in this value. So we can avoid adding so many hwprobe keys
>> for each vendor in the future.
>>
>> I proposed a commit here: https://github.com/cyyself/linux/commit/36390645d85d1ac75dd71172f167719df4297f59
> 
> I actually originally had this in one of my first versions of this
> series but was convinced by Conor to change it. The problem with it was
> that tying vendor extensions to mvendorid means that it is enforced by
> the kernel that vendors cannot share vendor extensions. It is possible
> for vendor A to purchase IP that contains a vendor extension from vendor
> B. This vendor extension should work on platforms created by vendor A
> and vendor B. However, vendor A and vendor B have different mvendorids,
> so the kernel can't support this if it is tied to mvendorid.  It could
> be solved by duplicating every extension that vendors have, but then
> userspace software would have to keep in mind the mvendorid they are
> running on and check the different extensions for the different vendors
> even though the implementation of the extension is the same.
> 
> The original conversation where Conor and I agreed that it was better to
> have vendor extensions not rely on mvendorid:
> 
> https://lore.kernel.org/linux-riscv/20240416-husband-flavored-96c1dad58b6e@wendy/
> 

Thanks for your explanation. I will strongly agree with Conor's opinion 
if the feature bitmask does not exist in RISC-V C-ABI.

However, as the feature mask defined in RISC-V C-ABI[1] uses the design 
depending on marchid currently, should we reconsider this key for its 
use case? The current target_clones and taget_version implemented in 
GCC[2] and LLVM[3] also use the bitmask defined in C-ABI. I think if we 
use this key depending on marchid, to make a key shared with all vendors 
will make this cleaner.

[1] 
https://github.com/riscv-non-isa/riscv-c-api-doc/blob/main/src/c-api.adoc#function-multi-version
[2] 
https://github.com/gcc-mirror/gcc/blob/8564d0948c72df0a66d7eb47e15c6ab43e9b25ce/gcc/config/riscv/riscv.cc#L13016
[3] 
https://github.com/llvm/llvm-project/blob/f407dff50cdcbcfee9dd92397d3792627c3ac708/clang/lib/CodeGen/CGBuiltin.cpp#L14627

>>
>>> This new key will allow userspace code to probe for which thead vendor
>>> extensions are supported. This API is modeled to be consistent with
>>> RISCV_HWPROBE_KEY_IMA_EXT_0. The bitmask returned will have each bit
>>> corresponding to a supported thead vendor extension of the cpumask set.
>>> Just like RISCV_HWPROBE_KEY_IMA_EXT_0, this allows a userspace program
>>> to determine all of the supported thead vendor extensions in one call.
>>>
>>> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
>>> Reviewed-by: Evan Green <evan@...osinc.com>
>>> ---
>>>    arch/riscv/include/asm/hwprobe.h                   |  3 +-
>>>    .../include/asm/vendor_extensions/thead_hwprobe.h  | 19 +++++++++++
>>>    .../include/asm/vendor_extensions/vendor_hwprobe.h | 37 ++++++++++++++++++++++
>>>    arch/riscv/include/uapi/asm/hwprobe.h              |  3 +-
>>>    arch/riscv/include/uapi/asm/vendor/thead.h         |  3 ++
>>>    arch/riscv/kernel/sys_hwprobe.c                    |  5 +++
>>>    arch/riscv/kernel/vendor_extensions/Makefile       |  1 +
>>>    .../riscv/kernel/vendor_extensions/thead_hwprobe.c | 19 +++++++++++
>>>    8 files changed, 88 insertions(+), 2 deletions(-)
>>>
>>

Powered by blists - more mailing lists