All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] target/riscv: Add isa extenstion strings to the device tree
@ 2022-02-15  9:05 ` Atish Patra
  0 siblings, 0 replies; 7+ messages in thread
From: Atish Patra @ 2022-02-15  9:05 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alistair Francis, Bin Meng, Atish Patra, Palmer Dabbelt, qemu-riscv

Append the available ISA extensions to the "riscv,isa" string if it
is enabled so that kernel can process it.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b0a40b83e7a8..c70260d0df15 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -34,6 +34,9 @@
 
 /* RISC-V CPU definitions */
 
+/* This includes the null terminated character '\0' */
+#define MAX_ISA_EXT_LEN 256
+
 static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
 
 const char * const riscv_int_regnames[] = {
@@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
+static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
+{
+    int offset = strnlen(isa_str, max_str_len);
+
+    if (cpu->cfg.ext_svpbmt) {
+        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");
+    }
+    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
+        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
+    }
+    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
+        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
+    }
+}
+
 char *riscv_isa_string(RISCVCPU *cpu)
 {
     int i;
-    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
+    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
+                          MAX_ISA_EXT_LEN;
     char *isa_str = g_new(char, maxlen);
     char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
     for (i = 0; i < sizeof(riscv_exts); i++) {
@@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
         }
     }
     *p = '\0';
+    riscv_isa_string_ext(cpu, isa_str, maxlen);
+
     return isa_str;
 }
 
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH] target/riscv: Add isa extenstion strings to the device tree
@ 2022-02-15  9:05 ` Atish Patra
  0 siblings, 0 replies; 7+ messages in thread
From: Atish Patra @ 2022-02-15  9:05 UTC (permalink / raw)
  To: qemu-devel
  Cc: Atish Patra, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

Append the available ISA extensions to the "riscv,isa" string if it
is enabled so that kernel can process it.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b0a40b83e7a8..c70260d0df15 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -34,6 +34,9 @@
 
 /* RISC-V CPU definitions */
 
+/* This includes the null terminated character '\0' */
+#define MAX_ISA_EXT_LEN 256
+
 static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
 
 const char * const riscv_int_regnames[] = {
@@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
+static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
+{
+    int offset = strnlen(isa_str, max_str_len);
+
+    if (cpu->cfg.ext_svpbmt) {
+        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");
+    }
+    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
+        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
+    }
+    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
+        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
+    }
+}
+
 char *riscv_isa_string(RISCVCPU *cpu)
 {
     int i;
-    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
+    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
+                          MAX_ISA_EXT_LEN;
     char *isa_str = g_new(char, maxlen);
     char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
     for (i = 0; i < sizeof(riscv_exts); i++) {
@@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
         }
     }
     *p = '\0';
+    riscv_isa_string_ext(cpu, isa_str, maxlen);
+
     return isa_str;
 }
 
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] target/riscv: Add isa extenstion strings to the device tree
  2022-02-15  9:05 ` Atish Patra
  (?)
@ 2022-02-15  9:58 ` Heiko Stübner
  -1 siblings, 0 replies; 7+ messages in thread
From: Heiko Stübner @ 2022-02-15  9:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alistair Francis, Bin Meng, Atish Patra, Palmer Dabbelt,
	qemu-riscv, Atish Patra

Hi Atish,

Am Dienstag, 15. Februar 2022, 10:05:30 CET schrieb Atish Patra:
> Append the available ISA extensions to the "riscv,isa" string if it
> is enabled so that kernel can process it.

For people glancing at these patches, a bit more insight into
how the isa string comes together might be helpful.

See question below for the reason ;-)

> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  target/riscv/cpu.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b0a40b83e7a8..c70260d0df15 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,9 @@
>  
>  /* RISC-V CPU definitions */
>  
> +/* This includes the null terminated character '\0' */
> +#define MAX_ISA_EXT_LEN 256
> +
>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>  
>  const char * const riscv_int_regnames[] = {
> @@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>  
> +static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
> +{
> +    int offset = strnlen(isa_str, max_str_len);
> +
> +    if (cpu->cfg.ext_svpbmt) {
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");

is a double-"__" the correct divider for isa extensions?

I.e. you have "_%s" as well as "_svpbmt" here, which creates a double
underscore in the ISA string.


> +    }
> +    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
> +    }
> +    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
> +    }
> +}
> +
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
> +                          MAX_ISA_EXT_LEN;
>      char *isa_str = g_new(char, maxlen);
>      char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
>      for (i = 0; i < sizeof(riscv_exts); i++) {
> @@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
>          }
>      }
>      *p = '\0';

