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é <address@hidden>
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 <address@hidden>
> > Cc: Peter Maydell <address@hidden>
> > Cc: Philippe Mathieu-Daudé <address@hidden>
> > Signed-off-by: Michael Clark <address@hidden>
> > ---
> >  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.

address@hidden 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
address@hidden riscv-qemu]$ ./riscv64-linux-user/qemu-riscv64 hello.riscv64
Hello World
address@hidden riscv-qemu]$ ./x86_64-linux-user/qemu-x86_64 hello.x86_64
Hello World

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