[Qemu-devel] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Qemu-devel] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Georg Kotheimer]

The size of the FPU registers depends solely on the floating point
extensions supported by the target architecture.
However, in the previous implementation the floating point register
size was derived from whether the target architecture is 32-bit or
64-bit.

To allow RVF without RVD, changes to riscv_gdb_get_fpu() and
riscv_gdb_set_fpu() were necessary.

In addition fflags, frm and fcsr were removed from
riscv-XXbit-csr.xml, as the floating point csr registers are only
available, if a FPU is present.

Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>
---
 configure                                     |  4 +-
 gdb-xml/riscv-32bit-csr.xml                   |  3 --
 gdb-xml/riscv-64bit-csr.xml                   |  3 --
 .../{riscv-64bit-fpu.xml => riscv-fpu-d.xml}  |  0
 .../{riscv-32bit-fpu.xml => riscv-fpu-f.xml}  |  0
 target/riscv/gdbstub.c                        | 50 ++++++++++---------
 6 files changed, 29 insertions(+), 31 deletions(-)
 rename gdb-xml/{riscv-64bit-fpu.xml => riscv-fpu-d.xml} (100%)
 rename gdb-xml/{riscv-32bit-fpu.xml => riscv-fpu-f.xml} (100%)

diff --git a/configure b/configure
index 714e7fb6a1..f9e2320586 100755
--- a/configure
+++ b/configure
@@ -7596,14 +7596,14 @@ case "$target_name" in
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
-    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
+    gdb_xml_files="riscv-32bit-cpu.xml riscv-fpu-f.xml riscv-fpu-d.xml riscv-32bit-csr.xml"
     target_compiler=$cross_cc_riscv32
   ;;
   riscv64)
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
-    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
+    gdb_xml_files="riscv-64bit-cpu.xml riscv-fpu-f.xml riscv-fpu-d.xml riscv-64bit-csr.xml"
     target_compiler=$cross_cc_riscv64
   ;;
   sh4|sh4eb)
diff --git a/gdb-xml/riscv-32bit-csr.xml b/gdb-xml/riscv-32bit-csr.xml
index da1bf19e2f..cb36f7050a 100644
--- a/gdb-xml/riscv-32bit-csr.xml
+++ b/gdb-xml/riscv-32bit-csr.xml
@@ -15,9 +15,6 @@
   <reg name="ucause" bitsize="32"/>
   <reg name="utval" bitsize="32"/>
   <reg name="uip" bitsize="32"/>
-  <reg name="fflags" bitsize="32"/>
-  <reg name="frm" bitsize="32"/>
-  <reg name="fcsr" bitsize="32"/>
   <reg name="cycle" bitsize="32"/>
   <reg name="time" bitsize="32"/>
   <reg name="instret" bitsize="32"/>
diff --git a/gdb-xml/riscv-64bit-csr.xml b/gdb-xml/riscv-64bit-csr.xml
index 6aa4bed9f5..34ffe1e426 100644
--- a/gdb-xml/riscv-64bit-csr.xml
+++ b/gdb-xml/riscv-64bit-csr.xml
@@ -15,9 +15,6 @@
   <reg name="ucause" bitsize="64"/>
   <reg name="utval" bitsize="64"/>
   <reg name="uip" bitsize="64"/>
-  <reg name="fflags" bitsize="64"/>
-  <reg name="frm" bitsize="64"/>
-  <reg name="fcsr" bitsize="64"/>
   <reg name="cycle" bitsize="64"/>
   <reg name="time" bitsize="64"/>
   <reg name="instret" bitsize="64"/>
