[Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

This version uses a constant size memory buffer sized for
the maximum possible ISA string length. It also uses g_new
instead of g_new0, uses more efficient logic to append
extensions and adds manual zero termination of the string.

Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 target/riscv/cpu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4851890..298abbd 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -391,16 +391,16 @@ static const TypeInfo riscv_cpu_type_info = {
 char *riscv_isa_string(RISCVCPU *cpu)
 {
     int i;
-    size_t maxlen = 5 + ctz32(cpu->env.misa);
-    char *isa_string = g_new0(char, maxlen);
-    snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
+    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
+    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++) {
         if (cpu->env.misa & RV(riscv_exts[i])) {
-            isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
-
+            *p++ = riscv_exts[i] - 'A' + 'a';
         }
     }
-    return isa_string;
+    *p = '\0';
+    return isa_str;
 }
 
 void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Philippe Mathieu-Daudé]

On 03/16/2018 06:26 PM, Michael Clark wrote:
> This version uses a constant size memory buffer sized for
> the maximum possible ISA string length. It also uses g_new
> instead of g_new0, uses more efficient logic to append
> extensions and adds manual zero termination of the string.
> 
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Michael Clark <mjc@sifive.com>
> ---
>  target/riscv/cpu.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4851890..298abbd 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -391,16 +391,16 @@ static const TypeInfo riscv_cpu_type_info = {
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
> -    char *isa_string = g_new0(char, maxlen);
> -    snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
> +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> +    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++) {
>          if (cpu->env.misa & RV(riscv_exts[i])) {
> -            isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> -
> +            *p++ = riscv_exts[i] - 'A' + 'a';
                                                 /* tolower() */

I prefer unobfuscated code (think newcomers and reviewers) :|

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>          }
>      }
> -    return isa_string;
> +    *p = '\0';
> +    return isa_str;
>  }
>  
>  void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> 

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

On Sun, Mar 18, 2018 at 4:22 PM, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 03/16/2018 06:26 PM, Michael Clark wrote:
> > This version uses a constant size memory buffer sized for
> > the maximum possible ISA string length. It also uses g_new
> > instead of g_new0, uses more efficient logic to append
> > extensions and adds manual zero termination of the string.
> >
> > Cc: Palmer Dabbelt <palmer@sifive.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Michael Clark <mjc@sifive.com>
> > ---
> >  target/riscv/cpu.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 4851890..298abbd 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -391,16 +391,16 @@ static const TypeInfo riscv_cpu_type_info = {
> >  char *riscv_isa_string(RISCVCPU *cpu)
> >  {
> >      int i;
> > -    size_t maxlen = 5 + ctz32(cpu->env.misa);
> > -    char *isa_string = g_new0(char, maxlen);
> > -    snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
> > +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> > +    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++) {
> >          if (cpu->env.misa & RV(riscv_exts[i])) {
> > -            isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> > -
> > +            *p++ = riscv_exts[i] - 'A' + 'a';
>                                                  /* tolower() */
>
> I prefer unobfuscated code (think newcomers and reviewers) :|
>

Thanks! Done.

I think i'll add the riscv_isa_string fix onto the RISC-V post-merge spec
conformance and cleanup series, and send I'll send out a (hopefully final)
version of it today, so we can merge all of the spec conformance, bug fixes
and cleanups in one PR.

The RISC-V post-merge spec conformance and cleanup series has had a lot of
testing. I've been using it to compile QEMU inside of QEMU using the RISC-V
Fedora Image and its native RISC-V GCC toolchain running inside SMP Linux
4.16-rc2. It appears to be pretty rock-solid. The rcu lock fix would likely
only affect users who are ballooning memory while a guest is under load.
The page walker changes have also been tested under load (including
performance tests).

FYI - I also have an experimental branch containing a RISC-V TCG back-end
that I started on during the RISC-V Hackathon in Portland last week:

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

I'm able to run a very simple x86_64 hello world program on RISC-V,
avoiding all of the usual libc startup. However, it may be some time before
I submit a patch series for this branch. So far the RISC-V TCG backend
doesn't implement softmmu, have bigendian byteswapping in its load/store
implementation or 64-bit on 32-bit support (brcond2/setcond2). This will
come, however, at present I'm still testing the basic codegen. That being
said, it has meant that i've been using the RISC-V QEMU Fedora image pretty
extensively as my main development machine, with the  RISC-V post-merge
spec conformance and cleanup series applied, performing a lot of parallel
makes on a quad-core i7. The only thing I am really missing is gdb. The
current RISC-V GDB port seems to be missing user-space debugging support as
I believe the focus has been on the JTAG remote debugging use case.

[mclark@stage4 riscv-qemu]$ uname -a
Linux stage4.fedoraproject.org 4.16.0-rc2-00328-gebea62367bc4 #4 SMP Mon
Feb 26 16:05:16 GMT 2018 riscv64 riscv64 riscv64 GNU/Linux
[mclark@stage4 riscv-qemu]$ ./riscv64-linux-user/qemu-riscv64 hello.riscv64
Hello World
[mclark@stage4 riscv-qemu]$ ./x86_64-linux-user/qemu-x86_64 hello.x86_64
Hello World

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> >          }
> >      }
> > -    return isa_string;
> > +    *p = '\0';
> > +    return isa_str;
> >  }
> >
> >  void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> >
>

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Richard W.M. Jones]

