[PATCH 1/4] RISC-V: Remove per cpu clocksource

[PATCH 1/4] RISC-V: Remove per cpu clocksource

[Atish Patra]

There is only one clocksource in RISC-V. The boot cpu initializes
that clocksource. No need to keep a percpu data structure.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 drivers/clocksource/timer-riscv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 5e6038fbf115..09e031176bc6 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -55,7 +55,7 @@ static u64 riscv_sched_clock(void)
 	return get_cycles64();
 }
 
-static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
+static struct clocksource riscv_clocksource = {
 	.name		= "riscv_clocksource",
 	.rating		= 300,
 	.mask		= CLOCKSOURCE_MASK(64),
@@ -92,7 +92,6 @@ void riscv_timer_interrupt(void)
 static int __init riscv_timer_init_dt(struct device_node *n)
 {
 	int cpuid, hartid, error;
-	struct clocksource *cs;
 
 	hartid = riscv_of_processor_hartid(n);
 	if (hartid < 0) {
@@ -112,8 +111,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
 
 	pr_info("%s: Registering clocksource cpuid [%d] hartid [%d]\n",
 	       __func__, cpuid, hartid);
-	cs = per_cpu_ptr(&riscv_clocksource, cpuid);
-	error = clocksource_register_hz(cs, riscv_timebase);
+	error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
 	if (error) {
 		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
 		       error, cpuid);
-- 
2.21.0

[PATCH 2/4] RISC-V: Add riscv_isa reprensenting ISA features common across CPUs

[Atish Patra]

From: Anup Patel <anup.patel@wdc.com>

This patch adds riscv_isa integer to represent ISA features common
across all CPUs. The riscv_isa is not same as elf_hwcap because
elf_hwcap will only have ISA features relevant for user-space apps
whereas riscv_isa will have ISA features relevant to both kernel
and user-space apps.

One of the use case is KVM hypervisor where riscv_isa will be used
to do following operations:

1. Check whether hypervisor extension is available
2. Find ISA features that need to be virtualized (e.g. floating
   point support, vector extension, etc.)

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/include/asm/hwcap.h | 25 ++++++++++++++++++++++
 arch/riscv/kernel/cpufeature.c | 39 +++++++++++++++++++++++++++++++---
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 7ecb7c6a57b1..e069f60ad5d2 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -22,5 +22,30 @@ enum {
 };
 
 extern unsigned long elf_hwcap;
+
+#define RISCV_ISA_EXT_A		(1UL << ('A' - 'A'))
+#define RISCV_ISA_EXT_a		RISCV_ISA_EXT_A
+#define RISCV_ISA_EXT_C		(1UL << ('C' - 'A'))
+#define RISCV_ISA_EXT_c		RISCV_ISA_EXT_C
+#define RISCV_ISA_EXT_D		(1UL << ('D' - 'A'))
+#define RISCV_ISA_EXT_d		RISCV_ISA_EXT_D
+#define RISCV_ISA_EXT_F		(1UL << ('F' - 'A'))
+#define RISCV_ISA_EXT_f		RISCV_ISA_EXT_F
+#define RISCV_ISA_EXT_H		(1UL << ('H' - 'A'))
+#define RISCV_ISA_EXT_h		RISCV_ISA_EXT_H
+#define RISCV_ISA_EXT_I		(1UL << ('I' - 'A'))
+#define RISCV_ISA_EXT_i		RISCV_ISA_EXT_I
+#define RISCV_ISA_EXT_M		(1UL << ('M' - 'A'))
+#define RISCV_ISA_EXT_m		RISCV_ISA_EXT_M
+#define RISCV_ISA_EXT_S		(1UL << ('S' - 'A'))
+#define RISCV_ISA_EXT_s		RISCV_ISA_EXT_S
+#define RISCV_ISA_EXT_U		(1UL << ('U' - 'A'))
+#define RISCV_ISA_EXT_u		RISCV_ISA_EXT_U
+
+extern unsigned long riscv_isa;
+
+#define riscv_isa_extension_available(ext_char)	\
+		(riscv_isa & RISCV_ISA_EXT_##ext_char)
+
 #endif
 #endif
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index b1ade9a49347..d76c806b4fc9 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -12,6 +12,7 @@
 #include <asm/smp.h>
 
 unsigned long elf_hwcap __read_mostly;
+unsigned long riscv_isa __read_mostly;
 #ifdef CONFIG_FPU
 bool has_fpu __read_mostly;
 #endif
@@ -20,7 +21,8 @@ void riscv_fill_hwcap(void)
 {
 	struct device_node *node;
 	const char *isa;
-	size_t i;
+	char print_str[BITS_PER_LONG+1];
+	size_t i, j, isa_len;
 	static unsigned long isa2hwcap[256] = {0};
 
 	isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I;
@@ -31,9 +33,11 @@ void riscv_fill_hwcap(void)
 	isa2hwcap['c'] = isa2hwcap['C'] = COMPAT_HWCAP_ISA_C;
 
 	elf_hwcap = 0;
+	riscv_isa = 0;
 
 	for_each_of_cpu_node(node) {
 		unsigned long this_hwcap = 0;
+		unsigned long this_isa = 0;
 
 		if (riscv_of_processor_hartid(node) < 0)
 			continue;
@@ -43,8 +47,22 @@ void riscv_fill_hwcap(void)
 			continue;
 		}
 
-		for (i = 0; i < strlen(isa); ++i)
+		i = 0;
+		isa_len = strlen(isa);
+#if defined(CONFIG_32BIT)
+		if (strncasecmp(isa, "rv32", 4) != 0)
+			i += 4;
+#elif defined(CONFIG_64BIT)
+		if (strncasecmp(isa, "rv64", 4) != 0)
+			i += 4;
+#endif
+		for (; i < isa_len; ++i) {
 			this_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
+			if ('a' <= isa[i] && isa[i] <= 'z')
+				this_isa |= (1UL << (isa[i] - 'a'));
+			if ('A' <= isa[i] && isa[i] <= 'Z')
+				this_isa |= (1UL << (isa[i] - 'A'));
+		}
 
 		/*
 		 * All "okay" hart should have same isa. Set HWCAP based on
@@ -55,6 +73,11 @@ void riscv_fill_hwcap(void)
 			elf_hwcap &= this_hwcap;
 		else
 			elf_hwcap = this_hwcap;
+
+		if (riscv_isa)
+			riscv_isa &= this_isa;
+		else
+			riscv_isa = this_isa;
 	}
 
 	/* We don't support systems with F but without D, so mask those out
@@ -64,7 +87,17 @@ void riscv_fill_hwcap(void)
 		elf_hwcap &= ~COMPAT_HWCAP_ISA_F;
 	}
 
-	pr_info("elf_hwcap is 0x%lx\n", elf_hwcap);
+	memset(print_str, 0, sizeof(print_str));
+	for (i = 0, j = 0; i < BITS_PER_LONG; i++)
+		if (riscv_isa & (1UL << i))
+			print_str[j++] = (char)('A' + i);
+	pr_info("riscv: ISA extensions %s\n", print_str);
+
+	memset(print_str, 0, sizeof(print_str));
+	for (i = 0, j = 0; i < BITS_PER_LONG; i++)
+		if (elf_hwcap & (1UL << i))
+			print_str[j++] = (char)('A' + i);
+	pr_info("riscv: ELF capabilities %s\n", print_str);
 
 #ifdef CONFIG_FPU
 	if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D))
-- 
2.21.0

[PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Atish Patra]

As per riscv specification, ISA naming strings are
case insensitive. However, currently only lower case
strings are parsed during cpu procfs.

Support parsing of upper case letters as well.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/kernel/cpu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 7da3c6a93abd..185143478830 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -5,6 +5,7 @@
 
 #include <linux/init.h>
 #include <linux/seq_file.h>
+#include <linux/ctype.h>
 #include <linux/of.h>
 #include <asm/smp.h>
 
@@ -57,10 +58,10 @@ static void print_isa(struct seq_file *f, const char *orig_isa)
 	 * kernels on harts with the same ISA that the kernel is compiled for.
 	 */
 #if defined(CONFIG_32BIT)
-	if (strncmp(isa, "rv32i", 5) != 0)
+	if (strncasecmp(isa, "rv32i", 5) != 0)
 		return;
 #elif defined(CONFIG_64BIT)
-	if (strncmp(isa, "rv64i", 5) != 0)
+	if (strncasecmp(isa, "rv64i", 5) != 0)
 		return;
 #endif
 
@@ -76,8 +77,8 @@ static void print_isa(struct seq_file *f, const char *orig_isa)
 	 * extension from userspace as it's not accessible from there.
 	 */
 	for (e = ext; *e != '\0'; ++e) {
-		if (isa[0] == e[0]) {
-			if (isa[0] != 's')
+		if (tolower(isa[0]) == e[0]) {
+			if (tolower(isa[0] != 's'))
 				seq_write(f, isa, 1);
 
 			isa++;
-- 
2.21.0

[PATCH 4/4] RISC-V: Fix unsupported isa string info.

[Atish Patra]

Currently, kernel prints a info warning if any of the extensions
from "mafdcsu" is missing in device tree. This is not entirely
correct as Linux can boot with "f or d" extensions if kernel is
configured accordingly. Moreover, it will continue to print the
info string for future extensions such as hypervisor as well which
is misleading. /proc/cpuinfo also doesn't print any other extensions
except "mafdcsu".

Make sure that info log is only printed only if kernel is configured
to have any mandatory extensions but device tree doesn't describe it.
All the extensions present in device tree and follow the order
described in the RISC-V specification (except 'S') are printed via
/proc/cpuinfo always.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/kernel/cpu.c | 47 ++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 185143478830..3d050440364c 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -8,6 +8,7 @@
 #include <linux/ctype.h>
 #include <linux/of.h>
 #include <asm/smp.h>
+#include <asm/hwcap.h>
 
 /*
  * Returns the hart ID of the given device tree node, or -ENODEV if the node
@@ -47,11 +48,14 @@ int riscv_of_processor_hartid(struct device_node *node)
 
 #ifdef CONFIG_PROC_FS
 
-static void print_isa(struct seq_file *f, const char *orig_isa)
+static void print_isa(struct seq_file *f, const char *orig_isa,
+		      unsigned long cpuid)
 {
-	static const char *ext = "mafdcsu";
+	static const char *mandatory_ext = "mafdcsu";
 	const char *isa = orig_isa;
 	const char *e;
+	char unsupported_isa[26] = {0};
+	int index = 0;
 
 	/*
 	 * Linux doesn't support rv32e or rv128i, and we only support booting
@@ -71,27 +75,50 @@ static void print_isa(struct seq_file *f, const char *orig_isa)
 	isa += 5;
 
 	/*
-	 * Check the rest of the ISA string for valid extensions, printing those
-	 * we find.  RISC-V ISA strings define an order, so we only print the
+	 * RISC-V ISA strings define an order, so we only print all the
 	 * extension bits when they're in order. Hide the supervisor (S)
 	 * extension from userspace as it's not accessible from there.
+	 * Throw a warning only if any mandatory extensions are not available
+	 * and kernel is configured to have that mandatory extensions.
 	 */
-	for (e = ext; *e != '\0'; ++e) {
-		if (tolower(isa[0]) == e[0]) {
+	for (e = mandatory_ext; *e != '\0'; ++e) {
+		if (tolower(isa[0]) != e[0]) {
+#if defined(CONFIG_ISA_RISCV_C)
+			if (tolower(isa[0] == 'c'))
+				continue;
+#endif
+#if defined(CONFIG_FP)
+			if ((tolower(isa[0]) == 'f') || tolower(isa[0] == 'd'))
+				continue;
+#endif
+			unsupported_isa[index] = e[0];
+			index++;
+		}
+		if (isa[0] != '\0') {
+			/* Only write if part of isa string */
 			if (tolower(isa[0] != 's'))
 				seq_write(f, isa, 1);
-
 			isa++;
 		}
 	}
+	if (isa[0] != '\0') {
+		/* Add remainging isa strings */
+		for (e = isa; *e != '\0'; ++e) {
+#if !defined(CONFIG_VIRTUALIZATION)
+			if ((tolower(e[0]) != 'h'))
+#endif
+				seq_write(f, e, 1);
+		}
+	}
 	seq_puts(f, "\n");
 
 	/*
 	 * If we were given an unsupported ISA in the device tree then print
 	 * a bit of info describing what went wrong.
 	 */
-	if (isa[0] != '\0')
-		pr_info("unsupported ISA \"%s\" in device tree\n", orig_isa);
+	if (unsupported_isa[0])
+		pr_info("unsupported ISA extensions \"%s\" in device tree for cpu [%ld]\n",
+			unsupported_isa, cpuid);
 }
 
 static void print_mmu(struct seq_file *f, const char *mmu_type)
@@ -135,7 +162,7 @@ static int c_show(struct seq_file *m, void *v)
 	seq_printf(m, "processor\t: %lu\n", cpu_id);
 	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
 	if (!of_property_read_string(node, "riscv,isa", &isa))
-		print_isa(m, isa);
+		print_isa(m, isa, cpu_id);
 	if (!of_property_read_string(node, "mmu-type", &mmu))
 		print_mmu(m, mmu);
 	if (!of_property_read_string(node, "compatible", &compat)
-- 
2.21.0

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Paul Walmsley]

On Fri, 26 Jul 2019, Atish Patra wrote:

> As per riscv specification, ISA naming strings are
> case insensitive. However, currently only lower case
> strings are parsed during cpu procfs.
> 
> Support parsing of upper case letters as well.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

Is there a use case that's driving this, or can we just say, "use 
lowercase letters" and leave it at that?


- Paul

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Atish Patra]

On 7/26/19 1:47 PM, Paul Walmsley wrote:
> On Fri, 26 Jul 2019, Atish Patra wrote:
> 
>> As per riscv specification, ISA naming strings are
>> case insensitive. However, currently only lower case
>> strings are parsed during cpu procfs.
>>
>> Support parsing of upper case letters as well.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> 
> Is there a use case that's driving this, or 

Currently, we use all lower case isa string in kvmtool. But somebody can 
have uppercase letters in future as spec allows it.


can we just say, "use
> lowercase letters" and leave it at that?
> 

In that case, it will not comply with RISC-V spec. Is that okay ?

> 
> - Paul
> 


-- 
Regards,
Atish

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Paul Walmsley]

On Fri, 26 Jul 2019, Atish Patra wrote:

> On 7/26/19 1:47 PM, Paul Walmsley wrote:
> > On Fri, 26 Jul 2019, Atish Patra wrote:
> > 
> > > As per riscv specification, ISA naming strings are
> > > case insensitive. However, currently only lower case
> > > strings are parsed during cpu procfs.
> > > 
> > > Support parsing of upper case letters as well.
> > > 
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > 
> > Is there a use case that's driving this, or 
> 
> Currently, we use all lower case isa string in kvmtool. But somebody can have
> uppercase letters in future as spec allows it.
>
> 
> can we just say, "use
> > lowercase letters" and leave it at that?
> > 
> 
> In that case, it will not comply with RISC-V spec. Is that okay ?

I think that section of the specification is mostly concerned with someone 
trying to define "f" as a different extension than "F", or something like 
that.  I'm not sure that it imposes any constraint that software must 
accept both upper and lower case ISA strings.  

What gives me pause here is that this winds up impacting DT schema 
validation:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/cpus.yaml#n41



- Paul

RE: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Anup Patel]

> -----Original Message-----
> From: Paul Walmsley <paul.walmsley@sifive.com>
> Sent: Saturday, July 27, 2019 5:00 AM
> To: Atish Patra <Atish.Patra@wdc.com>
> Cc: linux-kernel@vger.kernel.org; Alan Kao <alankao@andestech.com>;
> Albert Ou <aou@eecs.berkeley.edu>; Allison Randal <allison@lohutok.net>;
> Anup Patel <Anup.Patel@wdc.com>; Daniel Lezcano
> <daniel.lezcano@linaro.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Johan Hovold <johan@kernel.org>; linux-
> riscv@lists.infradead.org; Palmer Dabbelt <palmer@sifive.com>; Thomas
> Gleixner <tglx@linutronix.de>
> Subject: Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.
> 
> On Fri, 26 Jul 2019, Atish Patra wrote:
> 
> > On 7/26/19 1:47 PM, Paul Walmsley wrote:
> > > On Fri, 26 Jul 2019, Atish Patra wrote:
> > >
> > > > As per riscv specification, ISA naming strings are case
> > > > insensitive. However, currently only lower case strings are parsed
> > > > during cpu procfs.
> > > >
> > > > Support parsing of upper case letters as well.
> > > >
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > >
> > > Is there a use case that's driving this, or
> >
> > Currently, we use all lower case isa string in kvmtool. But somebody
> > can have uppercase letters in future as spec allows it.
> >
> >
> > can we just say, "use
> > > lowercase letters" and leave it at that?
> > >
> >
> > In that case, it will not comply with RISC-V spec. Is that okay ?
> 
> I think that section of the specification is mostly concerned with someone
> trying to define "f" as a different extension than "F", or something like that.
> I'm not sure that it imposes any constraint that software must accept both
> upper and lower case ISA strings.
> 
> What gives me pause here is that this winds up impacting DT schema
> validation:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> mentation/devicetree/bindings/riscv/cpus.yaml#n41

If 'f' and 'F' mean same extension as-per RISC-V spec then software should also
interpret it that way hence this patch.

Regards,
Anup

RE: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Paul Walmsley]

On Sat, 27 Jul 2019, Anup Patel wrote:

> > -----Original Message-----
> > From: Paul Walmsley <paul.walmsley@sifive.com>
> > Sent: Saturday, July 27, 2019 5:00 AM
> > 
> > On Fri, 26 Jul 2019, Atish Patra wrote:
> > 
> > > On 7/26/19 1:47 PM, Paul Walmsley wrote:
> > > > On Fri, 26 Jul 2019, Atish Patra wrote:
> > > >
> > > > > As per riscv specification, ISA naming strings are case
> > > > > insensitive. However, currently only lower case strings are parsed
> > > > > during cpu procfs.
> > > > >
> > > > > Support parsing of upper case letters as well.
> > > > >
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > >
> > > > Is there a use case that's driving this, or
> > >
> > > Currently, we use all lower case isa string in kvmtool. But somebody
> > > can have uppercase letters in future as spec allows it.
> > >
> > >
> > > can we just say, "use
> > > > lowercase letters" and leave it at that?
> > > >
> > >
> > > In that case, it will not comply with RISC-V spec. Is that okay ?
> > 
> > I think that section of the specification is mostly concerned with someone
> > trying to define "f" as a different extension than "F", or something like that.
> > I'm not sure that it imposes any constraint that software must accept both
> > upper and lower case ISA strings.
> > 
> > What gives me pause here is that this winds up impacting DT schema
> > validation:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> > mentation/devicetree/bindings/riscv/cpus.yaml#n41
> 
> If 'f' and 'F' mean same extension as-per RISC-V spec then software should also
> interpret it that way hence this patch.

The list of valid RISC-V ISA strings is already constrained by the DT 
schema to be all lowercase.  Anything else would violate the schema.

I'd take a patch that would pr_warn() or a BUG() if any uppercase letters 
were found in the riscv,isa DT property, though, in case the developer 
skipped the DT schema validator.


- Paul

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Anup Patel]

On Sat, Jul 27, 2019 at 1:23 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Sat, 27 Jul 2019, Anup Patel wrote:
>
> > > -----Original Message-----
> > > From: Paul Walmsley <paul.walmsley@sifive.com>
> > > Sent: Saturday, July 27, 2019 5:00 AM
> > >
> > > On Fri, 26 Jul 2019, Atish Patra wrote:
> > >
> > > > On 7/26/19 1:47 PM, Paul Walmsley wrote:
> > > > > On Fri, 26 Jul 2019, Atish Patra wrote:
> > > > >
> > > > > > As per riscv specification, ISA naming strings are case
> > > > > > insensitive. However, currently only lower case strings are parsed
> > > > > > during cpu procfs.
> > > > > >
> > > > > > Support parsing of upper case letters as well.
> > > > > >
> > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > >
> > > > > Is there a use case that's driving this, or
> > > >
> > > > Currently, we use all lower case isa string in kvmtool. But somebody
> > > > can have uppercase letters in future as spec allows it.
> > > >
> > > >
> > > > can we just say, "use
> > > > > lowercase letters" and leave it at that?
> > > > >
> > > >
> > > > In that case, it will not comply with RISC-V spec. Is that okay ?
> > >
> > > I think that section of the specification is mostly concerned with someone
> > > trying to define "f" as a different extension than "F", or something like that.
> > > I'm not sure that it imposes any constraint that software must accept both
> > > upper and lower case ISA strings.
> > >
> > > What gives me pause here is that this winds up impacting DT schema
> > > validation:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> > > mentation/devicetree/bindings/riscv/cpus.yaml#n41
> >
> > If 'f' and 'F' mean same extension as-per RISC-V spec then software should also
> > interpret it that way hence this patch.
>
> The list of valid RISC-V ISA strings is already constrained by the DT
> schema to be all lowercase.  Anything else would violate the schema.
>
> I'd take a patch that would pr_warn() or a BUG() if any uppercase letters
> were found in the riscv,isa DT property, though, in case the developer
> skipped the DT schema validator.

If your only objection is uppercase letter not agreeing with YMAL schema
then why not fix the YMAL schema to have regex for RISC-V ISA string?

The YMAL schema should not enforce any artificial restriction which is
theoretically allowed in the RISC-V spec.

Regards,
Anup

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Paul Walmsley]

On Sat, 27 Jul 2019, Anup Patel wrote:

> If your only objection is uppercase letter not agreeing with YMAL schema
> then why not fix the YMAL schema to have regex for RISC-V ISA string?

I don't agree with you that the specification compels software to accept 
arbitrary case combinations in the riscv,isa DT string.

> The YMAL schema should not enforce any artificial restriction which is
> theoretically allowed in the RISC-V spec.

Unless someone can come up with a compelling reason for why restricting 
the DT ISA strings to all lowercase letters and numbers is insufficient to 
express the full range of options in the spec, the additional complexity 
to add mixed-case parsing, both in this patch and in the other patches in 
this series, seems pointless.


- Paul

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Anup Patel]

On Sat, Jul 27, 2019 at 1:46 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Sat, 27 Jul 2019, Anup Patel wrote:
>
> > If your only objection is uppercase letter not agreeing with YMAL schema
> > then why not fix the YMAL schema to have regex for RISC-V ISA string?
>
> I don't agree with you that the specification compels software to accept
> arbitrary case combinations in the riscv,isa DT string.

DT describes HW and HW follows RISC-V spec.

Enforcing software choices in DT YMAL schema is not correct approach.

Some other OS (such as FreeBSD, NetBSD, etc) might choose to go with
upper-case characters only in their DTS files.

>
> > The YMAL schema should not enforce any artificial restriction which is
> > theoretically allowed in the RISC-V spec.
>
> Unless someone can come up with a compelling reason for why restricting
> the DT ISA strings to all lowercase letters and numbers is insufficient to
> express the full range of options in the spec, the additional complexity
> to add mixed-case parsing, both in this patch and in the other patches in
> this series, seems pointless.

So, using strncasecmp() in-place of strncmp() and using tolower() for
each character comparison is complex for you ?

Why do we need a pointless restriction  in YAML schema ?

Regards,
Anup

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Andreas Schwab]

On Jul 27 2019, Anup Patel <anup@brainfault.org> wrote:

> So, using strncasecmp() in-place of strncmp() and using tolower() for
> each character comparison is complex for you ?

Apparently too complex for you.

+			if (tolower(isa[0] != 's'))

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Atish Patra]