diff --git a/gdb-xml/riscv-64bit-fpu.xml b/gdb-xml/riscv-fpu-d.xml
similarity index 100%
rename from gdb-xml/riscv-64bit-fpu.xml
rename to gdb-xml/riscv-fpu-d.xml
diff --git a/gdb-xml/riscv-32bit-fpu.xml b/gdb-xml/riscv-fpu-f.xml
similarity index 100%
rename from gdb-xml/riscv-32bit-fpu.xml
rename to gdb-xml/riscv-fpu-f.xml
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 27be93279b..c6e530fdd2 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -35,9 +35,6 @@ static int csr_register_map[] = {
     CSR_UCAUSE,
     CSR_UTVAL,
     CSR_UIP,
-    CSR_FFLAGS,
-    CSR_FRM,
-    CSR_FCSR,
     CSR_CYCLE,
     CSR_TIME,
     CSR_INSTRET,
@@ -303,19 +300,22 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 static int riscv_gdb_get_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
-        return gdb_get_reg64(mem_buf, env->fpr[n]);
+        if (env->misa & RVD) {
+            return gdb_get_reg64(mem_buf, env->fpr[n]);
+        }
+        return gdb_get_reg32(mem_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 8 in csr_register, and gdb says it is FP
-         * register 33, so we recalculate the map index.
+         * CSR_FFLAGS is at index 1 in the csr space, and gdb says it is FP
+         * register 33, so we recalculate the csr index.
          * This also works for CSR_FRM and CSR_FCSR.
          */
-        result = riscv_csrrw_debug(env, n - 33 +  8, &val, 0, 0);
+        result = riscv_csrrw_debug(env, n - 33 + CSR_FFLAGS, &val, 0, 0);
         if (result == 0) {
-            return gdb_get_regl(mem_buf, val);
+            return gdb_get_reg32(mem_buf, val);
         }
     }
     return 0;
@@ -324,20 +324,25 @@ static int riscv_gdb_get_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
 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);
+        if (env->misa & RVD) {
+            env->fpr[n] = ldq_p(mem_buf);
+            return sizeof(uint64_t);
+        } else {
+            env->fpr[n] = ldl_p(mem_buf);
+            return sizeof(uint32_t);
+        }
     /* there is hole between ft11 and fflags in fpu.xml */
     } else if (n < 36 && n > 32) {
-        target_ulong val = ldtul_p(mem_buf);
+        target_ulong val = ldl_p(mem_buf);
         int result;
         /*
-         * CSR_FFLAGS is at index 8 in csr_register, and gdb says it is FP
-         * register 33, so we recalculate the map index.
+         * CSR_FFLAGS is at index 1 in the csr space, and gdb says it is FP
+         * register 33, so we recalculate the csr index.
          * This also works for CSR_FRM and CSR_FCSR.
          */
-        result = riscv_csrrw_debug(env, n - 33 + 8, NULL, val, -1);
+        result = riscv_csrrw_debug(env, n - 33 + CSR_FFLAGS, NULL, val, -1);
         if (result == 0) {
-            return sizeof(target_ulong);
+            return sizeof(uint32_t);
         }
     }
     return 0;
@@ -375,20 +380,19 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
     CPURISCVState *env = &cpu->env;
-#if defined(TARGET_RISCV32)
-    if (env->misa & RVF) {
+
+    if (env->misa & RVD) {
+        gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
+                                 36, "riscv-fpu-d.xml", 0);
+    } else if (env->misa & RVF) {
         gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
-                                 36, "riscv-32bit-fpu.xml", 0);
+                                 36, "riscv-fpu-f.xml", 0);
     }
 
+#if defined(TARGET_RISCV32)
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
                              4096, "riscv-32bit-csr.xml", 0);
 #elif defined(TARGET_RISCV64)
-    if (env->misa & RVF) {
-        gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
-                                 36, "riscv-64bit-fpu.xml", 0);
-    }
-
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
                              4096, "riscv-64bit-csr.xml", 0);
 #endif
-- 
2.20.1

[Qemu-riscv] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Georg Kotheimer]

The size of the FPU registers depends solely on the floating point
extensions supported by the target architecture.
However, in the previous implementation the floating point register
size was derived from whether the target architecture is 32-bit or
64-bit.

To allow RVF without RVD, changes to riscv_gdb_get_fpu() and
riscv_gdb_set_fpu() were necessary.