blank line here?

> +    riscv_isa_string_ext(cpu, isa_str, maxlen);
> +
>      return isa_str;
>  }

Heiko




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] target/riscv: Add isa extenstion strings to the device tree
  2022-02-15  9:05 ` Atish Patra
  (?)
  (?)
@ 2022-02-15 16:20 ` Heiko Stübner
  2022-02-15 19:39     ` Atish Patra
  -1 siblings, 1 reply; 7+ messages in thread
From: Heiko Stübner @ 2022-02-15 16:20 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alistair Francis, Bin Meng, Atish Patra, Palmer Dabbelt,
	qemu-riscv, Atish Patra

Am Dienstag, 15. Februar 2022, 10:05:30 CET schrieb Atish Patra:
> Append the available ISA extensions to the "riscv,isa" string if it
> is enabled so that kernel can process it.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  target/riscv/cpu.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b0a40b83e7a8..c70260d0df15 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,9 @@
>  
>  /* RISC-V CPU definitions */
>  
> +/* This includes the null terminated character '\0' */
> +#define MAX_ISA_EXT_LEN 256
> +
>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>  
>  const char * const riscv_int_regnames[] = {
> @@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>  
> +static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
> +{
> +    int offset = strnlen(isa_str, max_str_len);
> +
> +    if (cpu->cfg.ext_svpbmt) {
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");
> +    }
> +    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {

shouldn't offset + strlen("svinval") +1 be < max_str_len?
snprintf will write partial strings but this would throw off a
qemu client completely I guess.


> +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
> +    }
> +    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
> +    }

wouldn't it make more sense to do something like:

+    struct {
+        const char *value;
+        bool enabled;
+    } extensions[] = {
+        { "svpbmt", cpu->cfg.ext_svpbmt },
+        { "svinval", cpu->cfg.ext_svinval },
+        { "svnapot", cpu->cfg.ext_svnapot },
+    };
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(extensions); i++) {
+        if (!extensions[i].enabled)
+            continue;
+
+        /* check available space */
+        if (offset + strlen(extensions[i].value) + 1 > max_str_len) {
+            //do warn about exceeded length
+            return;
+        }
+
+        offset += snprintf(isa_str + offset, max_str_len, "_%s",
+                                                          extensions[i].value);
+    }

instead?

Because that list will get longer over time and repeating checks
and snprintf calls will get harder to keep in sync over time?


Heiko




> +}
> +
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
> +                          MAX_ISA_EXT_LEN;
>      char *isa_str = g_new(char, maxlen);
>      char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
>      for (i = 0; i < sizeof(riscv_exts); i++) {
> @@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
>          }
>      }
>      *p = '\0';
> +    riscv_isa_string_ext(cpu, isa_str, maxlen);
> +
>      return isa_str;
>  }
>  
> 






^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] target/riscv: Add isa extenstion strings to the device tree
  2022-02-15 16:20 ` Heiko Stübner