On Sat, 2019-07-27 at 01:16 -0700, Paul Walmsley wrote:
> On Sat, 27 Jul 2019, Anup Patel wrote:
> 
> > If your only objection is uppercase letter not agreeing with YMAL
> > schema
> > then why not fix the YMAL schema to have regex for RISC-V ISA
> > string?
> 
> I don't agree with you that the specification compels software to
> accept 
> arbitrary case combinations in the riscv,isa DT string.
> 
> > The YMAL schema should not enforce any artificial restriction which
> > is
> > theoretically allowed in the RISC-V spec.
> 
> Unless someone can come up with a compelling reason for why
> restricting 
> the DT ISA strings to all lowercase letters and numbers is
> insufficient to 
> express the full range of options in the spec,

The yaml document did not specify anything about all isa-strings has to
be lowercase unless I missed something. The two enum values are all
lowercase but the description says all ISA strings are documented in
ISA specification which describes the ISA strings to be case
insensitive. IMHO, this creates confusion resulting the patch.
The existing enum strings are already outdated with kvm patchset.

I am fine with dropping this patch if yaml clearly document the case
sensititve thing.

Following approaches can done to avoid this confusion in future.

1. Update the enum strings with every new extension added.
	- Good chance that somebody miss something and this gets
outdated in future.

2. Just add one line in DT binding description saying that 

"All isa strings has to be lowercase strings". We should mandate this
in Unix Platform specification as well to be in sync.

Thoughts ?

>  the additional complexity 
> to add mixed-case parsing, both in this patch and in the other
> patches in 
> this series, seems pointless.
> 


> 
> - Paul
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

-- 
Regards,
Atish

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Palmer Dabbelt]

On Fri, 26 Jul 2019 15:20:47 PDT (-0700), Atish Patra wrote:
> On 7/26/19 1:47 PM, Paul Walmsley wrote:
>> On Fri, 26 Jul 2019, Atish Patra wrote:
>>
>>> As per riscv specification, ISA naming strings are
>>> case insensitive. However, currently only lower case
>>> strings are parsed during cpu procfs.
>>>
>>> Support parsing of upper case letters as well.
>>>
>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>
>> Is there a use case that's driving this, or
>
> Currently, we use all lower case isa string in kvmtool. But somebody can
> have uppercase letters in future as spec allows it.
>
>
> can we just say, "use
>> lowercase letters" and leave it at that?
>>
>
> In that case, it will not comply with RISC-V spec. Is that okay ?

We could make the platform spec say "use lowercase letters" and wipe our hands
of it -- IIRC we still only support the lower case letters in GCC due to
multilib headaches, so it's kind of the de-facto standard already.