In addition fflags, frm and fcsr were removed from
riscv-XXbit-csr.xml, as the floating point csr registers are only
available, if a FPU is present.

Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>
---
 configure                                     |  4 +-
 gdb-xml/riscv-32bit-csr.xml                   |  3 --
 gdb-xml/riscv-64bit-csr.xml                   |  3 --
 .../{riscv-64bit-fpu.xml => riscv-fpu-d.xml}  |  0
 .../{riscv-32bit-fpu.xml => riscv-fpu-f.xml}  |  0
 target/riscv/gdbstub.c                        | 50 ++++++++++---------
 6 files changed, 29 insertions(+), 31 deletions(-)
 rename gdb-xml/{riscv-64bit-fpu.xml => riscv-fpu-d.xml} (100%)
 rename gdb-xml/{riscv-32bit-fpu.xml => riscv-fpu-f.xml} (100%)

diff --git a/configure b/configure
index 714e7fb6a1..f9e2320586 100755
--- a/configure
+++ b/configure
@@ -7596,14 +7596,14 @@ case "$target_name" in
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
-    gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
+    gdb_xml_files="riscv-32bit-cpu.xml riscv-fpu-f.xml riscv-fpu-d.xml riscv-32bit-csr.xml"
     target_compiler=$cross_cc_riscv32
   ;;
   riscv64)
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
-    gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
+    gdb_xml_files="riscv-64bit-cpu.xml riscv-fpu-f.xml riscv-fpu-d.xml riscv-64bit-csr.xml"
     target_compiler=$cross_cc_riscv64
   ;;
   sh4|sh4eb)
diff --git a/gdb-xml/riscv-32bit-csr.xml b/gdb-xml/riscv-32bit-csr.xml
index da1bf19e2f..cb36f7050a 100644
--- a/gdb-xml/riscv-32bit-csr.xml
+++ b/gdb-xml/riscv-32bit-csr.xml
@@ -15,9 +15,6 @@
   <reg name="ucause" bitsize="32"/>
   <reg name="utval" bitsize="32"/>
   <reg name="uip" bitsize="32"/>
-  <reg name="fflags" bitsize="32"/>
-  <reg name="frm" bitsize="32"/>
-  <reg name="fcsr" bitsize="32"/>
   <reg name="cycle" bitsize="32"/>
   <reg name="time" bitsize="32"/>
   <reg name="instret" bitsize="32"/>
diff --git a/gdb-xml/riscv-64bit-csr.xml b/gdb-xml/riscv-64bit-csr.xml
index 6aa4bed9f5..34ffe1e426 100644
--- a/gdb-xml/riscv-64bit-csr.xml
+++ b/gdb-xml/riscv-64bit-csr.xml
@@ -15,9 +15,6 @@
   <reg name="ucause" bitsize="64"/>
   <reg name="utval" bitsize="64"/>
   <reg name="uip" bitsize="64"/>
-  <reg name="fflags" bitsize="64"/>
-  <reg name="frm" bitsize="64"/>
-  <reg name="fcsr" bitsize="64"/>
   <reg name="cycle" bitsize="64"/>
   <reg name="time" bitsize="64"/>
   <reg name="instret" bitsize="64"/>
diff --git a/gdb-xml/riscv-64bit-fpu.xml b/gdb-xml/riscv-fpu-d.xml
similarity index 100%
rename from gdb-xml/riscv-64bit-fpu.xml
rename to gdb-xml/riscv-fpu-d.xml
diff --git a/gdb-xml/riscv-32bit-fpu.xml b/gdb-xml/riscv-fpu-f.xml
similarity index 100%
rename from gdb-xml/riscv-32bit-fpu.xml
rename to gdb-xml/riscv-fpu-f.xml
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 27be93279b..c6e530fdd2 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -35,9 +35,6 @@ static int csr_register_map[] = {
     CSR_UCAUSE,
     CSR_UTVAL,
     CSR_UIP,
-    CSR_FFLAGS,
-    CSR_FRM,
-    CSR_FCSR,
     CSR_CYCLE,
     CSR_TIME,
     CSR_INSTRET,
@@ -303,19 +300,22 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 static int riscv_gdb_get_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
-        return gdb_get_reg64(mem_buf, env->fpr[n]);
+        if (env->misa & RVD) {
+            return gdb_get_reg64(mem_buf, env->fpr[n]);
+        }
+        return gdb_get_reg32(mem_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 8 in csr_register, and gdb says it is FP
-         * register 33, so we recalculate the map index.
+         * CSR_FFLAGS is at index 1 in the csr space, and gdb says it is FP
+         * register 33, so we recalculate the csr index.
          * This also works for CSR_FRM and CSR_FCSR.
          */
-        result = riscv_csrrw_debug(env, n - 33 +  8, &val, 0, 0);
+        result = riscv_csrrw_debug(env, n - 33 + CSR_FFLAGS, &val, 0, 0);
         if (result == 0) {
-            return gdb_get_regl(mem_buf, val);
+            return gdb_get_reg32(mem_buf, val);
         }
     }
     return 0;
@@ -324,20 +324,25 @@ static int riscv_gdb_get_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
 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);
