Message ID | 20231017131456.2053396-2-cleger@rivosinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp4126747vqb; Tue, 17 Oct 2023 06:16:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHVwHBoPliHhmaKEzNRGoI4SK/F9+kw4cXVRe2sT+wLzkVhZvdMcKFZchmD9ofYOUOPjDU7 X-Received: by 2002:a05:6a20:4288:b0:171:737:df97 with SMTP id o8-20020a056a20428800b001710737df97mr2525357pzj.2.1697548593053; Tue, 17 Oct 2023 06:16:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697548593; cv=none; d=google.com; s=arc-20160816; b=dLy7T20zQUYwVit6Z4/76n2vqqEWkCWv075Hh1g7gdD5c7eFfXnrLr6dYYw8RSPhoI o4X4BKy/FMC+1B/1GG0GMrlL3czofka/BDjpgYP/3iv2sEGZxenLmswQ1MoPhmYgwcxE 37/0zbl4qvIFRQMNK7Qm/C6AyIbO2JXBioTRmPo5dTOOstuv4QOPBk/NjJQAnifDf6jf n8prkEb/quQ0eDnEwYQRbQSZMdYXl/KF2YTFyn/89p03e++ryOOct32MNCcksn9ibIJp kQank7Kk7p+Nd1Vn+vu6PJq6fhowmxJ3QShGbPJ8VkBslZQDcsnmvpgMQikP97gyyQtU mPVw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=gsST45qXCpXW6hXAZjZQAx82ZOViPLGcbU4pTeMM644=; fh=lvGFqwUxN+Dx1jonz8cRGrvjuKDcjMXylffmtrJGOYA=; b=d433CeDcAGy7DeugEPP/otHQ6hf3KFQ96jJ01qxXyrzIj0RGgXCwqBMk06dMmbqO5F e8LqoXXN03t0wd2KoT2REJ3bu26AXMA12PLzA900ygoJFLMzxMlEZtgtssYy/UJNFouQ X08hs0VhznlIE5tqj66OW2jYYygkR3Zf+eN6MAxwZIJxOlR0Uca+631cZu/e7hyStypM mkgjHZ63gHv/YhLjOOLj3Omm3AVbHWXdGA6s0fDHQR1k4lA3wbPNd65z7oYsO346KAPV 1YUFTQVCaEySGEFSxt7efrbJo9k9+ZJeEu9BdrO8gksvMFuHQ2K8g1wldlGlWEwvyzgl xcHg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=uyB2M5R+; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from lipwig.vger.email (lipwig.vger.email. [2620:137:e000::3:3]) by mx.google.com with ESMTPS id bn6-20020a056a02030600b005784ab1c4afsi1897198pgb.736.2023.10.17.06.16.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Oct 2023 06:16:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3; Authentication-Results: mx.google.com; dkim=pass header.i=@rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=uyB2M5R+; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 6530D8028A49; Tue, 17 Oct 2023 06:16:25 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343806AbjJQNPg (ORCPT <rfc822;hjfbswb@gmail.com> + 19 others); Tue, 17 Oct 2023 09:15:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57710 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234960AbjJQNPb (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 17 Oct 2023 09:15:31 -0400 Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CE8A2F7 for <linux-kernel@vger.kernel.org>; Tue, 17 Oct 2023 06:15:29 -0700 (PDT) Received: by mail-wm1-x334.google.com with SMTP id 5b1f17b1804b1-406aaccb41dso18299135e9.0 for <linux-kernel@vger.kernel.org>; Tue, 17 Oct 2023 06:15:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1697548528; x=1698153328; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=gsST45qXCpXW6hXAZjZQAx82ZOViPLGcbU4pTeMM644=; b=uyB2M5R+DJ/rHyT6Pmb69lo81ITr7UvFocveAU7syQsTuFPZxtnivmU/7djjdH/e6y jqgjPATS5uES8wvBOPABCuOv4f/R+zRAN3MdNnMzTncivzd5IZH+DqPJjLzjIdcoe7Ba /oa7xLRjmNZHwV6KbrMMXEJFfO8mj1Qrikef9zh4o1Zwe0Dxxm28SpQXViBtQNm3S+sW H11nLtitzYJ9AuozwHdAWW7oC9/9zQWJXRRIkqj0iR+GscKQSF/kVuux8aCcvsdpeZvW yjHjr54HHK8XFYFurSGbMdcwOtOOTcWJly8QKti6dxieiw8Oo+GiIzxiAww27Rtnjbua EthA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697548528; x=1698153328; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=gsST45qXCpXW6hXAZjZQAx82ZOViPLGcbU4pTeMM644=; b=GxXoJcMAWUSXWYkJP7ZoB+rbF4bJ/vCs5Ljd8Y40Ol3VpY2LRJnKD5g+8rLH+9wBjd fODJsUMJAysddRHt13MChC0M70VkkVtPP06ey/hIQpKe6Kl0TJFRgjnBSL50T+SXzjAP EYFgSRg8vfz7N2gW95LXThI2v9Uf275H9wVYAvUVf1z5YHYsHONEYnU5+FWYweMjpZWN s2upJGIY2G1OODiDLbWE3v/w6xijXtmxrUOY311r0HRnALFJkaDqrQaA8lj1fUDgphEZ W7ZCgfptxOO23hpUBrlNBfT8odhMVmgq9nl4EQ6tHYJGGrIZ4RWEhNuqe/v09i3e/huB +UKQ== X-Gm-Message-State: AOJu0Yzd2bbzARXZh9bIcSeNnMplq59b3XFybP5UUHP4m2oyuW3R314D jeDu4dKbU+tMceJMnM6+h3pRcko6gj4bVH3JGUjDuA== X-Received: by 2002:a05:600c:4f88:b0:405:3cc1:e115 with SMTP id n8-20020a05600c4f8800b004053cc1e115mr1603989wmq.3.1697548527550; Tue, 17 Oct 2023 06:15:27 -0700 (PDT) Received: from carbon-x1.. ([2a01:e0a:999:a3a0:96:820c:ecf7:a817]) by smtp.gmail.com with ESMTPSA id fj7-20020a05600c0c8700b0040772138bb7sm9873393wmb.2.2023.10.17.06.15.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Oct 2023 06:15:26 -0700 (PDT) From: =?utf-8?b?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com> To: linux-riscv@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org Cc: =?utf-8?b?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>, Palmer Dabbelt <palmer@rivosinc.com>, Paul Walmsley <paul.walmsley@sifive.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Albert Ou <aou@eecs.berkeley.edu>, Jonathan Corbet <corbet@lwn.net>, Andrew Jones <ajones@ventanamicro.com>, Evan Green <evan@rivosinc.com>, Conor Dooley <conor@kernel.org>, Samuel Ortiz <sameo@rivosinc.com> Subject: [PATCH v2 01/19] riscv: hwprobe: factorize hwprobe ISA extension reporting Date: Tue, 17 Oct 2023 15:14:38 +0200 Message-ID: <20231017131456.2053396-2-cleger@rivosinc.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20231017131456.2053396-1-cleger@rivosinc.com> References: <20231017131456.2053396-1-cleger@rivosinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Tue, 17 Oct 2023 06:16:25 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780008713196210656 X-GMAIL-MSGID: 1780008713196210656 |
Series |
riscv: report more ISA extensions through hwprobe
|
|
Commit Message
Clément Léger
Oct. 17, 2023, 1:14 p.m. UTC
Factorize ISA extension reporting by using a macro rather than
copy/pasting extension names. This will allow adding new extensions more
easily.
Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
Comments
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > Factorize ISA extension reporting by using a macro rather than > copy/pasting extension names. This will allow adding new extensions more > easily. > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > index 473159b5f303..e207874e686e 100644 > --- a/arch/riscv/kernel/sys_riscv.c > +++ b/arch/riscv/kernel/sys_riscv.c > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > for_each_cpu(cpu, cpus) { > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > - else > - missing |= RISCV_HWPROBE_EXT_ZBA; > - > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > - else > - missing |= RISCV_HWPROBE_EXT_ZBB; > - > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > - else > - missing |= RISCV_HWPROBE_EXT_ZBS; > +#define CHECK_ISA_EXT(__ext) \ > + do { \ > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > + else \ > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > + } while (false) > + > + /* > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > + * to userspace, regardless of the kernel's configuration, as no > + * other checks, besides presence in the hart_isa bitmap, are > + * made. This comment alludes to a dangerous trap, but I'm having trouble understanding what it is. Perhaps some rewording to more explicitly state the danger would be appropriate. Other than that: Reviewed-by: Evan Green <evan@rivosinc.com>
On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > Factorize ISA extension reporting by using a macro rather than > > copy/pasting extension names. This will allow adding new extensions more > > easily. > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > --- > > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > index 473159b5f303..e207874e686e 100644 > > --- a/arch/riscv/kernel/sys_riscv.c > > +++ b/arch/riscv/kernel/sys_riscv.c > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > > for_each_cpu(cpu, cpus) { > > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > > - else > > - missing |= RISCV_HWPROBE_EXT_ZBA; > > - > > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > > - else > > - missing |= RISCV_HWPROBE_EXT_ZBB; > > - > > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > > - else > > - missing |= RISCV_HWPROBE_EXT_ZBS; > > +#define CHECK_ISA_EXT(__ext) \ > > + do { \ > > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > > + else \ > > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > > + } while (false) > > + > > + /* > > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > > + * to userspace, regardless of the kernel's configuration, as no > > + * other checks, besides presence in the hart_isa bitmap, are > > + * made. > > This comment alludes to a dangerous trap, but I'm having trouble > understanding what it is. You cannot, for example, use this for communicating the presence of F or D, since they require a config option to be set before their use is safe. > Perhaps some rewording to more explicitly > state the danger would be appropriate. Other than that: > > Reviewed-by: Evan Green <evan@rivosinc.com>
On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: > On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > > > Factorize ISA extension reporting by using a macro rather than > > > copy/pasting extension names. This will allow adding new extensions more > > > easily. > > > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > > --- > > > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > > index 473159b5f303..e207874e686e 100644 > > > --- a/arch/riscv/kernel/sys_riscv.c > > > +++ b/arch/riscv/kernel/sys_riscv.c > > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > > > for_each_cpu(cpu, cpus) { > > > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > > > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > > > - else > > > - missing |= RISCV_HWPROBE_EXT_ZBA; > > > - > > > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > > > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > > > - else > > > - missing |= RISCV_HWPROBE_EXT_ZBB; > > > - > > > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > > > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > > > - else > > > - missing |= RISCV_HWPROBE_EXT_ZBS; > > > +#define CHECK_ISA_EXT(__ext) \ > > > + do { \ > > > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > > > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > > > + else \ > > > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > > > + } while (false) > > > + > > > + /* > > > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > > > + * to userspace, regardless of the kernel's configuration, as no > > > + * other checks, besides presence in the hart_isa bitmap, are > > > + * made. > > > > This comment alludes to a dangerous trap, but I'm having trouble > > understanding what it is. > > You cannot, for example, use this for communicating the presence of F or > D, since they require a config option to be set before their use is > safe. Funnily enough, this comment is immediately contradicted by the vector subset extensions, where these CHECK_ISA_EXT() macros are used wrapped in has_vector(). The code looks valid to me, since has_vector() contains the Kconfig check, but does fly in the face of this comment. > > > Perhaps some rewording to more explicitly > > state the danger would be appropriate. Other than that: > > > > Reviewed-by: Evan Green <evan@rivosinc.com>
On Wed, Oct 18, 2023 at 10:37 AM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: > > On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > > > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > > > > > Factorize ISA extension reporting by using a macro rather than > > > > copy/pasting extension names. This will allow adding new extensions more > > > > easily. > > > > > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > > > --- > > > > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > > > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > > > index 473159b5f303..e207874e686e 100644 > > > > --- a/arch/riscv/kernel/sys_riscv.c > > > > +++ b/arch/riscv/kernel/sys_riscv.c > > > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > > > > for_each_cpu(cpu, cpus) { > > > > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > > > > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > > > > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > > > > - else > > > > - missing |= RISCV_HWPROBE_EXT_ZBA; > > > > - > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > > > > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > > > > - else > > > > - missing |= RISCV_HWPROBE_EXT_ZBB; > > > > - > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > > > > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > > > > - else > > > > - missing |= RISCV_HWPROBE_EXT_ZBS; > > > > +#define CHECK_ISA_EXT(__ext) \ > > > > + do { \ > > > > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > > > > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > > > > + else \ > > > > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > > > > + } while (false) > > > > + > > > > + /* > > > > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > > > > + * to userspace, regardless of the kernel's configuration, as no > > > > + * other checks, besides presence in the hart_isa bitmap, are > > > > + * made. > > > > > > This comment alludes to a dangerous trap, but I'm having trouble > > > understanding what it is. > > > > You cannot, for example, use this for communicating the presence of F or > > D, since they require a config option to be set before their use is > > safe. > > Funnily enough, this comment is immediately contradicted by the vector > subset extensions, where these CHECK_ISA_EXT() macros are used wrapped > in has_vector(). The code looks valid to me, since has_vector() contains > the Kconfig check, but does fly in the face of this comment. Ohh, got it. The word "can" is doing a lot of heavy lifting in that comment. So maybe something like: "This macro performs little in the way of extension-specific kernel readiness checks. It's assumed other gating factors like required Kconfig settings have already been confirmed to support exposing the given extension to usermode". ... But, you know, make it sparkle. -Evan
On 18/10/2023 19:36, Conor Dooley wrote: > On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: >> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: >>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: >>>> >>>> Factorize ISA extension reporting by using a macro rather than >>>> copy/pasting extension names. This will allow adding new extensions more >>>> easily. >>>> >>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>>> --- >>>> arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- >>>> 1 file changed, 18 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >>>> index 473159b5f303..e207874e686e 100644 >>>> --- a/arch/riscv/kernel/sys_riscv.c >>>> +++ b/arch/riscv/kernel/sys_riscv.c >>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, >>>> for_each_cpu(cpu, cpus) { >>>> struct riscv_isainfo *isainfo = &hart_isa[cpu]; >>>> >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBA)) >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBA; >>>> - else >>>> - missing |= RISCV_HWPROBE_EXT_ZBA; >>>> - >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBB)) >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBB; >>>> - else >>>> - missing |= RISCV_HWPROBE_EXT_ZBB; >>>> - >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBS)) >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBS; >>>> - else >>>> - missing |= RISCV_HWPROBE_EXT_ZBS; >>>> +#define CHECK_ISA_EXT(__ext) \ >>>> + do { \ >>>> + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ >>>> + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ >>>> + else \ >>>> + missing |= RISCV_HWPROBE_EXT_##__ext; \ >>>> + } while (false) >>>> + >>>> + /* >>>> + * Only use CHECK_ISA_EXT() for extensions which can be exposed >>>> + * to userspace, regardless of the kernel's configuration, as no >>>> + * other checks, besides presence in the hart_isa bitmap, are >>>> + * made. >>> >>> This comment alludes to a dangerous trap, but I'm having trouble >>> understanding what it is. >> >> You cannot, for example, use this for communicating the presence of F or >> D, since they require a config option to be set before their use is >> safe. > > Funnily enough, this comment is immediately contradicted by the vector > subset extensions, where these CHECK_ISA_EXT() macros are used wrapped > in has_vector(). The code looks valid to me, since has_vector() contains > the Kconfig check, but does fly in the face of this comment. Yes, the KConfig checks are already done by the headers, adding #ifdef would be redundant even if more coherent with the comment. BTW, wouldn't it make more sense to get rid out of the unsupported extensions directly at ISA string parsing ? ie, if kernel is compiled without V support, then do not set the bits corresponding to these in the riscv_isa_ext[] array ? But the initial intent was probably to be able to report the full string through cpuinfo. Clément > >> >>> Perhaps some rewording to more explicitly >>> state the danger would be appropriate. Other than that: >>> >>> Reviewed-by: Evan Green <evan@rivosinc.com> > >
On 18/10/2023 19:45, Evan Green wrote: > On Wed, Oct 18, 2023 at 10:37 AM Conor Dooley <conor@kernel.org> wrote: >> >> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: >>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: >>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: >>>>> >>>>> Factorize ISA extension reporting by using a macro rather than >>>>> copy/pasting extension names. This will allow adding new extensions more >>>>> easily. >>>>> >>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>>>> --- >>>>> arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- >>>>> 1 file changed, 18 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >>>>> index 473159b5f303..e207874e686e 100644 >>>>> --- a/arch/riscv/kernel/sys_riscv.c >>>>> +++ b/arch/riscv/kernel/sys_riscv.c >>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, >>>>> for_each_cpu(cpu, cpus) { >>>>> struct riscv_isainfo *isainfo = &hart_isa[cpu]; >>>>> >>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBA)) >>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBA; >>>>> - else >>>>> - missing |= RISCV_HWPROBE_EXT_ZBA; >>>>> - >>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBB)) >>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBB; >>>>> - else >>>>> - missing |= RISCV_HWPROBE_EXT_ZBB; >>>>> - >>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBS)) >>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBS; >>>>> - else >>>>> - missing |= RISCV_HWPROBE_EXT_ZBS; >>>>> +#define CHECK_ISA_EXT(__ext) \ >>>>> + do { \ >>>>> + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ >>>>> + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ >>>>> + else \ >>>>> + missing |= RISCV_HWPROBE_EXT_##__ext; \ >>>>> + } while (false) >>>>> + >>>>> + /* >>>>> + * Only use CHECK_ISA_EXT() for extensions which can be exposed >>>>> + * to userspace, regardless of the kernel's configuration, as no >>>>> + * other checks, besides presence in the hart_isa bitmap, are >>>>> + * made. >>>> >>>> This comment alludes to a dangerous trap, but I'm having trouble >>>> understanding what it is. >>> >>> You cannot, for example, use this for communicating the presence of F or >>> D, since they require a config option to be set before their use is >>> safe. >> >> Funnily enough, this comment is immediately contradicted by the vector >> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped >> in has_vector(). The code looks valid to me, since has_vector() contains >> the Kconfig check, but does fly in the face of this comment. > > > Ohh, got it. The word "can" is doing a lot of heavy lifting in that > comment. So maybe something like: "This macro performs little in the > way of extension-specific kernel readiness checks. It's assumed other > gating factors like required Kconfig settings have already been > confirmed to support exposing the given extension to usermode". ... > But, you know, make it sparkle. Hi Even, Indeed the comment was a bit misleading, is this more clear ? /* * Only use CHECK_ISA_EXT() for extensions which are usable by * userspace with respect to the kernel current configuration. * For instance, ISA extensions that uses float operations * should not be exposed when CONFIG_FPU is not set. */ Clément > > -Evan
On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote: > > > On 18/10/2023 19:36, Conor Dooley wrote: > > On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: > >> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > >>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > >>>> > >>>> Factorize ISA extension reporting by using a macro rather than > >>>> copy/pasting extension names. This will allow adding new extensions more > >>>> easily. > >>>> > >>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> > >>>> --- > >>>> arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > >>>> 1 file changed, 18 insertions(+), 14 deletions(-) > >>>> > >>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > >>>> index 473159b5f303..e207874e686e 100644 > >>>> --- a/arch/riscv/kernel/sys_riscv.c > >>>> +++ b/arch/riscv/kernel/sys_riscv.c > >>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > >>>> for_each_cpu(cpu, cpus) { > >>>> struct riscv_isainfo *isainfo = &hart_isa[cpu]; > >>>> > >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBA; > >>>> - else > >>>> - missing |= RISCV_HWPROBE_EXT_ZBA; > >>>> - > >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBB; > >>>> - else > >>>> - missing |= RISCV_HWPROBE_EXT_ZBB; > >>>> - > >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBS; > >>>> - else > >>>> - missing |= RISCV_HWPROBE_EXT_ZBS; > >>>> +#define CHECK_ISA_EXT(__ext) \ > >>>> + do { \ > >>>> + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > >>>> + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > >>>> + else \ > >>>> + missing |= RISCV_HWPROBE_EXT_##__ext; \ > >>>> + } while (false) > >>>> + > >>>> + /* > >>>> + * Only use CHECK_ISA_EXT() for extensions which can be exposed > >>>> + * to userspace, regardless of the kernel's configuration, as no > >>>> + * other checks, besides presence in the hart_isa bitmap, are > >>>> + * made. > >>> > >>> This comment alludes to a dangerous trap, but I'm having trouble > >>> understanding what it is. > >> > >> You cannot, for example, use this for communicating the presence of F or > >> D, since they require a config option to be set before their use is > >> safe. > > > > Funnily enough, this comment is immediately contradicted by the vector > > subset extensions, where these CHECK_ISA_EXT() macros are used wrapped > > in has_vector(). The code looks valid to me, since has_vector() contains > > the Kconfig check, but does fly in the face of this comment. > Yes, the KConfig checks are already done by the headers, adding #ifdef > would be redundant even if more coherent with the comment I don't really understand what the first part of this means, or why using avoidable ifdeffery here would be desirable. > BTW, wouldn't > it make more sense to get rid out of the unsupported extensions directly > at ISA string parsing ? ie, if kernel is compiled without V support, > then do not set the bits corresponding to these in the riscv_isa_ext[] > array ? But the initial intent was probably to be able to report the > full string through cpuinfo. Yeah, hysterical raisins I guess, it's always been that way. I don't think anyone originally thought about such configurations and that is how the cpuinfo stuff behaves. I strongly dislike the riscv_isa_extension_available() interface, but one of Drew's patches does at least improve things a bit. Kinda waiting for some of the patches in flight to settle down before deciding if I want to refactor stuff to be less of a potential for shooting oneself in the foot.
On Thu, Oct 19, 2023 at 11:46:51AM +0200, Clément Léger wrote: > Indeed the comment was a bit misleading, is this more clear ? > > /* > * Only use CHECK_ISA_EXT() for extensions which are usable by > * userspace with respect to the kernel current configuration. > * For instance, ISA extensions that uses float operations s/that uses/that use/ > * should not be exposed when CONFIG_FPU is not set. s/is not set/is not enabled/ But yeah, definitely more clear, thanks.
On 19/10/2023 12:22, Conor Dooley wrote: > On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote: >> >> >> On 18/10/2023 19:36, Conor Dooley wrote: >>> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: >>>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: >>>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: >>>>>> >>>>>> Factorize ISA extension reporting by using a macro rather than >>>>>> copy/pasting extension names. This will allow adding new extensions more >>>>>> easily. >>>>>> >>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>>>>> --- >>>>>> arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- >>>>>> 1 file changed, 18 insertions(+), 14 deletions(-) >>>>>> >>>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >>>>>> index 473159b5f303..e207874e686e 100644 >>>>>> --- a/arch/riscv/kernel/sys_riscv.c >>>>>> +++ b/arch/riscv/kernel/sys_riscv.c >>>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, >>>>>> for_each_cpu(cpu, cpus) { >>>>>> struct riscv_isainfo *isainfo = &hart_isa[cpu]; >>>>>> >>>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBA)) >>>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBA; >>>>>> - else >>>>>> - missing |= RISCV_HWPROBE_EXT_ZBA; >>>>>> - >>>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBB)) >>>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBB; >>>>>> - else >>>>>> - missing |= RISCV_HWPROBE_EXT_ZBB; >>>>>> - >>>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBS)) >>>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBS; >>>>>> - else >>>>>> - missing |= RISCV_HWPROBE_EXT_ZBS; >>>>>> +#define CHECK_ISA_EXT(__ext) \ >>>>>> + do { \ >>>>>> + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ >>>>>> + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ >>>>>> + else \ >>>>>> + missing |= RISCV_HWPROBE_EXT_##__ext; \ >>>>>> + } while (false) >>>>>> + >>>>>> + /* >>>>>> + * Only use CHECK_ISA_EXT() for extensions which can be exposed >>>>>> + * to userspace, regardless of the kernel's configuration, as no >>>>>> + * other checks, besides presence in the hart_isa bitmap, are >>>>>> + * made. >>>>> >>>>> This comment alludes to a dangerous trap, but I'm having trouble >>>>> understanding what it is. >>>> >>>> You cannot, for example, use this for communicating the presence of F or >>>> D, since they require a config option to be set before their use is >>>> safe. >>> >>> Funnily enough, this comment is immediately contradicted by the vector >>> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped >>> in has_vector(). The code looks valid to me, since has_vector() contains >>> the Kconfig check, but does fly in the face of this comment. > >> Yes, the KConfig checks are already done by the headers, adding #ifdef >> would be redundant even if more coherent with the comment > > I don't really understand what the first part of this means, or why using > avoidable ifdeffery here would be desirable. Sorry, I was not clear enough. What I meant is that the has_fpu() and has_vector() functions are already ifdef'd in headers based on the KConfig options for their support (CONFIG_FPU/CONFIG_RISCV_ISA_V) So in the end, using ifdef here in hwprobe_isa_ext0() would be redundant. > >> BTW, wouldn't >> it make more sense to get rid out of the unsupported extensions directly >> at ISA string parsing ? ie, if kernel is compiled without V support, >> then do not set the bits corresponding to these in the riscv_isa_ext[] >> array ? But the initial intent was probably to be able to report the >> full string through cpuinfo. > > Yeah, hysterical raisins I guess, it's always been that way. I don't > think anyone originally thought about such configurations and that is > how the cpuinfo stuff behaves. I strongly dislike the > riscv_isa_extension_available() interface, but one of Drew's patches > does at least improve things a bit. Kinda waiting for some of the > patches in flight to settle down before deciding if I want to refactor > stuff to be less of a potential for shooting oneself in the foot. Make sense. Clément
On Thu, Oct 19, 2023 at 3:24 AM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Oct 19, 2023 at 11:46:51AM +0200, Clément Léger wrote: > > Indeed the comment was a bit misleading, is this more clear ? > > > > /* > > * Only use CHECK_ISA_EXT() for extensions which are usable by > > * userspace with respect to the kernel current configuration. > > * For instance, ISA extensions that uses float operations > > s/that uses/that use/ > > > * should not be exposed when CONFIG_FPU is not set. > > s/is not set/is not enabled/ > > But yeah, definitely more clear, thanks. Looks good to me too. Thanks, Clément! -Evan
On Thu, Oct 19, 2023 at 11:22:26AM +0100, Conor Dooley wrote: > On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote: ... > > BTW, wouldn't > > it make more sense to get rid out of the unsupported extensions directly > > at ISA string parsing ? ie, if kernel is compiled without V support, > > then do not set the bits corresponding to these in the riscv_isa_ext[] > > array ? But the initial intent was probably to be able to report the > > full string through cpuinfo. > > Yeah, hysterical raisins I guess, it's always been that way. I don't > think anyone originally thought about such configurations and that is > how the cpuinfo stuff behaves. I strongly dislike the > riscv_isa_extension_available() interface, but one of Drew's patches > does at least improve things a bit. Kinda waiting for some of the > patches in flight to settle down before deciding if I want to refactor > stuff to be less of a potential for shooting oneself in the foot. And I recall promising to try and do something with it too, but that promise got buried under other promises... It's still on the TODO, at least! drew
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c index 473159b5f303..e207874e686e 100644 --- a/arch/riscv/kernel/sys_riscv.c +++ b/arch/riscv/kernel/sys_riscv.c @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, for_each_cpu(cpu, cpus) { struct riscv_isainfo *isainfo = &hart_isa[cpu]; - if (riscv_isa_extension_available(isainfo->isa, ZBA)) - pair->value |= RISCV_HWPROBE_EXT_ZBA; - else - missing |= RISCV_HWPROBE_EXT_ZBA; - - if (riscv_isa_extension_available(isainfo->isa, ZBB)) - pair->value |= RISCV_HWPROBE_EXT_ZBB; - else - missing |= RISCV_HWPROBE_EXT_ZBB; - - if (riscv_isa_extension_available(isainfo->isa, ZBS)) - pair->value |= RISCV_HWPROBE_EXT_ZBS; - else - missing |= RISCV_HWPROBE_EXT_ZBS; +#define CHECK_ISA_EXT(__ext) \ + do { \ + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ + else \ + missing |= RISCV_HWPROBE_EXT_##__ext; \ + } while (false) + + /* + * Only use CHECK_ISA_EXT() for extensions which can be exposed + * to userspace, regardless of the kernel's configuration, as no + * other checks, besides presence in the hart_isa bitmap, are + * made. + */ + CHECK_ISA_EXT(ZBA); + CHECK_ISA_EXT(ZBB); + CHECK_ISA_EXT(ZBS); +#undef CHECK_ISA_EXT } /* Now turn off reporting features if any CPU is missing it. */