>
>>
>> - Paul
>>

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Atish Patra]

On Mon, 2019-07-29 at 20:42 -0700, Palmer Dabbelt wrote:
> On Fri, 26 Jul 2019 15:20:47 PDT (-0700), Atish Patra wrote:
> > On 7/26/19 1:47 PM, Paul Walmsley wrote:
> > > On Fri, 26 Jul 2019, Atish Patra wrote:
> > > 
> > > > As per riscv specification, ISA naming strings are
> > > > case insensitive. However, currently only lower case
> > > > strings are parsed during cpu procfs.
> > > > 
> > > > Support parsing of upper case letters as well.
> > > > 
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > 
> > > Is there a use case that's driving this, or
> > 
> > Currently, we use all lower case isa string in kvmtool. But
> > somebody can
> > have uppercase letters in future as spec allows it.
> > 
> > 
> > can we just say, "use
> > > lowercase letters" and leave it at that?
> > > 
> > 
> > In that case, it will not comply with RISC-V spec. Is that okay ?
> 
> We could make the platform spec say "use lowercase letters" and wipe
> our hands
> of it -- IIRC we still only support the lower case letters in GCC due
> to
> multilib headaches, so it's kind of the de-facto standard already.
> 

Sounds good. That's what I suggested in earlier email as well.
It would be good to add "use lowercase letters" in yaml documentation
as well just to avoid any confusion at all.

