Message-ID: <20230706-endorphin-choice-bd9da1f0bf84@spud>
Date:   Thu, 6 Jul 2023 22:24:56 +0100
From:   Conor Dooley <conor@...nel.org>
To:     Charlie Jenkins <charlie@...osinc.com>
Cc:     linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Jonathan Corbet <corbet@....net>, evan@...osinc.com,
        heiko@...ech.de, linux-doc@...r.kernel.org
Subject: Re: [PATCH 2/3] RISC-V: Add T-Head 0.7.1 vector extension to hwprobe

Hey Charlie,

On Thu, Jul 06, 2023 at 01:00:52PM -0700, Charlie Jenkins wrote:
> On Thu, Jul 06, 2023 at 06:38:17PM +0100, Conor Dooley wrote:
> > 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.

Just to be clear, you *need* to do something on top of Heiko's patches,
but with an adaptation for how you actually get the information as to
whether the device supports the extension. It makes no sense to tell
userspace that this "flavour" of V is present, if using it is going to
be problematic, as the kernel doesn't actually support it.

As I have pointed out above, while you might have a D1 with a c906 that
does have vector, other T-Head cores that respond identically to
mvendorid/marchid/mimpid may not.

For anything you do, please do it on top of my series adding a new
mechanism for parsing this information:
https://lore.kernel.org/all/20230703-repayment-vocalist-e4f3eeac2b2a@wendy/

If you have not already, you should coordinate with Heiko about what to
do here, before taking over the series he submitted.

> > > Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
> > > ---
> > >  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.

Hopefully there will be no other vendors that decide to implement
v0.7.1! We probably do need to re-do how our menus look, although I
question whether adding yet another file (we have Kconfig.socs...) is
the right thing to do.


> > > 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.

Ye, you shouldn't...

> > > +
> > > +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.

Ditto.

> > > +
> > > +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 <asm/extensions.h>
> > > +
> > > +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.

> 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. 

Yeah, you unfortunately cannot use that kconfig symbol for this purpose,
they are incompatible after all. You probably need to use some interface
like riscv_isa_extension_available() that supports the vendor flavour of
this stuff. I've given no more thought to how that should look though
than the time it took to type this out. I'd be down to collaborate on
something in that neck of the woods, once things settle down after -rc1
& I've written a patch for dealing with extensions that have multiple
subsets in the extension detection code.

Perhaps Palmer or someone at Rivos could give you a rundown of the
vector incompatibility stuff on a platform with a shorter response
time than email ;)

Cheers,
Conor.

> > > +		if (marchid == 0 && mimpid == 0)
> > > +			pair->value |= THEAD_ISA_EXT0_V0_7_1;
> > > +#endif
> > > +		break;
> > > +	default:
> > > +		return -1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > 
> > > -- 
> > > 2.41.0
> > > 
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists