[Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Michael Clark]

Logic bug caused the string size calculation for the RISC-V
format ISA string to be small. This fix allows slack for rv128.

Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4851890..1456535 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
 char *riscv_isa_string(RISCVCPU *cpu)
 {
     int i;
-    size_t maxlen = 5 + ctz32(cpu->env.misa);
+    size_t maxlen = 8 + ctpop64(cpu->env.misa);
     char *isa_string = g_new0(char, maxlen);
     snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
     for (i = 0; i < sizeof(riscv_exts); i++) {
-- 
2.7.0

Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Philippe Mathieu-Daudé]

On 03/09/2018 10:01 PM, Michael Clark wrote:
> Logic bug caused the string size calculation for the RISC-V
> format ISA string to be small. This fix allows slack for rv128.
> 
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Michael Clark <mjc@sifive.com>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4851890..1456535 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
> +    size_t maxlen = 8 + ctpop64(cpu->env.misa);

Can you document the magic 5/8?

This looks nice, but this seems to me too much optimization to save few
bytes, using sizeof(riscv_exts) is overflow-free.

Maybe this is enough and self-explanatory:

       const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts);

>      char *isa_string = g_new0(char, maxlen);
>      snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);

Also, if you keep the snprintf() return value, you can (naming it 'n')
simplify (also easier to review):

>      for (i = 0; i < sizeof(riscv_exts); i++) {
> 
        if (cpu->env.misa & RV(riscv_exts[i])) {
-           isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
+           isa_string[n++] = tolower(riscv_exts[i]);
        }
    }

and simply use g_new() with:

+   isa_string[n] = '\0';

    return isa_string;
}

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Philippe Mathieu-Daudé]

On 03/10/2018 10:25 PM, Philippe Mathieu-Daudé wrote:
> On 03/09/2018 10:01 PM, Michael Clark wrote:
>> Logic bug caused the string size calculation for the RISC-V
>> format ISA string to be small. This fix allows slack for rv128.
>>
>> Cc: Palmer Dabbelt <palmer@sifive.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Michael Clark <mjc@sifive.com>
>> ---
>>  target/riscv/cpu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 4851890..1456535 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
>>  char *riscv_isa_string(RISCVCPU *cpu)
>>  {
>>      int i;
>> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
>> +    size_t maxlen = 8 + ctpop64(cpu->env.misa);
> 
> Can you document the magic 5/8?
> 
> This looks nice, but this seems to me too much optimization to save few
> bytes, using sizeof(riscv_exts) is overflow-free.
> 
> Maybe this is enough and self-explanatory:
> 
>        const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts);

Oops:
         const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;

> 
>>      char *isa_string = g_new0(char, maxlen);
>>      snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
> 
> Also, if you keep the snprintf() return value, you can (naming it 'n')
> simplify (also easier to review):
> 
>>      for (i = 0; i < sizeof(riscv_exts); i++) {
>>
>         if (cpu->env.misa & RV(riscv_exts[i])) {
> -           isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> +           isa_string[n++] = tolower(riscv_exts[i]);
>         }
>     }
> 
> and simply use g_new() with:
> 
> +   isa_string[n] = '\0';
> 
>     return isa_string;
> }
> 
> Regards,
> 
> Phil.
> 

Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Peter Maydell]

On 10 March 2018 at 21:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 03/09/2018 10:01 PM, Michael Clark wrote:
>> Logic bug caused the string size calculation for the RISC-V
>> format ISA string to be small. This fix allows slack for rv128.
>>
>> Cc: Palmer Dabbelt <palmer@sifive.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Michael Clark <mjc@sifive.com>
>> ---
>>  target/riscv/cpu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 4851890..1456535 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
>>  char *riscv_isa_string(RISCVCPU *cpu)
>>  {
>>      int i;
>> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
>> +    size_t maxlen = 8 + ctpop64(cpu->env.misa);
>
> Can you document the magic 5/8?
>
> This looks nice, but this seems to me too much optimization to save few
> bytes, using sizeof(riscv_exts) is overflow-free.
>
> Maybe this is enough and self-explanatory:
>
>        const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts);
>
>>      char *isa_string = g_new0(char, maxlen);
>>      snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
>
> Also, if you keep the snprintf() return value, you can (naming it 'n')
> simplify (also easier to review):
>
>>      for (i = 0; i < sizeof(riscv_exts); i++) {
>>
>         if (cpu->env.misa & RV(riscv_exts[i])) {
> -           isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> +           isa_string[n++] = tolower(riscv_exts[i]);
>         }
>     }
>
> and simply use g_new() with:
>
> +   isa_string[n] = '\0';
>
>     return isa_string;
> }

Hi -- any chance of a respin of this patch that addresses Philippe's
review comments so we can fix it before rc0? This is causing
my merge-build tests to fail about 50% of the time on OpenBSD
at the moment...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Michael Clark]