+        if (env->misa & RVD) {
+            env->fpr[n] = ldq_p(mem_buf);
+            return sizeof(uint64_t);
+        } else {
+            env->fpr[n] = ldl_p(mem_buf);
+            return sizeof(uint32_t);
+        }
     /* there is hole between ft11 and fflags in fpu.xml */
     } else if (n < 36 && n > 32) {
-        target_ulong val = ldtul_p(mem_buf);
+        target_ulong val = ldl_p(mem_buf);
         int result;
         /*
-         * CSR_FFLAGS is at index 8 in csr_register, and gdb says it is FP
-         * register 33, so we recalculate the map index.
+         * CSR_FFLAGS is at index 1 in the csr space, and gdb says it is FP
+         * register 33, so we recalculate the csr index.
          * This also works for CSR_FRM and CSR_FCSR.
          */
-        result = riscv_csrrw_debug(env, n - 33 + 8, NULL, val, -1);
+        result = riscv_csrrw_debug(env, n - 33 + CSR_FFLAGS, NULL, val, -1);
         if (result == 0) {
-            return sizeof(target_ulong);
+            return sizeof(uint32_t);
         }
     }
     return 0;
@@ -375,20 +380,19 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
     CPURISCVState *env = &cpu->env;
-#if defined(TARGET_RISCV32)
-    if (env->misa & RVF) {
+
+    if (env->misa & RVD) {
+        gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
+                                 36, "riscv-fpu-d.xml", 0);
+    } else if (env->misa & RVF) {
         gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
-                                 36, "riscv-32bit-fpu.xml", 0);
+                                 36, "riscv-fpu-f.xml", 0);
     }
 
+#if defined(TARGET_RISCV32)
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
                              4096, "riscv-32bit-csr.xml", 0);
 #elif defined(TARGET_RISCV64)
-    if (env->misa & RVF) {
-        gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
-                                 36, "riscv-64bit-fpu.xml", 0);
-    }
-
     gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
                              4096, "riscv-64bit-csr.xml", 0);
 #endif
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Jim Wilson]

On 8/20/19 7:39 AM, Georg Kotheimer wrote:
> The size of the FPU registers depends solely on the floating point
> extensions supported by the target architecture.
> However, in the previous implementation the floating point register
> size was derived from whether the target architecture is 32-bit or
> 64-bit.
> 
> To allow RVF without RVD, changes to riscv_gdb_get_fpu() and
> riscv_gdb_set_fpu() were necessary.
> 
> In addition fflags, frm and fcsr were removed from
> riscv-XXbit-csr.xml, as the floating point csr registers are only
> available, if a FPU is present.

The current XML files were identical to the XML files in gdb when 
implemented.  This seems to be existing practice, as this is true of all 
of the other targets I looked at when I implemented this.  Also, the 
file names are the same with a / replaced with a -.  These files are in 
the gdb/features/riscv dir in a gdb source tree.  It would be a shame to 
break this.  I'm not sure if they are still identical.  The gdb copies 
might have been updated since then, and may need to be copied into qemu 
to update qemu, but we don't have a dedicated gdb/qemu maintainer to do 
this.