I will send a v2 soon.

Regards,
Atish
> > > - Paul
> > > 

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Paul Walmsley]

On Sat, 27 Jul 2019, Anup Patel wrote:

> On Sat, Jul 27, 2019 at 1:46 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> >
> > On Sat, 27 Jul 2019, Anup Patel wrote:
> >
> > > If your only objection is uppercase letter not agreeing with YMAL 
> > > schema then why not fix the YMAL schema to have regex for RISC-V ISA 
> > > string?
> >
> > I don't agree with you that the specification compels software to 
> > accept arbitrary case combinations in the riscv,isa DT string.
> 
> DT describes HW and HW follows RISC-V spec.

The RISC-V specification doesn't specify anything about how the DT data is 
to describe the hardware.

> Enforcing software choices in DT YMAL schema is not correct approach.

That's the point of the DT YAML schemas.


- Paul

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Paul Walmsley]

On Mon, 29 Jul 2019, Atish Patra wrote:

> The yaml document did not specify anything about all isa-strings has to 
> be lowercase unless I missed something. The two enum values are all 
> lowercase but the description says all ISA strings are documented in ISA 
> specification which describes the ISA strings to be case insensitive. 
> IMHO, this creates confusion resulting the patch.

If it's helpful in understanding my earlier comments, I don't think that 
your patches were "wrong," or anything like that.  And you're right that 
the DT YAML binding does not unequivocally state that all future additions 
to the riscv,isa string must be in lowercase.  But to be clear, the DT 
YAML schema defines exactly what is currently permissible for riscv,isa 
strings in kernel-oriented DT data, and right now, all of the permissible 
values are lowercase.