@ 2022-02-15 19:39     ` Atish Patra
  0 siblings, 0 replies; 7+ messages in thread
From: Atish Patra @ 2022-02-15 19:39 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: open list:RISC-V, Bin Meng, Atish Patra,
	qemu-devel@nongnu.org Developers, Palmer Dabbelt,
	Alistair Francis

On Tue, Feb 15, 2022 at 8:20 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 15. Februar 2022, 10:05:30 CET schrieb Atish Patra:
> > Append the available ISA extensions to the "riscv,isa" string if it
> > is enabled so that kernel can process it.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index b0a40b83e7a8..c70260d0df15 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -34,6 +34,9 @@
> >
> >  /* RISC-V CPU definitions */
> >
> > +/* This includes the null terminated character '\0' */
> > +#define MAX_ISA_EXT_LEN 256
> > +
> >  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> >
> >  const char * const riscv_int_regnames[] = {
> > @@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> >      device_class_set_props(dc, riscv_cpu_properties);
> >  }
> >
> > +static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
> > +{
> > +    int offset = strnlen(isa_str, max_str_len);
> > +
> > +    if (cpu->cfg.ext_svpbmt) {
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");
> > +    }
> > +    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
>
> shouldn't offset + strlen("svinval") +1 be < max_str_len?
> snprintf will write partial strings but this would throw off a
> qemu client completely I guess.
>

We need that check to put out warnings to the user.

>
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
> > +    }
> > +    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
> > +    }
>
> wouldn't it make more sense to do something like:
>
> +    struct {
> +        const char *value;
> +        bool enabled;
> +    } extensions[] = {
> +        { "svpbmt", cpu->cfg.ext_svpbmt },
> +        { "svinval", cpu->cfg.ext_svinval },
> +        { "svnapot", cpu->cfg.ext_svnapot },
> +    };
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(extensions); i++) {
> +        if (!extensions[i].enabled)
> +            continue;
> +
> +        /* check available space */
> +        if (offset + strlen(extensions[i].value) + 1 > max_str_len) {
> +            //do warn about exceeded length
> +            return;
> +        }
> +
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s",
> +                                                          extensions[i].value);
> +    }
>
> instead?
>
> Because that list will get longer over time and repeating checks
> and snprintf calls will get harder to keep in sync over time?
>

Yeah. This is much better.

>
> Heiko
>
>
>
>
> > +}
> > +
> >  char *riscv_isa_string(RISCVCPU *cpu)
> >  {
> >      int i;
> > -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> > +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
> > +                          MAX_ISA_EXT_LEN;
> >      char *isa_str = g_new(char, maxlen);
> >      char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
> >      for (i = 0; i < sizeof(riscv_exts); i++) {
> > @@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
> >          }
> >      }
> >      *p = '\0';
> > +    riscv_isa_string_ext(cpu, isa_str, maxlen);
> > +
> >      return isa_str;
> >  }
> >
> >
>
>
>
>
>


-- 
Regards,
Atish


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] target/riscv: Add isa extenstion strings to the device tree
@ 2022-02-15 19:39     ` Atish Patra
  0 siblings, 0 replies; 7+ messages in thread
From: Atish Patra @ 2022-02-15 19:39 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng,
	Atish Patra, Palmer Dabbelt, open list:RISC-V

On Tue, Feb 15, 2022 at 8:20 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 15. Februar 2022, 10:05:30 CET schrieb Atish Patra:
> > Append the available ISA extensions to the "riscv,isa" string if it
> > is enabled so that kernel can process it.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index b0a40b83e7a8..c70260d0df15 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -34,6 +34,9 @@
> >
> >  /* RISC-V CPU definitions */
> >
> > +/* This includes the null terminated character '\0' */
> > +#define MAX_ISA_EXT_LEN 256
> > +
> >  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> >
> >  const char * const riscv_int_regnames[] = {
> > @@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> >      device_class_set_props(dc, riscv_cpu_properties);
> >  }
> >
> > +static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
> > +{
> > +    int offset = strnlen(isa_str, max_str_len);
> > +
> > +    if (cpu->cfg.ext_svpbmt) {
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");
> > +    }
> > +    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
>
> shouldn't offset + strlen("svinval") +1 be < max_str_len?
> snprintf will write partial strings but this would throw off a
> qemu client completely I guess.
>

We need that check to put out warnings to the user.

>
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
> > +    }
> > +    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
> > +    }
>
> wouldn't it make more sense to do something like:
>
> +    struct {
> +        const char *value;
> +        bool enabled;
> +    } extensions[] = {
> +        { "svpbmt", cpu->cfg.ext_svpbmt },
> +        { "svinval", cpu->cfg.ext_svinval },
> +        { "svnapot", cpu->cfg.ext_svnapot },
> +    };
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(extensions); i++) {
> +        if (!extensions[i].enabled)
> +            continue;
> +
> +        /* check available space */
> +        if (offset + strlen(extensions[i].value) + 1 > max_str_len) {
> +            //do warn about exceeded length
> +            return;
> +        }
> +
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s",
> +                                                          extensions[i].value);
> +    }
>
> instead?
>
> Because that list will get longer over time and repeating checks
> and snprintf calls will get harder to keep in sync over time?
>

Yeah. This is much better.

>
> Heiko
>
>
>
>
> > +}
> > +
> >  char *riscv_isa_string(RISCVCPU *cpu)
> >  {
> >      int i;
> > -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> > +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
> > +                          MAX_ISA_EXT_LEN;
> >      char *isa_str = g_new(char, maxlen);
> >      char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
> >      for (i = 0; i < sizeof(riscv_exts); i++) {
> > @@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
> >          }
> >      }
> >      *p = '\0';
> > +    riscv_isa_string_ext(cpu, isa_str, maxlen);
> > +
> >      return isa_str;
> >  }
> >
> >
>
>
>
>
>