On Thu, Mar 15, 2018 at 12:27 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 March 2018 at 21:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > On 03/09/2018 10:01 PM, Michael Clark wrote:
> >> Logic bug caused the string size calculation for the RISC-V
> >> format ISA string to be small. This fix allows slack for rv128.
> >>
> >> Cc: Palmer Dabbelt <palmer@sifive.com>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Michael Clark <mjc@sifive.com>
> >> ---
> >>  target/riscv/cpu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 4851890..1456535 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
> >>  char *riscv_isa_string(RISCVCPU *cpu)
> >>  {
> >>      int i;
> >> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
> >> +    size_t maxlen = 8 + ctpop64(cpu->env.misa);
> >
> > Can you document the magic 5/8?
> >
> > This looks nice, but this seems to me too much optimization to save few
> > bytes, using sizeof(riscv_exts) is overflow-free.
> >
> > Maybe this is enough and self-explanatory:
> >
> >        const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts);
> >
> >>      char *isa_string = g_new0(char, maxlen);
> >>      snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
> >
> > Also, if you keep the snprintf() return value, you can (naming it 'n')
> > simplify (also easier to review):
> >
> >>      for (i = 0; i < sizeof(riscv_exts); i++) {
> >>
> >         if (cpu->env.misa & RV(riscv_exts[i])) {
> > -           isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> > +           isa_string[n++] = tolower(riscv_exts[i]);
> >         }
> >     }
> >
> > and simply use g_new() with:
> >
> > +   isa_string[n] = '\0';
> >
> >     return isa_string;
> > }
>
> Hi -- any chance of a respin of this patch that addresses Philippe's
> review comments so we can fix it before rc0? This is causing
> my merge-build tests to fail about 50% of the time on OpenBSD
> at the moment...


I'll respin asap.

I was out earlier this week as I was at the RISC-V Hackathon at the
Embedded Linux Conference in Portland. I also have to go through the review
backlog for the post merge spec conformance and cleanup series...

Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Michael Clark]

On Fri, Mar 16, 2018 at 10:03 AM, Michael Clark <mjc@sifive.com> wrote:

>
> On Thu, Mar 15, 2018 at 12:27 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On 10 March 2018 at 21:25, Philippe Mathieu-Daudé <f4bug@amsat.org>
>> wrote:
>> > On 03/09/2018 10:01 PM, Michael Clark wrote:
>> >> Logic bug caused the string size calculation for the RISC-V
>> >> format ISA string to be small. This fix allows slack for rv128.
>> >>
>> >> Cc: Palmer Dabbelt <palmer@sifive.com>
>> >> Cc: Peter Maydell <peter.maydell@linaro.org>
>> >> Cc: Eric Blake <eblake@redhat.com>
>> >> Signed-off-by: Michael Clark <mjc@sifive.com>
>> >> ---
>> >>  target/riscv/cpu.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> >> index 4851890..1456535 100644
>> >> --- a/target/riscv/cpu.c
>> >> +++ b/target/riscv/cpu.c
>> >> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
>> >>  char *riscv_isa_string(RISCVCPU *cpu)
>> >>  {
>> >>      int i;
>> >> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
>> >> +    size_t maxlen = 8 + ctpop64(cpu->env.misa);
>> >
>> > Can you document the magic 5/8?
>> >
>> > This looks nice, but this seems to me too much optimization to save few
>> > bytes, using sizeof(riscv_exts) is overflow-free.
>> >
>> > Maybe this is enough and self-explanatory:
>> >
>> >        const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts);
>> >
>> >>      char *isa_string = g_new0(char, maxlen);
>> >>      snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
>> >
>> > Also, if you keep the snprintf() return value, you can (naming it 'n')
>> > simplify (also easier to review):
>> >
>> >>      for (i = 0; i < sizeof(riscv_exts); i++) {
>> >>
>> >         if (cpu->env.misa & RV(riscv_exts[i])) {
>> > -           isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
>> > +           isa_string[n++] = tolower(riscv_exts[i]);
>> >         }
>> >     }
>> >
>> > and simply use g_new() with:
>> >
>> > +   isa_string[n] = '\0';
>> >
>> >     return isa_string;
>> > }
>>
>> Hi -- any chance of a respin of this patch that addresses Philippe's
>> review comments so we can fix it before rc0? This is causing
>> my merge-build tests to fail about 50% of the time on OpenBSD
>> at the moment...
>
>
> I'll respin asap.
>
> I was out earlier this week as I was at the RISC-V Hackathon at the
> Embedded Linux Conference in Portland. I also have to go through the review
> backlog for the post merge spec conformance and cleanup series...
>

I've sent out the riscv_isa_string fix patch as well as the RISC-V
Post-merge spec conformance and cleanup series independently. I could
combine them so we have one post-merge PR that contains the riscv_isa_string
string fix, the spec conformance fixes and cleanup. Many of the cleanup
patches have been reviewed by Phil, but the patches that haven't been
reviewed are spec conformance fixes and likely require a reading of the
RISC-V Privileged ISA Specification, which may not happen, unless anyone
with RISC-V knowledge has time. They have sign off.

If I submit a PR it will include the checkpatch fixes that I noticed after
sending the patches. The series has been tested pretty extensively and the
earlier revision is what we have in the riscv.org repo and in SiFive's
freedom-u-sdk. i.e. it is what folk are using. I've been working inside of
QEMU RISC-V quite a lot with this series applied, running the latest Fedora
image with SMP Linux, building QEMU inside of QEMU as I'm working on a TCG
backend for RISC-V that I started this Monday during the RISC-V Hackathon.

FYI: This is the work in progress branch on the TCG backend for RISC-V
(it's not quite complete but the bones are there):

- https://github.com/michaeljclark/riscv-qemu/tree/riscv-tcg-backend