On Mon, Mar 19, 2018 at 11:35:51AM -0700, Michael Clark wrote:
> The RISC-V post-merge spec conformance and cleanup series has had a lot of
> testing. I've been using it to compile QEMU inside of QEMU using the RISC-V
> Fedora Image and its native RISC-V GCC toolchain running inside SMP Linux
> 4.16-rc2. It appears to be pretty rock-solid. The rcu lock fix would likely
> only affect users who are ballooning memory while a guest is under load.
> The page walker changes have also been tested under load (including
> performance tests).

Did you see the problem with restoring floating point registers on
context switch?  The test case is quite simple:

  http://oirase.annexia.org/tmp/sched.c

Note you must compile it with gcc -O2 to manifest the bug.  (We
originally thought it was a problem with gcc's optimization, but it
isn't.)

In Fedora's qemu tree are carrying the following patch which is just a
workaround:

  https://github.com/rwmjones/fedora-riscv-bootstrap/blob/master/stage1-riscv-qemu/force-float-save.patch

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

On Mon, Mar 19, 2018 at 12:42 PM, Richard W.M. Jones <rjones@redhat.com>
wrote:

> On Mon, Mar 19, 2018 at 11:35:51AM -0700, Michael Clark wrote:
> > The RISC-V post-merge spec conformance and cleanup series has had a lot
> of
> > testing. I've been using it to compile QEMU inside of QEMU using the
> RISC-V
> > Fedora Image and its native RISC-V GCC toolchain running inside SMP Linux
> > 4.16-rc2. It appears to be pretty rock-solid. The rcu lock fix would
> likely
> > only affect users who are ballooning memory while a guest is under load.
> > The page walker changes have also been tested under load (including
> > performance tests).
>
> Did you see the problem with restoring floating point registers on
> context switch?  The test case is quite simple:
>
>   http://oirase.annexia.org/tmp/sched.c


No I didn't. Thanks.


> Note you must compile it with gcc -O2 to manifest the bug.  (We
> originally thought it was a problem with gcc's optimization, but it
> isn't.)
>

Okay. I must admit most of my testing has been integer heavy workloads.


> In Fedora's qemu tree are carrying the following patch which is just a
> workaround:
>
>   https://github.com/rwmjones/fedora-riscv-bootstrap/blob/
> master/stage1-riscv-qemu/force-float-save.patch


I will take a look...

There were some changes around v4 of our port upstream patch series to add
MSTATUS.FS (TB_FLAGS_FP_ENABLE) to to tb->flags to avoid checking the flags
on all FP ops.

static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong
*pc,
                                        target_ulong *cs_base, uint32_t
*flags)
{
    *pc = env->pc;
    *cs_base = 0;
#ifdef CONFIG_USER_ONLY
    *flags = TB_FLAGS_FP_ENABLE;
#else
    *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
#endif
}

I'll have to get my head around the implication of this or with the
privilege level and how MSTATUS.FS is currently set...

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Richard Henderson]