Good Linux kernel patches are driven by clear functional motivations.  
Like, "The current kernel crashes or doesn't support the hardware in 
situation X; thus we change the kernel to add feature or bugfix Y."  And 
in the kernel, solutions that involve fewer lines of code are generally 
preferred to solutions that involve more lines of code - more bugs, more 
noise, etc.  

Where these case-insensitivity parsing patches fall short, in my opinion, 
is that they don't have strong functional motivations.  There's been a 
precedent for a few years now in the mainline kernel that the RISC-V ISA 
string is all lowercase.  I've asked you and Anup for situations where 
that precedent isn't sufficient to handle functionality that's described 
in the RISC-V specification, or to phrase it differently, "what breaks if 
we don't make this change?"  So far no one's been able to cite anything 
here.  There's just a repeated appeal to authority to the section of the 
RISC-V specification that states that "[t]he ISA naming strings are case 
insensitive."  As you can probably sense, this doesn't seem like a strong 
justification for changing the existing behavior.  From a functional point 
of view, if case doesn't matter, why care if the DT data and kernel only 
support lowercase strings?  An all-lowercase string should be functionally 
equivalent to an all-uppercase or mixed-case string.  Since there's no 
functional point to the changes, why add more code to the kernel?

Later in your E-mail, it sounds like you ultimately agree with these basic 
conclusions.  If that's so, I don't understand the amount of effort that 
you and Anup have put into pushing back here.  There's nothing personal 
about these review comments.  If there's some meta-problem here that's 
unrelated to the technical merit of the patches, please send a private 
E-mail.  Otherwise, if you disagree with the functional conclusions in the 
previous paragraph, let's hash the issues out here.