-- 
Regards,
Atish


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] target/riscv: Add isa extenstion strings to the device tree
  2022-02-15 19:39     ` Atish Patra
  (?)
@ 2022-02-15 21:20     ` Heiko Stübner
  -1 siblings, 0 replies; 7+ messages in thread
From: Heiko Stübner @ 2022-02-15 21:20 UTC (permalink / raw)
  To: Atish Patra
  Cc: qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng,
	Atish Patra, Palmer Dabbelt, open list:RISC-V

Am Dienstag, 15. Februar 2022, 20:39:10 CET schrieb Atish Patra:
> On Tue, Feb 15, 2022 at 8:20 AM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Am Dienstag, 15. Februar 2022, 10:05:30 CET schrieb Atish Patra:
> > > Append the available ISA extensions to the "riscv,isa" string if it
> > > is enabled so that kernel can process it.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >  target/riscv/cpu.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index b0a40b83e7a8..c70260d0df15 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -34,6 +34,9 @@
> > >
> > >  /* RISC-V CPU definitions */
> > >
> > > +/* This includes the null terminated character '\0' */
> > > +#define MAX_ISA_EXT_LEN 256
> > > +
> > >  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> > >
> > >  const char * const riscv_int_regnames[] = {
> > > @@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> > >      device_class_set_props(dc, riscv_cpu_properties);
> > >  }
> > >
> > > +static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int max_str_len)
> > > +{
> > > +    int offset = strnlen(isa_str, max_str_len);
> > > +
> > > +    if (cpu->cfg.ext_svpbmt) {
> > > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svpbmt");
> > > +    }
> > > +    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
> >
> > shouldn't offset + strlen("svinval") +1 be < max_str_len?
> > snprintf will write partial strings but this would throw off a
> > qemu client completely I guess.
> >
> 
> We need that check to put out warnings to the user.

That is what I meant :-)

The current check above is for "there is still space for _something_"
but it doesn't check if the whole extension name fits.

That is what I'm trying to do in my example below check (and warn if not)
if the whole name fits into the remaining space.

> 
> >
> > > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svinval");
> > > +    }
> > > +    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
> > > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", "_svnapot");
> > > +    }
> >
> > wouldn't it make more sense to do something like:
> >
> > +    struct {
> > +        const char *value;
> > +        bool enabled;
> > +    } extensions[] = {
> > +        { "svpbmt", cpu->cfg.ext_svpbmt },
> > +        { "svinval", cpu->cfg.ext_svinval },
> > +        { "svnapot", cpu->cfg.ext_svnapot },
> > +    };
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(extensions); i++) {
> > +        if (!extensions[i].enabled)
> > +            continue;
> > +
> > +        /* check available space */
> > +        if (offset + strlen(extensions[i].value) + 1 > max_str_len) {
> > +            //do warn about exceeded length
> > +            return;
> > +        }
> > +
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s",
> > +                                                          extensions[i].value);
> > +    }
> >
> > instead?
> >
> > Because that list will get longer over time and repeating checks
> > and snprintf calls will get harder to keep in sync over time?
> >
> 
> Yeah. This is much better.
> 
> >
> > Heiko
> >
> >
> >
> >
> > > +}
> > > +
> > >  char *riscv_isa_string(RISCVCPU *cpu)
> > >  {
> > >      int i;
> > > -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> > > +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
> > > +                          MAX_ISA_EXT_LEN;
> > >      char *isa_str = g_new(char, maxlen);
> > >      char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
> > >      for (i = 0; i < sizeof(riscv_exts); i++) {
> > > @@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
> > >          }
> > >      }
> > >      *p = '\0';
> > > +    riscv_isa_string_ext(cpu, isa_str, maxlen);
> > > +
> > >      return isa_str;
> > >  }
> > >
> > >
> >
> >
> >
> >
> >
> 
> 
> 






^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-02-15 21:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-15  9:05 [PATCH] target/riscv: Add isa extenstion strings to the device tree Atish Patra
2022-02-15  9:05 ` Atish Patra
2022-02-15  9:58 ` Heiko Stübner
2022-02-15 16:20 ` Heiko Stübner
2022-02-15 19:39   ` Atish Patra
2022-02-15 19:39     ` Atish Patra
2022-02-15 21:20     ` Heiko Stübner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.