Re: [PATCH 3/4] target/riscv: Generate the GDB XML file for CSR registers dynamically
On Sat, Jan 16, 2021 at 6:08 AM Alistair Francis wrote: > > On Fri, Jan 15, 2021 at 1:59 PM Alistair Francis wrote: > > > > On Mon, Jan 11, 2021 at 8:55 PM Bin Meng wrote: > > > > > > From: Bin Meng > > > > > > At present QEMU RISC-V uses a hardcoded XML to report the feature > > > "org.gnu.gdb.riscv.csr" [1]. There are two major issues with the > > > approach being used currently: > > > > > > - The XML does not specify the "regnum" field of a CSR entry, hence > > > consecutive numbers are used by the remote GDB client to access > > > CSRs. In QEMU we have to maintain a map table to convert the GDB > > > number to the hardware number which is error prone. > > > - The XML contains some CSRs that QEMU does not implement at all, > > > which causes an "E14" response sent to remote GDB client. > > > > > > Change to generate the CSR register list dynamically, based on the > > > availability presented in the CSR function table. This new approach > > > will reflect a correct list of CSRs that QEMU actually implements. > > > > > > [1] > > > https://sourceware.org/gdb/current/onlinedocs/gdb/RISC_002dV-Features.html#RISC_002dV-Features > > Do you mind rebasing this patch on the current riscv-to-apply.next > branch: https://github.com/alistair23/qemu/tree/riscv-to-apply.next > Yes, for sure. Regards, Bin
Re: [PATCH 3/4] target/riscv: Generate the GDB XML file for CSR registers dynamically
On Fri, Jan 15, 2021 at 1:59 PM Alistair Francis wrote:
>
> On Mon, Jan 11, 2021 at 8:55 PM Bin Meng wrote:
> >
> > From: Bin Meng
> >
> > At present QEMU RISC-V uses a hardcoded XML to report the feature
> > "org.gnu.gdb.riscv.csr" [1]. There are two major issues with the
> > approach being used currently:
> >
> > - The XML does not specify the "regnum" field of a CSR entry, hence
> > consecutive numbers are used by the remote GDB client to access
> > CSRs. In QEMU we have to maintain a map table to convert the GDB
> > number to the hardware number which is error prone.
> > - The XML contains some CSRs that QEMU does not implement at all,
> > which causes an "E14" response sent to remote GDB client.
> >
> > Change to generate the CSR register list dynamically, based on the
> > availability presented in the CSR function table. This new approach
> > will reflect a correct list of CSRs that QEMU actually implements.
> >
> > [1]
> > https://sourceware.org/gdb/current/onlinedocs/gdb/RISC_002dV-Features.html#RISC_002dV-Features
Do you mind rebasing this patch on the current riscv-to-apply.next
branch: https://github.com/alistair23/qemu/tree/riscv-to-apply.next
Alistair
> >
> > Signed-off-by: Bin Meng
>
> Reviewed-by: Alistair Francis
>
> Alistair
>
> > ---
> >
> > target/riscv/cpu.h | 2 +
> > target/riscv/cpu.c | 12 ++
> > target/riscv/gdbstub.c | 308
> > +++--
> > 3 files changed, 58 insertions(+), 264 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 6684316..f810169 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -272,6 +272,8 @@ struct RISCVCPU {
> > CPUNegativeOffsetState neg;
> > CPURISCVState env;
> >
> > +char *dyn_csr_xml;
> > +
> > /* Configuration Settings */
> > struct {
> > bool ext_i;
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index dfe5d4e..c0dd646 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -569,6 +569,17 @@ static gchar *riscv_gdb_arch_name(CPUState *cs)
> > }
> > }
> >
> > +static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char
> > *xmlname)
> > +{
> > +RISCVCPU *cpu = RISCV_CPU(cs);
> > +
> > +if (strcmp(xmlname, "riscv-csr.xml") == 0) {
> > +return cpu->dyn_csr_xml;
> > +}
> > +
> > +return NULL;
> > +}
> > +
> > static void riscv_cpu_class_init(ObjectClass *c, void *data)
> > {
> > RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> > @@ -607,6 +618,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void
> > *data)
> > cc->write_elf32_note = riscv_cpu_write_elf32_note;
> > #endif
> > cc->gdb_arch_name = riscv_gdb_arch_name;
> > +cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
> > #ifdef CONFIG_TCG
> > cc->tcg_initialize = riscv_translate_init;
> > cc->tlb_fill = riscv_cpu_tlb_fill;
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index eba12a8..5f96b7e 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -20,256 +20,6 @@
> > #include "exec/gdbstub.h"
> > #include "cpu.h"
> >
> > -/*
> > - * The GDB CSR xml files list them in documentation order, not numerical
> > order,
> > - * and are missing entries for unnamed CSRs. So we need to map the gdb
> > numbers
> > - * to the hardware numbers.
> > - */
> > -
> > -static int csr_register_map[] = {
> > -CSR_USTATUS,
> > -CSR_UIE,
> > -CSR_UTVEC,
> > -CSR_USCRATCH,
> > -CSR_UEPC,
> > -CSR_UCAUSE,
> > -CSR_UTVAL,
> > -CSR_UIP,
> > -CSR_FFLAGS,
> > -CSR_FRM,
> > -CSR_FCSR,
> > -CSR_CYCLE,
> > -CSR_TIME,
> > -CSR_INSTRET,
> > -CSR_HPMCOUNTER3,
> > -CSR_HPMCOUNTER4,
> > -CSR_HPMCOUNTER5,
> > -CSR_HPMCOUNTER6,
> > -CSR_HPMCOUNTER7,
> > -CSR_HPMCOUNTER8,
> > -CSR_HPMCOUNTER9,
> > -CSR_HPMCOUNTER10,
> > -CSR_HPMCOUNTER11,
> > -CSR_HPMCOUNTER12,
> > -CSR_HPMCOUNTER13,
> > -CSR_HPMCOUNTER14,
> > -CSR_HPMCOUNTER15,
> > -CSR_HPMCOUNTER16,
> > -CSR_HPMCOUNTER17,
> > -CSR_HPMCOUNTER18,
> > -CSR_HPMCOUNTER19,
> > -CSR_HPMCOUNTER20,
> > -CSR_HPMCOUNTER21,
> > -CSR_HPMCOUNTER22,
> > -CSR_HPMCOUNTER23,
> > -CSR_HPMCOUNTER24,
> > -CSR_HPMCOUNTER25,
> > -CSR_HPMCOUNTER26,
> > -CSR_HPMCOUNTER27,
> > -CSR_HPMCOUNTER28,
> > -CSR_HPMCOUNTER29,
> > -CSR_HPMCOUNTER30,
> > -CSR_HPMCOUNTER31,
> > -CSR_CYCLEH,
> > -CSR_TIMEH,
> > -CSR_INSTRETH,
> > -CSR_HPMCOUNTER3H,
> > -CSR_HPMCOUNTER4H,
> > -CSR_HPMCOUNTER5H,
> > -CSR_HPMCOUNTER6H,
> > -CSR_HPMCOUNTER7H,
> > -CSR_HPMCOUNTER8H,
> > -CSR_HPMCOUNTER9H,
> > -CSR_HPMCOUNTER10H,
> > -CSR_HPMCOUNTER11H,
> > -CSR_HPMCOUNTER12H,
> > -CSR_HPMCOUNTER13H,
> > -CSR_HPMCOUNTER14H,
> > -CSR_HPMCOUNTER15H,
> > -CSR_HPMCOUNTER16H,
> > -CSR_HPMCOUNTER17H,
> > -
Re: [PATCH 3/4] target/riscv: Generate the GDB XML file for CSR registers dynamically
On Mon, Jan 11, 2021 at 8:55 PM Bin Meng wrote:
>
> From: Bin Meng
>
> At present QEMU RISC-V uses a hardcoded XML to report the feature
> "org.gnu.gdb.riscv.csr" [1]. There are two major issues with the
> approach being used currently:
>
> - The XML does not specify the "regnum" field of a CSR entry, hence
> consecutive numbers are used by the remote GDB client to access
> CSRs. In QEMU we have to maintain a map table to convert the GDB
> number to the hardware number which is error prone.
> - The XML contains some CSRs that QEMU does not implement at all,
> which causes an "E14" response sent to remote GDB client.
>
> Change to generate the CSR register list dynamically, based on the
> availability presented in the CSR function table. This new approach
> will reflect a correct list of CSRs that QEMU actually implements.
>
> [1]
> https://sourceware.org/gdb/current/onlinedocs/gdb/RISC_002dV-Features.html#RISC_002dV-Features
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
> ---
>
> target/riscv/cpu.h | 2 +
> target/riscv/cpu.c | 12 ++
> target/riscv/gdbstub.c | 308
> +++--
> 3 files changed, 58 insertions(+), 264 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6684316..f810169 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -272,6 +272,8 @@ struct RISCVCPU {
> CPUNegativeOffsetState neg;
> CPURISCVState env;
>
> +char *dyn_csr_xml;
> +
> /* Configuration Settings */
> struct {
> bool ext_i;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index dfe5d4e..c0dd646 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -569,6 +569,17 @@ static gchar *riscv_gdb_arch_name(CPUState *cs)
> }
> }
>
> +static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char
> *xmlname)
> +{
> +RISCVCPU *cpu = RISCV_CPU(cs);
> +
> +if (strcmp(xmlname, "riscv-csr.xml") == 0) {
> +return cpu->dyn_csr_xml;
> +}
> +
> +return NULL;
> +}
> +
> static void riscv_cpu_class_init(ObjectClass *c, void *data)
> {
> RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -607,6 +618,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void
> *data)
> cc->write_elf32_note = riscv_cpu_write_elf32_note;
> #endif
> cc->gdb_arch_name = riscv_gdb_arch_name;
> +cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
> #ifdef CONFIG_TCG
> cc->tcg_initialize = riscv_translate_init;
> cc->tlb_fill = riscv_cpu_tlb_fill;
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index eba12a8..5f96b7e 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -20,256 +20,6 @@
> #include "exec/gdbstub.h"
> #include "cpu.h"
>
> -/*
> - * The GDB CSR xml files list them in documentation order, not numerical
> order,
> - * and are missing entries for unnamed CSRs. So we need to map the gdb
> numbers
> - * to the hardware numbers.
> - */
> -
> -static int csr_register_map[] = {
> -CSR_USTATUS,
> -CSR_UIE,
> -CSR_UTVEC,
> -CSR_USCRATCH,
> -CSR_UEPC,
> -CSR_UCAUSE,
> -CSR_UTVAL,
> -CSR_UIP,
> -CSR_FFLAGS,
> -CSR_FRM,
> -CSR_FCSR,
> -CSR_CYCLE,
> -CSR_TIME,
> -CSR_INSTRET,
> -CSR_HPMCOUNTER3,
> -CSR_HPMCOUNTER4,
> -CSR_HPMCOUNTER5,
> -CSR_HPMCOUNTER6,
> -CSR_HPMCOUNTER7,
> -CSR_HPMCOUNTER8,
> -CSR_HPMCOUNTER9,
> -CSR_HPMCOUNTER10,
> -CSR_HPMCOUNTER11,
> -CSR_HPMCOUNTER12,
> -CSR_HPMCOUNTER13,
> -CSR_HPMCOUNTER14,
> -CSR_HPMCOUNTER15,
> -CSR_HPMCOUNTER16,
> -CSR_HPMCOUNTER17,
> -CSR_HPMCOUNTER18,
> -CSR_HPMCOUNTER19,
> -CSR_HPMCOUNTER20,
> -CSR_HPMCOUNTER21,
> -CSR_HPMCOUNTER22,
> -CSR_HPMCOUNTER23,
> -CSR_HPMCOUNTER24,
> -CSR_HPMCOUNTER25,
> -CSR_HPMCOUNTER26,
> -CSR_HPMCOUNTER27,
> -CSR_HPMCOUNTER28,
> -CSR_HPMCOUNTER29,
> -CSR_HPMCOUNTER30,
> -CSR_HPMCOUNTER31,
> -CSR_CYCLEH,
> -CSR_TIMEH,
> -CSR_INSTRETH,
> -CSR_HPMCOUNTER3H,
> -CSR_HPMCOUNTER4H,
> -CSR_HPMCOUNTER5H,
> -CSR_HPMCOUNTER6H,
> -CSR_HPMCOUNTER7H,
> -CSR_HPMCOUNTER8H,
> -CSR_HPMCOUNTER9H,
> -CSR_HPMCOUNTER10H,
> -CSR_HPMCOUNTER11H,
> -CSR_HPMCOUNTER12H,
> -CSR_HPMCOUNTER13H,
> -CSR_HPMCOUNTER14H,
> -CSR_HPMCOUNTER15H,
> -CSR_HPMCOUNTER16H,
> -CSR_HPMCOUNTER17H,
> -CSR_HPMCOUNTER18H,
> -CSR_HPMCOUNTER19H,
> -CSR_HPMCOUNTER20H,
> -CSR_HPMCOUNTER21H,
> -CSR_HPMCOUNTER22H,
> -CSR_HPMCOUNTER23H,
> -CSR_HPMCOUNTER24H,
> -CSR_HPMCOUNTER25H,
> -CSR_HPMCOUNTER26H,
> -CSR_HPMCOUNTER27H,
> -CSR_HPMCOUNTER28H,
> -CSR_HPMCOUNTER29H,
> -CSR_HPMCOUNTER30H,
> -CSR_HPMCOUNTER31H,
> -CSR_SSTATUS,
> -CSR_SEDELEG,
> -CSR_SIDELEG,
> -CSR_SIE,
> -CSR_STVEC,
> -CSR_SCOUNTEREN,
> -CSR_SSCRATCH,
> -CSR_SEPC,
> -
[PATCH 3/4] target/riscv: Generate the GDB XML file for CSR registers dynamically
From: Bin Meng
At present QEMU RISC-V uses a hardcoded XML to report the feature
"org.gnu.gdb.riscv.csr" [1]. There are two major issues with the
approach being used currently:
- The XML does not specify the "regnum" field of a CSR entry, hence
consecutive numbers are used by the remote GDB client to access
CSRs. In QEMU we have to maintain a map table to convert the GDB
number to the hardware number which is error prone.
- The XML contains some CSRs that QEMU does not implement at all,
which causes an "E14" response sent to remote GDB client.
Change to generate the CSR register list dynamically, based on the
availability presented in the CSR function table. This new approach
will reflect a correct list of CSRs that QEMU actually implements.
[1]
https://sourceware.org/gdb/current/onlinedocs/gdb/RISC_002dV-Features.html#RISC_002dV-Features
Signed-off-by: Bin Meng
---
target/riscv/cpu.h | 2 +
target/riscv/cpu.c | 12 ++
target/riscv/gdbstub.c | 308 +++--
3 files changed, 58 insertions(+), 264 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6684316..f810169 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -272,6 +272,8 @@ struct RISCVCPU {
CPUNegativeOffsetState neg;
CPURISCVState env;
+char *dyn_csr_xml;
+
/* Configuration Settings */
struct {
bool ext_i;
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index dfe5d4e..c0dd646 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -569,6 +569,17 @@ static gchar *riscv_gdb_arch_name(CPUState *cs)
}
}
+static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
+{
+RISCVCPU *cpu = RISCV_CPU(cs);
+
+if (strcmp(xmlname, "riscv-csr.xml") == 0) {
+return cpu->dyn_csr_xml;
+}
+
+return NULL;
+}
+
static void riscv_cpu_class_init(ObjectClass *c, void *data)
{
RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -607,6 +618,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
cc->write_elf32_note = riscv_cpu_write_elf32_note;
#endif
cc->gdb_arch_name = riscv_gdb_arch_name;
+cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
#ifdef CONFIG_TCG
cc->tcg_initialize = riscv_translate_init;
cc->tlb_fill = riscv_cpu_tlb_fill;
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index eba12a8..5f96b7e 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -20,256 +20,6 @@
#include "exec/gdbstub.h"
#include "cpu.h"
-/*
- * The GDB CSR xml files list them in documentation order, not numerical order,
- * and are missing entries for unnamed CSRs. So we need to map the gdb numbers
- * to the hardware numbers.
- */
-
-static int csr_register_map[] = {
-CSR_USTATUS,
-CSR_UIE,
-CSR_UTVEC,
-CSR_USCRATCH,
-CSR_UEPC,
-CSR_UCAUSE,
-CSR_UTVAL,
-CSR_UIP,
-CSR_FFLAGS,
-CSR_FRM,
-CSR_FCSR,
-CSR_CYCLE,
-CSR_TIME,
-CSR_INSTRET,
-CSR_HPMCOUNTER3,
-CSR_HPMCOUNTER4,
-CSR_HPMCOUNTER5,
-CSR_HPMCOUNTER6,
-CSR_HPMCOUNTER7,
-CSR_HPMCOUNTER8,
-CSR_HPMCOUNTER9,
-CSR_HPMCOUNTER10,
-CSR_HPMCOUNTER11,
-CSR_HPMCOUNTER12,
-CSR_HPMCOUNTER13,
-CSR_HPMCOUNTER14,
-CSR_HPMCOUNTER15,
-CSR_HPMCOUNTER16,
-CSR_HPMCOUNTER17,
-CSR_HPMCOUNTER18,
-CSR_HPMCOUNTER19,
-CSR_HPMCOUNTER20,
-CSR_HPMCOUNTER21,
-CSR_HPMCOUNTER22,
-CSR_HPMCOUNTER23,
-CSR_HPMCOUNTER24,
-CSR_HPMCOUNTER25,
-CSR_HPMCOUNTER26,
-CSR_HPMCOUNTER27,
-CSR_HPMCOUNTER28,
-CSR_HPMCOUNTER29,
-CSR_HPMCOUNTER30,
-CSR_HPMCOUNTER31,
-CSR_CYCLEH,
-CSR_TIMEH,
-CSR_INSTRETH,
-CSR_HPMCOUNTER3H,
-CSR_HPMCOUNTER4H,
-CSR_HPMCOUNTER5H,
-CSR_HPMCOUNTER6H,
-CSR_HPMCOUNTER7H,
-CSR_HPMCOUNTER8H,
-CSR_HPMCOUNTER9H,
-CSR_HPMCOUNTER10H,
-CSR_HPMCOUNTER11H,
-CSR_HPMCOUNTER12H,
-CSR_HPMCOUNTER13H,
-CSR_HPMCOUNTER14H,
-CSR_HPMCOUNTER15H,
-CSR_HPMCOUNTER16H,
-CSR_HPMCOUNTER17H,
-CSR_HPMCOUNTER18H,
-CSR_HPMCOUNTER19H,
-CSR_HPMCOUNTER20H,
-CSR_HPMCOUNTER21H,
-CSR_HPMCOUNTER22H,
-CSR_HPMCOUNTER23H,
-CSR_HPMCOUNTER24H,
-CSR_HPMCOUNTER25H,
-CSR_HPMCOUNTER26H,
-CSR_HPMCOUNTER27H,
-CSR_HPMCOUNTER28H,
-CSR_HPMCOUNTER29H,
-CSR_HPMCOUNTER30H,
-CSR_HPMCOUNTER31H,
-CSR_SSTATUS,
-CSR_SEDELEG,
-CSR_SIDELEG,
-CSR_SIE,
-CSR_STVEC,
-CSR_SCOUNTEREN,
-CSR_SSCRATCH,
-CSR_SEPC,
-CSR_SCAUSE,
-CSR_STVAL,
-CSR_SIP,
-CSR_SATP,
-CSR_MVENDORID,
-CSR_MARCHID,
-CSR_MIMPID,
-CSR_MHARTID,
-CSR_MSTATUS,
-CSR_MISA,
-CSR_MEDELEG,
-CSR_MIDELEG,
-CSR_MIE,
-CSR_MTVEC,
-CSR_MCOUNTEREN,
-CSR_MSCRATCH,
-CSR_MEPC,
-CSR_MCAUSE,
-CSR_MTVAL,
-CSR_MIP,
-CSR_MTINST,
-CSR_MTVAL2,
-CSR_PMPCFG0,
-CSR_PMPCFG1,
-CSR_PMPCFG2,
-CSR_PMPCFG3,