> I am fine with dropping this patch if yaml clearly document the case 
> sensititve thing.

Great!  If you still think so now, let's resolve this issue with a 
one-line patch to the DT YAML schema to note that riscv,isa DT string 
values should be all lowercase.  Will you send a patch?


- Paul

Re: [PATCH 3/4] RISC-V: Support case insensitive ISA string parsing.

[Atish Patra]

On Tue, 2019-07-30 at 17:08 -0700, Paul Walmsley wrote:
> On Mon, 29 Jul 2019, Atish Patra wrote:
> 
> > The yaml document did not specify anything about all isa-strings
> > has to 
> > be lowercase unless I missed something. The two enum values are
> > all 
> > lowercase but the description says all ISA strings are documented
> > in ISA 
> > specification which describes the ISA strings to be case
> > insensitive. 
> > IMHO, this creates confusion resulting the patch.
> 
> If it's helpful in understanding my earlier comments, I don't think
> that 
> your patches were "wrong," or anything like that.  And you're right
> that 
> the DT YAML binding does not unequivocally state that all future
> additions 
> to the riscv,isa string must be in lowercase.  But to be clear, the
> DT 
> YAML schema defines exactly what is currently permissible for
> riscv,isa 
> strings in kernel-oriented DT data, and right now, all of the
> permissible 
> values are lowercase.
> 

