Re: [PATCH 1/2] target/riscv: remove fflags, frm, and fcsr from riscv-*-fpu.xml

[Alistair Francis]

On Wed, Aug 31, 2022 at 10:43 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> While testing some changes to GDB's handling for the RISC-V registers
> fcsr, fflags, and frm, I spotted that QEMU includes these registers
> twice in the target description it sends to GDB, once in the fpu
> feature, and once in the csr feature.
>
> Right now things basically work OK, QEMU maps these registers onto two
> different register numbers, e.g. fcsr maps to both 68 and 73, and GDB
> can use either of these to access the register.
>
> However, GDB's target descriptions don't really work this way, each
> register should appear just once in a target description, mapping the
> register name onto the number GDB should use when accessing the
> register on the target.  Duplicate register names actually result in
> duplicate registers on the GDB side, however, as the registers have
> the same name, the user can only access one of these registers.
>
> Currently GDB has a hack in place, specifically for RISC-V, to spot
> the duplicate copies of these three registers, and hide them from the
> user, ensuring the user only ever sees a single copy of each.
>
> In this commit I propose fixing this issue on the QEMU side, and in
> the process, simplify the fpu register handling a little.
>
> I think we should, remove fflags, frm, and fcsr from the two (32-bit
> and 64-bit) fpu feature xml files.  These files will only contain the
> 32 core floating point register f0 to f31.  The fflags, frm, and fcsr
> registers will continue to be advertised in the csr feature as they
> currently are.
>
> With that change made, I will simplify riscv_gdb_get_fpu and
> riscv_gdb_set_fpu, removing the extra handling for the 3 status
> registers.
>
> Signed-off-by: Andrew Burgess <aburgess@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  gdb-xml/riscv-32bit-fpu.xml |  4 ----
>  gdb-xml/riscv-64bit-fpu.xml |  4 ----
>  target/riscv/gdbstub.c      | 32 ++------------------------------
>  3 files changed, 2 insertions(+), 38 deletions(-)
>
> diff --git a/gdb-xml/riscv-32bit-fpu.xml b/gdb-xml/riscv-32bit-fpu.xml
> index 1eaae9119e..84a44ba8df 100644
> --- a/gdb-xml/riscv-32bit-fpu.xml
> +++ b/gdb-xml/riscv-32bit-fpu.xml
> @@ -43,8 +43,4 @@
>    <reg name="ft9" bitsize="32" type="ieee_single"/>
>    <reg name="ft10" bitsize="32" type="ieee_single"/>
>    <reg name="ft11" bitsize="32" type="ieee_single"/>
> -
> -  <reg name="fflags" bitsize="32" type="int" regnum="66"/>
> -  <reg name="frm" bitsize="32" type="int" regnum="67"/>
> -  <reg name="fcsr" bitsize="32" type="int" regnum="68"/>
>  </feature>
> diff --git a/gdb-xml/riscv-64bit-fpu.xml b/gdb-xml/riscv-64bit-fpu.xml
> index 794854cc01..9856a9d1d3 100644
> --- a/gdb-xml/riscv-64bit-fpu.xml
> +++ b/gdb-xml/riscv-64bit-fpu.xml
> @@ -49,8 +49,4 @@
>    <reg name="ft9" bitsize="64" type="riscv_double"/>
>    <reg name="ft10" bitsize="64" type="riscv_double"/>
>    <reg name="ft11" bitsize="64" type="riscv_double"/>
> -
> -  <reg name="fflags" bitsize="32" type="int" regnum="66"/>
> -  <reg name="frm" bitsize="32" type="int" regnum="67"/>
> -  <reg name="fcsr" bitsize="32" type="int" regnum="68"/>
>  </feature>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 9ed049c29e..9974b7aac6 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -114,20 +114,6 @@ static int riscv_gdb_get_fpu(CPURISCVState *env, 
> GByteArray *buf, int n)
>          if (env->misa_ext & RVF) {
>              return gdb_get_reg32(buf, env->fpr[n]);
>          }
> -    /* there is hole between ft11 and fflags in fpu.xml */
> -    } else if (n < 36 && n > 32) {
> -        target_ulong val = 0;
> -        int result;
> -        /*
> -         * CSR_FFLAGS is at index 1 in csr_register, and gdb says it is FP
> -         * register 33, so we recalculate the map index.
> -         * This also works for CSR_FRM and CSR_FCSR.
> -         */
> -        result = riscv_csrrw_debug(env, n - 32, &val,
> -                                   0, 0);
> -        if (result == RISCV_EXCP_NONE) {
> -            return gdb_get_regl(buf, val);
> -        }
>      }
>      return 0;
>  }
> @@ -137,20 +123,6 @@ static int riscv_gdb_set_fpu(CPURISCVState *env, uint8_t 
> *mem_buf, int n)
>      if (n < 32) {
>          env->fpr[n] = ldq_p(mem_buf); /* always 64-bit */
>          return sizeof(uint64_t);
> -    /* there is hole between ft11 and fflags in fpu.xml */
> -    } else if (n < 36 && n > 32) {
> -        target_ulong val = ldtul_p(mem_buf);
> -        int result;
> -        /*
> -         * CSR_FFLAGS is at index 1 in csr_register, and gdb says it is FP
> -         * register 33, so we recalculate the map index.
> -         * This also works for CSR_FRM and CSR_FCSR.
> -         */
> -        result = riscv_csrrw_debug(env, n - 32, NULL,
> -                                   val, -1);
> -        if (result == RISCV_EXCP_NONE) {
> -            return sizeof(target_ulong);
> -        }
>      }
>      return 0;
>  }
> @@ -404,10 +376,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState 
> *cs)
>      CPURISCVState *env = &cpu->env;
>      if (env->misa_ext & RVD) {
>          gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> -                                 36, "riscv-64bit-fpu.xml", 0);
> +                                 32, "riscv-64bit-fpu.xml", 0);
>      } else if (env->misa_ext & RVF) {
>          gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> -                                 36, "riscv-32bit-fpu.xml", 0);
> +                                 32, "riscv-32bit-fpu.xml", 0);
>      }
>      if (env->misa_ext & RVV) {
>          gdb_register_coprocessor(cs, riscv_gdb_get_vector, 
> riscv_gdb_set_vector,
> --
> 2.25.4
>
>