On 03/20/2018 02:35 AM, Michael Clark wrote:
> FYI - I also have an experimental branch containing a RISC-V TCG back-end
> that I started on during the RISC-V Hackathon in Portland last week:
> 
> - https://github.com/michaeljclark/riscv-qemu/tree/wip-riscv-tcg-backend

Cool.

> I'm able to run a very simple x86_64 hello world program on RISC-V,
> avoiding all of the usual libc startup. However, it may be some time before
> I submit a patch series for this branch. So far the RISC-V TCG backend
> doesn't implement softmmu, have bigendian byteswapping in its load/store
> implementation ...

Before you work too hard on this last, ping me.  I've been intending to change
TCG such that hosts that do not support byte-swapping memory operations do not
need to handle them specially.  Instead, we'd expand a "normal" byte swap from
a temporary after a load (and similarly before a store).

I expect this to improve the state of affairs for i386 (without movbe), where
we are currently extremely limited in the available registers.


r~

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

On Mon, Mar 19, 2018 at 9:43 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 03/20/2018 02:35 AM, Michael Clark wrote:
> > FYI - I also have an experimental branch containing a RISC-V TCG back-end
> > that I started on during the RISC-V Hackathon in Portland last week:
> >
> > - https://github.com/michaeljclark/riscv-qemu/tree/wip-riscv-tcg-backend
>
> Cool.
>
> > I'm able to run a very simple x86_64 hello world program on RISC-V,
> > avoiding all of the usual libc startup. However, it may be some time
> before
> > I submit a patch series for this branch. So far the RISC-V TCG backend
> > doesn't implement softmmu, have bigendian byteswapping in its load/store
> > implementation ...
>
> Before you work too hard on this last, ping me.  I've been intending to
> change
> TCG such that hosts that do not support byte-swapping memory operations do
> not
> need to handle them specially.  Instead, we'd expand a "normal" byte swap
> from
> a temporary after a load (and similarly before a store).
>

Okay. I'll leave the byte swapping out for now. The first priority is to
test the basic codegen with little endian front-ends e.g. riscv -> riscv
and x86 -> riscv, and the second priority is to implement softmmu. I still
have some bugs as its pretty new code, but as I mentioned, the bones are
there. i.e. we have relocs and instruction encoders for riscv.


> I expect this to improve the state of affairs for i386 (without movbe),
> where
> we are currently extremely limited in the available registers.
>

Okay, so this would move byte swapping into TCG generic code instead of the
TCG backend, unless the backend explicitly supports load/store with byte
swap?

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

On Mon, Mar 19, 2018 at 2:39 PM, Michael Clark <mjc@sifive.com> wrote:

>
>
> On Mon, Mar 19, 2018 at 12:42 PM, Richard W.M. Jones <rjones@redhat.com>
> wrote:
>
>> On Mon, Mar 19, 2018 at 11:35:51AM -0700, Michael Clark wrote:
>> > The RISC-V post-merge spec conformance and cleanup series has had a lot
>> of
>> > testing. I've been using it to compile QEMU inside of QEMU using the
>> RISC-V
>> > Fedora Image and its native RISC-V GCC toolchain running inside SMP
>> Linux
>> > 4.16-rc2. It appears to be pretty rock-solid. The rcu lock fix would
>> likely
>> > only affect users who are ballooning memory while a guest is under load.
>> > The page walker changes have also been tested under load (including
>> > performance tests).
>>
>> Did you see the problem with restoring floating point registers on
>> context switch?  The test case is quite simple:
>>
>>   http://oirase.annexia.org/tmp/sched.c
>
>
> No I didn't. Thanks.
>
>
>> Note you must compile it with gcc -O2 to manifest the bug.  (We
>> originally thought it was a problem with gcc's optimization, but it
>> isn't.)
>>
>
> Okay. I must admit most of my testing has been integer heavy workloads.
>
>
>> In Fedora's qemu tree are carrying the following patch which is just a
>> workaround:
>>
>>   https://github.com/rwmjones/fedora-riscv-bootstrap/blob/mast
>> er/stage1-riscv-qemu/force-float-save.patch
>
>
> I will take a look...
>

>From my local testing, it appears to only show up with SMP enabled. With 1
CPU enabled the sched.c test passes.


> There were some changes around v4 of our port upstream patch series to add
> MSTATUS.FS (TB_FLAGS_FP_ENABLE) to to tb->flags to avoid checking the flags
> on all FP ops.
>

I'm curious about the change in v4 of our upstream patch series "Enforce
MSTATUS_FS via TB flags"

-
https://github.com/riscv/riscv-qemu/commit/1b0631573008d19b2cb4bb1ef388bd51d6a1d722

I'll could try and revert this and see if it makes any difference...
however I think the float code has changed substantially with respect to
handling of flags. We could backport the more conservative approach of
handling MSTATUS_FS in the float routines. We should also run the test case
on hardware to rule out linux-kernel MSTATIS_FS bugs...

BTW We didn't have MTTCG enabled until v6 of our patch series so given it
only shows up on SMP, it wouldn't have been visible until v6 or after.

static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong
> *pc,
>                                         target_ulong *cs_base, uint32_t
> *flags)
> {
>     *pc = env->pc;
>     *cs_base = 0;
> #ifdef CONFIG_USER_ONLY
>     *flags = TB_FLAGS_FP_ENABLE;
> #else
>     *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
> #endif
> }
>
> I'll have to get my head around the implication of this or with the
> privilege level and how MSTATUS.FS is currently set...
>

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Richard W.M. Jones]

On Thu, Mar 22, 2018 at 02:06:24PM -0700, Michael Clark wrote:
> On Mon, Mar 19, 2018 at 2:39 PM, Michael Clark <mjc@sifive.com> wrote:
> > On Mon, Mar 19, 2018 at 12:42 PM, Richard W.M. Jones <rjones@redhat.com>
> > wrote:
> >> Did you see the problem with restoring floating point registers on
> >> context switch?  The test case is quite simple:
> >>
> >>   http://oirase.annexia.org/tmp/sched.c
> >
> > No I didn't. Thanks.
> From my local testing, it appears to only show up with SMP enabled. With 1
> CPU enabled the sched.c test passes.
> I'll could try and revert this and see if it makes any difference...
> however I think the float code has changed substantially with respect to
> handling of flags. We could backport the more conservative approach of
> handling MSTATUS_FS in the float routines. We should also run the test case
> on hardware to rule out linux-kernel MSTATUS_FS bugs...

DJ, am I remembering correctly that you tried the test case on the
HiFive evaluation board and it didn't demonstrate the bug?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[DJ Delorie]

"Richard W.M. Jones" <rjones@redhat.com> writes:
> DJ, am I remembering correctly that you tried the test case on the
> HiFive evaluation board and it didn't demonstrate the bug?

I tested it on the vc707 board, without seeing the bug.

I can test other test cases if needed, I've got the board running Fedora
at the moment.

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

On Thu, Mar 22, 2018 at 3:17 PM, DJ Delorie <dj@redhat.com> wrote:

> "Richard W.M. Jones" <rjones@redhat.com> writes:
> > DJ, am I remembering correctly that you tried the test case on the
> > HiFive evaluation board and it didn't demonstrate the bug?
>
> I tested it on the vc707 board, without seeing the bug.
>
> I can test other test cases if needed, I've got the board running Fedora
> at the moment.
>

Okay, I'll assume its a QEMU RISC-V bug.

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Palmer Dabbelt]

On Thu, 22 Mar 2018 15:17:16 PDT (-0700), dj@redhat.com wrote:
> "Richard W.M. Jones" <rjones@redhat.com> writes:
>> DJ, am I remembering correctly that you tried the test case on the
>> HiFive evaluation board and it didn't demonstrate the bug?
>
> I tested it on the vc707 board, without seeing the bug.

FWIW, the cores in the bitstream for the vc707 are almost identical to the
FU540-C000.

> I can test other test cases if needed, I've got the board running Fedora
> at the moment.

Thanks!

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Richard Henderson]

On 03/21/2018 02:06 AM, Michael Clark wrote:
> Okay, so this would move byte swapping into TCG generic code instead of the TCG
> backend, unless the backend explicitly supports load/store with byte swap? 

Yes.


r~