I don't see a need to remove the fp csr's from the csr list.  There are 
other extension dependent CSRs, like hypervisor ones. I think it makes 
more sense for the csr list to contain all of the csrs, and then disable 
access to them if that extension is not enabled.  If there is a good 
reason to require changes to the csr XML files, then it probably should 
be discussed with the gdb developers too, so that the gdb and qemu 
copies of the files remain consistent.

Fixing the rvf/rvd 32/64-bit support is good.  That is a bug in my 
original implementation.

Jim

Re: [Qemu-riscv] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Jim Wilson]

On 8/20/19 7:39 AM, Georg Kotheimer wrote:
> The size of the FPU registers depends solely on the floating point
> extensions supported by the target architecture.
> However, in the previous implementation the floating point register
> size was derived from whether the target architecture is 32-bit or
> 64-bit.
> 
> To allow RVF without RVD, changes to riscv_gdb_get_fpu() and
> riscv_gdb_set_fpu() were necessary.
> 
> In addition fflags, frm and fcsr were removed from
> riscv-XXbit-csr.xml, as the floating point csr registers are only
> available, if a FPU is present.

The current XML files were identical to the XML files in gdb when 
implemented.  This seems to be existing practice, as this is true of all 
of the other targets I looked at when I implemented this.  Also, the 
file names are the same with a / replaced with a -.  These files are in 
the gdb/features/riscv dir in a gdb source tree.  It would be a shame to 
break this.  I'm not sure if they are still identical.  The gdb copies 
might have been updated since then, and may need to be copied into qemu 
to update qemu, but we don't have a dedicated gdb/qemu maintainer to do 
this.

I don't see a need to remove the fp csr's from the csr list.  There are 
other extension dependent CSRs, like hypervisor ones. I think it makes 
more sense for the csr list to contain all of the csrs, and then disable 
access to them if that extension is not enabled.  If there is a good 
reason to require changes to the csr XML files, then it probably should 
be discussed with the gdb developers too, so that the gdb and qemu 
copies of the files remain consistent.

Fixing the rvf/rvd 32/64-bit support is good.  That is a bug in my 
original implementation.

Jim

Re: [Qemu-devel] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Richard Henderson]

On 8/20/19 1:06 PM, Jim Wilson wrote:
> I don't see a need to remove the fp csr's from the csr list.  There are other
> extension dependent CSRs, like hypervisor ones. I think it makes more sense for
> the csr list to contain all of the csrs, and then disable access to them if
> that extension is not enabled.  If there is a good reason to require changes to
> the csr XML files, then it probably should be discussed with the gdb developers
> too, so that the gdb and qemu copies of the files remain consistent.

Another possibility is to generate the list of csr's at runtime and export that
to the connecting gdb.

We do this in target/arm/gdbstub.c, arm_gen_dynamic_xml.


r~

Re: [Qemu-riscv] [Qemu-devel] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Richard Henderson]

On 8/20/19 1:06 PM, Jim Wilson wrote:
> I don't see a need to remove the fp csr's from the csr list.  There are other
> extension dependent CSRs, like hypervisor ones. I think it makes more sense for
> the csr list to contain all of the csrs, and then disable access to them if
> that extension is not enabled.  If there is a good reason to require changes to
> the csr XML files, then it probably should be discussed with the gdb developers
> too, so that the gdb and qemu copies of the files remain consistent.

Another possibility is to generate the list of csr's at runtime and export that
to the connecting gdb.

We do this in target/arm/gdbstub.c, arm_gen_dynamic_xml.


r~

Re: [Qemu-devel] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Georg Kotheimer]

