From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B0199EB64D9 for ; Thu, 6 Jul 2023 20:01:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=F6O6hgpdHe8h+5cneeWyJDrGTnVSnMw0fefjjuIue6M=; b=g67I/ttDpwi2BT Qu6cFmOkphK9yZ1v12XYzO0NslKG6GV/nFURIKTR4J/P1b3eV7zDQw3CIZjsBrb+70fooGbEPjsiN 8ahj8Muz4L9zjDFnqp3WTRr+8/PTlQYZVle4RRf3FVL07aJ89lCR7/Y2965teoN0bK0GMTDL9N+2z 7P1g7QdkZamRlRgl4t2/GxCKbfgjpSk3TKieC7SUbnBW+JXXCH8X0PaxZWdBFcGL2JFBUtGNqJM21 k8oTnXkWbFQklkd9yfn0HMTskes/JLRn2Or0itmXmDhjY6TCuMtGlAzlR+dURWptpAkV5+RJIfKUu tJnlkb6Kffy3hLNX579Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qHV9w-002Z0Q-2w; Thu, 06 Jul 2023 20:01:00 +0000 Received: from mail-pl1-x629.google.com ([2607:f8b0:4864:20::629]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qHV9t-002YzA-0i for linux-riscv@lists.infradead.org; Thu, 06 Jul 2023 20:00:59 +0000 Received: by mail-pl1-x629.google.com with SMTP id d9443c01a7336-1b89114266dso7222595ad.0 for ; Thu, 06 Jul 2023 13:00:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20221208.gappssmtp.com; s=20221208; t=1688673655; x=1691265655; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=h1WU2V04iVZxWiGrBNfGBOzGfOXPgmYyppOpGBIsnMQ=; b=dFyhEMLkcRRKiKGrmVpevNYL5dlYe9YRhDfDkuthC4m5TyQ0XEDeymG2969D+y3roL MIVibHl5zKvPxc3cQ678knVvqNlFg/Edm/WBczLrvl7YgWQgcbG3n7ORhxLj+PId16Rh OlpC2r9Fql6CzqCSSz/qfCGaiATuI20SfcyR+SMXN/xKvlkZJ3NvTpx1iRrgU/KuTzW/ oGz6uD8aGXK1ilcqpCZm6bGedVBGk95iMiadi5FU9C3fzk3YfE+f2V2dQKPUQZ8Kn5eR 3WFsX68WjsA2KIgAewiju2ksHaDMC1SabFRmfkj11YcSitbIwwx2QhhzYnnaWJkCvynT 3MGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688673655; x=1691265655; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=h1WU2V04iVZxWiGrBNfGBOzGfOXPgmYyppOpGBIsnMQ=; b=c+yFhvI6BvmYkGeMrOk1k2b/bm3JfVDTe4B6tFjt4yDjUfweAIPD9HFH/rr/ab75A4 GhVOgybSLm48m6hR0N9oZwlC8813NLerWXJZnbFfGz3HfeFkZDrmwPJUpTXgseDuBevY WXZtaJlp0G15T+OCP4Tks6zq6Pz+nI8XLZgk7RVolslvzunvVcvkdahpwW0GKmhnKVyA IS/USLCntwInPYY/aD2xRfQnBP36qd5JZpmWd0/ge816FNi5VBbPXvWi6SXEM3u0pXRc 7/klgTzSBTiPQrYkqC/CAR4yxYsdEWAg6v5b+UXfncEs0LkM/o7L3/8gDh98pD8X2cO0 XYBw== X-Gm-Message-State: ABy/qLZYl28Jy9Q6j8EqYqJ1lGaEoYlYTQM2SJYW45qIJ8S7TIJBRjRd alp2MfQCuN1ZpIi30yZDnuAXkw== X-Google-Smtp-Source: APBJJlGCTJOroNz4RXrgThXrgYzG6Oh5sXI8M7dMJKnHdVqVCbocuXvVz+ICxCCiBbnj5ixDClNE1Q== X-Received: by 2002:a17:903:32c9:b0:1b8:9598:6508 with SMTP id i9-20020a17090332c900b001b895986508mr3353078plr.18.1688673655421; Thu, 06 Jul 2023 13:00:55 -0700 (PDT) Received: from ghost ([50.221.140.188]) by smtp.gmail.com with ESMTPSA id x1-20020a170902b40100b001b8013ed362sm1790618plr.96.2023.07.06.13.00.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jul 2023 13:00:55 -0700 (PDT) Date: Thu, 6 Jul 2023 13:00:52 -0700 From: Charlie Jenkins To: Conor Dooley Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Paul Walmsley , Palmer Dabbelt , Albert Ou , Jonathan Corbet , evan@rivosinc.com, heiko@sntech.de, linux-doc@vger.kernel.org Subject: Re: [PATCH 2/3] RISC-V: Add T-Head 0.7.1 vector extension to hwprobe Message-ID: References: <20230705-thead_vendor_extensions-v1-0-ad6915349c4d@rivosinc.com> <20230705-thead_vendor_extensions-v1-2-ad6915349c4d@rivosinc.com> <20230706-unwed-aeration-d8c7b71eac5e@spud> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230706-unwed-aeration-d8c7b71eac5e@spud> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230706_130057_265517_77F039D0 X-CRM114-Status: GOOD ( 64.45 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Thu, Jul 06, 2023 at 06:38:17PM +0100, Conor Dooley wrote: > Hey, > > On Wed, Jul 05, 2023 at 08:30:18PM -0700, Charlie Jenkins wrote: > > Using vendor extensions in hwprobe, add the ability to query if the > > 0.7.1 vector extension is available. It is determined to be available > > only if the kernel is compiled with vector support, > > > and the user is > > using the c906. > > Heh, unfortunately your patch doesn't apply this limitation. > > I'm not really sure where this series is intended to sit in relation to > Heiko's series that adds support for the actual T-Head vector stuff: > https://lore.kernel.org/linux-riscv/20230622231305.631331-1-heiko@sntech.de/ > > Is this intended to complement that? If so, these patches don't really > seem to integrate with it (and have some of the same flaws unfortunately > as that series does). > > Firstly, to my knowledge, all T-Head cpu cores report 0 for impid & > archid. Stefan pointed out: > C906 supports t-head/0.7.1 vectors as a configuration option. The C906 in > the D1 and BL808 has vectors, the recently announced CV1800B has one C906 > with vectors and one without, and I vaguely remember seeing a chip with only > a non-vector C906. > > C908 (announced, no manual yet) claims V 1.0 support. Presumably it will > not support 0.7.1. > > C910 (exists on evaluation boards) lacks vector support. > > C920 (TH1520, SG2042, etc) has 0.7.1 support, at least superficially > compatible with C906-with-vectors. Hopefully we can share errata. > > This probably needs to be handled as an orthogonal "xtheadv" or "v0p7p1" > extension in whatever replaces riscv,isa. > > This makes an approach that does anything w/ their vector implementation > not discernible based on the m*id CSRs. IMO, the only way to make this > stuff work properly is to detect based on a DT or ACPI property whether > this stuff is supported on a given core. > > Since the approach just cannot work, I don't have any detailed comments > to make, just a few small ones below. > It would be beneficial to use Heiko's vector support patch. I can look into using that. The main purpose of this patch is to propose a method of vendor extension support and since T-Head has hardware I have used their hardware as an example of how to implement vendor extensions. That is the reason for the kind of awkward patch segmentation. > > Signed-off-by: Charlie Jenkins > > --- > > arch/riscv/Kconfig.vendor | 11 +++++++++++ > > arch/riscv/include/asm/extensions.h | 16 ++++++++++++++++ > > arch/riscv/kernel/sys_riscv.c | 20 ++++++++++++++++++++ > > arch/riscv/vendor_extensions/Makefile | 2 ++ > > arch/riscv/vendor_extensions/thead/Makefile | 8 ++++++++ > > arch/riscv/vendor_extensions/thead/extensions.c | 24 ++++++++++++++++++++++++ > > 6 files changed, 81 insertions(+) > > > > diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor > > index 213ac3e6fed5..b8b9d15153eb 100644 > > --- a/arch/riscv/Kconfig.vendor > > +++ b/arch/riscv/Kconfig.vendor > > @@ -1,3 +1,14 @@ > > menu "Vendor extensions selection" > > > > +config VENDOR_EXTENSIONS_THEAD > > + bool "T-HEAD vendor extensions" > > > + depends on RISCV_ALTERNATIVE > > Why do you need this? > Thanks for pointing that out, I meant to remove that. > > + default n > > + help > > + All T-HEAD vendor extensions Kconfig depend on this Kconfig. Disabling > > + this Kconfig will disable all T-HEAD vendor extensions. Please say "Y" > > + here if your platform uses T-HEAD vendor extensions. > > I don't really like this Kconfig entry. We should just use the one in > Kconfig.errata that enables the actual vector stuff. > The purpose of this is to support more than just the T-Head vector extension. The vector extension is just the vendor extension I selected to support first. The purpose of this file is for all vendors to have their own Kconfig entries to enable the vector extension which I didn't feel that it properly fit into the errata. > > + > > + Otherwise, please say "N" here to avoid unnecessary overhead. > > + > > endmenu # "Vendor extensions selection" > > diff --git a/arch/riscv/include/asm/extensions.h b/arch/riscv/include/asm/extensions.h > > new file mode 100644 > > index 000000000000..27ce294a3d65 > > --- /dev/null > > +++ b/arch/riscv/include/asm/extensions.h > > @@ -0,0 +1,16 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (C) 2023 by Rivos Inc. > > + */ > > +#ifndef __ASM_EXTENSIONS_H > > +#define __ASM_EXTENSIONS_H > > + > > +#include > > +#include > > + > > +#define THEAD_ISA_EXT0 (RISCV_HWPROBE_VENDOR_EXTENSION_SPACE) > > +#define THEAD_ISA_EXT0_V0_7_1 (1 << 0) > > + > > +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair, > > + const struct cpumask *cpus); > > +#endif > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > index 2351a5f7b8b1..58b12eaeaf46 100644 > > --- a/arch/riscv/kernel/sys_riscv.c > > +++ b/arch/riscv/kernel/sys_riscv.c > > @@ -13,6 +13,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -192,6 +193,25 @@ static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair, > > const struct cpumask *cpus) > > { > > switch (mvendorid) { > > +#ifdef CONFIG_VENDOR_EXTENSIONS_THEAD > > Please use IS_ENABLED() in code where possible, so that we get compile > time coverage of the code it disables. IMO, this kinda overcomplicates > the switch anyway, and it should be as simple as: > case THEAD_VENDOR_ID: > return hwprobe_thead(pair, cpus); > > and deal with the specific stuff (like impid etc) inside that function. > > > + case THEAD_VENDOR_ID: > > + struct riscv_hwprobe marchid = { > > + .key = RISCV_HWPROBE_KEY_MARCHID, > > + .value = 0 > > + }; > > + struct riscv_hwprobe mimpid = { > > + .key = RISCV_HWPROBE_KEY_MIMPID, > > + .value = 0 > > + }; > > + > > + hwprobe_arch_id(&marchid, cpus); > > + hwprobe_arch_id(&mimpid, cpus); > > + if (marchid.value != -1ULL && mimpid.value != -1ULL) > > + hwprobe_thead(marchid.value, mimpid.value, pair, cpus); > > + else > > + return -1; > > + break; > > +#endif > > default: > > return -1; > > } > > diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile > > index e815895e9372..38c3e80469fd 100644 > > --- a/arch/riscv/vendor_extensions/Makefile > > +++ b/arch/riscv/vendor_extensions/Makefile > > @@ -1,3 +1,5 @@ > > ifdef CONFIG_RELOCATABLE > > KBUILD_CFLAGS += -fno-pie > > endif > > Again, why do you need this, or... This file is properly filled out in the next patch in the series but I wanted to break it up. I saw this in the errata Makefiles so I assumed it would be needed here. > > + > > +obj-$(CONFIG_VENDOR_EXTENSIONS_THEAD) += thead/ > > diff --git a/arch/riscv/vendor_extensions/thead/Makefile b/arch/riscv/vendor_extensions/thead/Makefile > > new file mode 100644 > > index 000000000000..7cf43c629b66 > > --- /dev/null > > +++ b/arch/riscv/vendor_extensions/thead/Makefile > > @@ -0,0 +1,8 @@ > > +ifdef CONFIG_FTRACE > > +CFLAGS_REMOVE_extensions.o = $(CC_FLAGS_FTRACE) > > +endif > > +ifdef CONFIG_KASAN > > +KASAN_SANITIZE_extensions.o := n > > +endif > > ...any of this? Not saying you don't, but I think it should be explained. > Same reasoning as above, I can remove it if it's not needed. > > + > > +obj-y += extensions.o > > diff --git a/arch/riscv/vendor_extensions/thead/extensions.c b/arch/riscv/vendor_extensions/thead/extensions.c > > new file mode 100644 > > index 000000000000..a177501bc99c > > --- /dev/null > > +++ b/arch/riscv/vendor_extensions/thead/extensions.c > > @@ -0,0 +1,24 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) 2023 by Rivos Inc. > > + */ > > + > > +#include > > + > > +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair, > > + const struct cpumask *cpus) > > +{ > > + pair->value = 0; > > + switch (pair->key) { > > + case THEAD_ISA_EXT0: > > +#ifdef CONFIG_RISCV_ISA_V > > As pointed out by Remi, this doesn't work either. > You should not claim this is supported, just because V is, you also need > the support for their vector "flavour" from Heiko's series. > > Plus, it should be IS_ENABLED() too. > > Cheers, > Conor. > The thought process was that it should only matter if they care about V. However since they are different versions of V I could see it being better to not depend on the same config. > > + if (marchid == 0 && mimpid == 0) > > + pair->value |= THEAD_ISA_EXT0_V0_7_1; > > +#endif > > + break; > > + default: > > + return -1; > > + } > > + > > + return 0; > > +} > > > > -- > > 2.41.0 > > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv