Re: [PATCH] target/riscv: correct "code should not be reached" for x-rv128

[Frédéric Pétrot]

Le 24/01/2022 à 09:47, LIU Zhiwei a écrit :
> On 2022/1/24 下午3:49, Frédéric Pétrot wrote:
> > The addition of uxl support in gdbstub adds a few checks on the maximum
> > register length, but omitted MXL_RV128, leading to the occurence of
> > "code should not be reached" in a few places.
> > This patch makes rv128 react as rv64 for gdb, as previously.
>
> If that is case for rv128, you should also add
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1cb0436187..5ada71e5bf 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -528,9 +528,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>       switch (env->misa_mxl_max) {
>   #ifdef TARGET_RISCV64
>       case MXL_RV64:
> -        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> -        break;
>       case MXL_RV128:
> +        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>           break;

  Thanks Zhiwei for pointing that out, I resend a patch with that too.
  Strangely enough, I didn't bump into that case.

> Still I don't know why we should make rv128 react as rv64 for gdb?

  Well, I should surely do what is necessary to have a working 128-bit
  connection to gdb, but it has a bit of influence on other stuff than qemu,
  e.g. when I configure the gdb-xml file with bitsize 128, gdb tests the
  xlens and returns "bfd requires xlen 8, but target has xlen 16", and
  after that no register can be queried.
  Gdb checks what is called "arch features" and knows only about ELFCLASS32
  and ELFCLASS64, so we are stuck for now.

  Thanks,
  Frédéric

> Thanks,
> Zhiwei
>
> > Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> > ---
> >   target/riscv/gdbstub.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index f531a74c2f..9ed049c29e 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -64,6 +64,7 @@ int riscv_cpu_gdb_read_register(CPUState *cs, GByteArray 
> > *mem_buf, int n)
> >       case MXL_RV32:
> >           return gdb_get_reg32(mem_buf, tmp);
> >       case MXL_RV64:
> > +    case MXL_RV128:
> >           return gdb_get_reg64(mem_buf, tmp);
> >       default:
> >           g_assert_not_reached();
> > @@ -84,6 +85,7 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t 
> > *mem_buf, int n)
> >           length = 4;
> >           break;
> >       case MXL_RV64:
> > +    case MXL_RV128:
> >           if (env->xl < MXL_RV64) {
> >               tmp = (int32_t)ldq_p(mem_buf);
> >           } else {
> > @@ -420,6 +422,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> >                                    1, "riscv-32bit-virtual.xml", 0);
> >           break;
> >       case MXL_RV64:
> > +    case MXL_RV128:
> >           gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
> >                                    riscv_gdb_set_virtual,
> >                                    1, "riscv-64bit-virtual.xml", 0);

--
+---------------------------------------------------------------------------+
| Frédéric Pétrot, Pr. Grenoble INP-Ensimag/TIMA,   Ensimag deputy director |
| Mob/Pho: +33 6 74 57 99 65/+33 4 76 57 48 70      Ad augusta  per angusta |
| http://tima.univ-grenoble-alpes.fr frederic.petrot@univ-grenoble-alpes.fr |
+---------------------------------------------------------------------------+