On Tuesday, 20 August 2019 22:06:51 CEST Jim Wilson wrote:
> The current XML files were identical to the XML files in gdb when
> implemented.  This seems to be existing practice, as this is true of all
> of the other targets I looked at when I implemented this.  Also, the
> file names are the same with a / replaced with a -.  These files are in
> the gdb/features/riscv dir in a gdb source tree.  It would be a shame to
> break this.  I'm not sure if they are still identical.  The gdb copies
> might have been updated since then, and may need to be copied into qemu
> to update qemu, but we don't have a dedicated gdb/qemu maintainer to do
> this.
> 
> I don't see a need to remove the fp csr's from the csr list.  There are
> other extension dependent CSRs, like hypervisor ones. I think it makes
> more sense for the csr list to contain all of the csrs, and then disable
> access to them if that extension is not enabled.  If there is a good
> reason to require changes to the csr XML files, then it probably should
> be discussed with the gdb developers too, so that the gdb and qemu
> copies of the files remain consistent.
> 
> Fixing the rvf/rvd 32/64-bit support is good.  That is a bug in my
> original implementation.
> 
> Jim

My motivation behind renaming the xml files was to work against the 
misconception that e.g. 64bit-cpu and 64bit-fpu are related to one another. 
But of course, 32bit-fpu and 64bit-fpu is technically not incorrect.

Regarding the removal of the fp csrs from the csr list: I found it confusing 
that the fp csr registers were listed in two files. In addition the bitsize of 
the fp csr registers was stated as 64 in riscv-64bit-csr.xml, wheras the other 
xml files (32bit-csr, 32bit-fpu and 64bit-fpu) stated 32, in accordance with 
the RISC-V ISA specification.
Then I had a look at the gdb source code, and came to the conclusion that gdb 
does not use the fp csr registers from the org.gnu.gdb.riscv.csr feaure set, 
but instead the ones from org.gnu.gdb.riscv.fpu. Therefore, I decided to 
remove the fp csr registers from the csr list.
However, as I don't have any prior experience with QEMU or gdb development is 
quite likely that I misinterpreted or overlooked something.

For now I prepared a third version of the patch that only fixes the rvf/rvd 
32/64-bit support.

Georg

Re: [Qemu-riscv] [PATCH v2] RISC-V: Select FPU gdb xml file based on the supported extensions

[Georg Kotheimer]

On Tuesday, 20 August 2019 22:06:51 CEST Jim Wilson wrote:
> The current XML files were identical to the XML files in gdb when
> implemented.  This seems to be existing practice, as this is true of all
> of the other targets I looked at when I implemented this.  Also, the
> file names are the same with a / replaced with a -.  These files are in
> the gdb/features/riscv dir in a gdb source tree.  It would be a shame to
> break this.  I'm not sure if they are still identical.  The gdb copies
> might have been updated since then, and may need to be copied into qemu
> to update qemu, but we don't have a dedicated gdb/qemu maintainer to do
> this.
> 
> I don't see a need to remove the fp csr's from the csr list.  There are
> other extension dependent CSRs, like hypervisor ones. I think it makes
> more sense for the csr list to contain all of the csrs, and then disable
> access to them if that extension is not enabled.  If there is a good
> reason to require changes to the csr XML files, then it probably should
> be discussed with the gdb developers too, so that the gdb and qemu
> copies of the files remain consistent.
> 
> Fixing the rvf/rvd 32/64-bit support is good.  That is a bug in my
> original implementation.
> 
> Jim

My motivation behind renaming the xml files was to work against the 
misconception that e.g. 64bit-cpu and 64bit-fpu are related to one another. 
But of course, 32bit-fpu and 64bit-fpu is technically not incorrect.

Regarding the removal of the fp csrs from the csr list: I found it confusing 
that the fp csr registers were listed in two files. In addition the bitsize of 
the fp csr registers was stated as 64 in riscv-64bit-csr.xml, wheras the other 
xml files (32bit-csr, 32bit-fpu and 64bit-fpu) stated 32, in accordance with 
the RISC-V ISA specification.
Then I had a look at the gdb source code, and came to the conclusion that gdb 
does not use the fp csr registers from the org.gnu.gdb.riscv.csr feaure set, 
but instead the ones from org.gnu.gdb.riscv.fpu. Therefore, I decided to 
remove the fp csr registers from the csr list.
However, as I don't have any prior experience with QEMU or gdb development is 
quite likely that I misinterpreted or overlooked something.

For now I prepared a third version of the patch that only fixes the rvf/rvd 
32/64-bit support.

Georg