Going forward, yaml schema should define only the mandatory isa strings
(i.e. rv64imafdc) or the list should be updated as we keep adding new
extensions (i.e. rv64imafdch with kvm patches).


> Good Linux kernel patches are driven by clear functional
> motivations.  
> Like, "The current kernel crashes or doesn't support the hardware in 
> situation X; thus we change the kernel to add feature or bugfix
> Y."  And 
> in the kernel, solutions that involve fewer lines of code are
> generally 
> preferred to solutions that involve more lines of code - more bugs,
> more 
> noise, etc.  
> 

I completely agree with you on this.

> Where these case-insensitivity parsing patches fall short, in my
> opinion, 
> is that they don't have strong functional motivations.  There's been
> a 
> precedent for a few years now in the mainline kernel that the RISC-V
> ISA 
> string is all lowercase.  I've asked you and Anup for situations
> where 
> that precedent isn't sufficient to handle functionality that's
> described 
> in the RISC-V specification, or to phrase it differently, "what
> breaks if 
> we don't make this change?"  So far no one's been able to cite
> anything 
> here.  There's just a repeated appeal to authority to the section of
> the 
> RISC-V specification that states that "[t]he ISA naming strings are
> case 
> insensitive."  As you can probably sense, this doesn't seem like a
> strong 
> justification for changing the existing behavior.  From a functional
> point 
> of view, if case doesn't matter, why care if the DT data and kernel
> only 
> support lowercase strings?  An all-lowercase string should be
> functionally 
> equivalent to an all-uppercase or mixed-case string.  Since there's
> no 
> functional point to the changes,cause mysterious DT parsing problems.
>  why add more code to the kernel?
> 

There was no immediate functional requirement but to have a more future
proof code. As I said earlier, the idea of the patch came from the
confusion created by discrepancies between two different piece of
documentation/specification. As long those are resolved, absolutely no
need of this patch.


> Later in your E-mail, it sounds like you ultimately agree with these
> basic 
> conclusions.  If that's so, I don't understand the amount of effort
> that 
> you and Anup have put into pushing back here.  There's nothing
> personal 
> about these review comments.  If there's some meta-problem here
> that's 
> unrelated to the technical merit of the patches, please send a
> private 
> E-mail.  Otherwise, if you disagree with the functional conclusions
> in the 
> previous paragraph, let's hash the issues out here.
> 
> > I am fine with dropping this patch if yaml clearly document the
> > case 
> > sensititve thing.
> 
> Great!  If you still think so now, let's resolve this issue with a 
> one-line patch to the DT YAML schema to note that riscv,isa DT
> string 
> values should be all lowercase.  Will you send a patch?
> 

Yeah. Sending it right now.

Regards,
Atish
> 
> - Paul