Re: [Qemu-devel] [PATCH 3/5 v2] RISC-V: Map gdb CSR reg numbers to hw reg numbers.

[Jim Wilson]

On Tue, Jan 22, 2019 at 1:45 PM Alistair Francis  wrote:
> I think it makes more sense to just define the variable in the
> gdbstubs.c file then. Can you move it to patch 5?

Yes, that is no problem.  That makes patch 3 a lot smaller and patch 5
a lot bigger, but it is the same code as before, just arranged
differently, so this shouldn't complicate the review too much.

Jim

Re: [Qemu-devel] [PATCH 3/5 v2] RISC-V: Map gdb CSR reg numbers to hw reg numbers.

[Alistair Francis]

On Sun, Dec 30, 2018 at 11:22 AM Jim Wilson  wrote:
>
> On Sat, Dec 29, 2018 at 2:23 PM Richard Henderson
>  wrote:
> > On 12/29/18 9:09 AM, Jim Wilson wrote:
> > > +int csr_register_map[] = {
> >
> > static const?
>
> If I add static const here, then I get a build error if this patch is
> applied to the tree but the following patch #5 that uses the variable
> is not applied.  Though I suppose I could fix that if I put the static
> const in patch 5.  That would look a little funny but would work.
>
> > Putting an initialized variable in a header file doesn't seem right.  Is 
> > this
> > supposed to be a declaration that is shared between c files?
>
> I did it this way for two reasons.  It makes it easier to keep the
> register mapping consistent with the gdb csr file, if the gdb csr file
> happens to change in the future.  We can just do a line by line
> comparison of the gdb csr file against the csr-map.h file to verify
> that the csr names match.  And it makes for a cleaner patch if the gdb
> csr file register numbering gets fixed in the future, in that case we
> can just delete this file and change a few lines to stop using this
> variable.
>
> This variable is only meant to be used in one file, the
> target/riscv/gdbstubs.c file.

I think it makes more sense to just define the variable in the
gdbstubs.c file then. Can you move it to patch 5?

Alistair

>
> Jim
>

Re: [Qemu-devel] [PATCH 3/5 v2] RISC-V: Map gdb CSR reg numbers to hw reg numbers.

[Jim Wilson]

On Sat, Dec 29, 2018 at 2:23 PM Richard Henderson
 wrote:
> On 12/29/18 9:09 AM, Jim Wilson wrote:
> > +int csr_register_map[] = {
>
> static const?

If I add static const here, then I get a build error if this patch is
applied to the tree but the following patch #5 that uses the variable
is not applied.  Though I suppose I could fix that if I put the static
const in patch 5.  That would look a little funny but would work.

> Putting an initialized variable in a header file doesn't seem right.  Is this
> supposed to be a declaration that is shared between c files?

I did it this way for two reasons.  It makes it easier to keep the
register mapping consistent with the gdb csr file, if the gdb csr file
happens to change in the future.  We can just do a line by line
comparison of the gdb csr file against the csr-map.h file to verify
that the csr names match.  And it makes for a cleaner patch if the gdb
csr file register numbering gets fixed in the future, in that case we
can just delete this file and change a few lines to stop using this
variable.

This variable is only meant to be used in one file, the
target/riscv/gdbstubs.c file.

Jim

Re: [Qemu-devel] [PATCH 3/5 v2] RISC-V: Map gdb CSR reg numbers to hw reg numbers.

[Richard Henderson]

On 12/29/18 9:09 AM, Jim Wilson wrote:
> +++ b/target/riscv/csr-map.h
> @@ -0,0 +1,248 @@
> +/*
> + * 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.
> + */
> +
> +int csr_register_map[] = {

static const?

Putting an initialized variable in a header file doesn't seem right.  Is this
supposed to be a declaration that is shared between c files?


r~

[Qemu-devel] [PATCH 3/5 v2] RISC-V: Map gdb CSR reg numbers to hw reg numbers.

[Jim Wilson]

The gdb CSR xml file has registers in documentation order, not numerical
order, so we need a table to map the register numbers.  This also adds
some missing CSR_* register macros.

Signed-off-by: Jim Wilson 
---
 target/riscv/cpu_bits.h |  35 ++-
 target/riscv/csr-map.h  | 248 
 target/riscv/gdbstub.c  |   1 +
 3 files changed, 282 insertions(+), 2 deletions(-)
 create mode 100644 target/riscv/csr-map.h

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 5439f47..316d500 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -135,16 +135,22 @@
 /* Legacy Counter Setup (priv v1.9.1) */
 #define CSR_MUCOUNTEREN 0x320
 #define CSR_MSCOUNTEREN 0x321
+#define CSR_MHCOUNTEREN 0x322
 
 /* Machine Trap Handling */
 #define CSR_MSCRATCH0x340
 #define CSR_MEPC0x341
 #define CSR_MCAUSE  0x342
-#define CSR_MBADADDR0x343
+#define CSR_MTVAL   0x343
 #define CSR_MIP 0x344
 
+/* Legacy Machine Trap Handling (priv v1.9.1) */
+#define CSR_MBADADDR0x343
+
 /* Supervisor Trap Setup */
 #define CSR_SSTATUS 0x100
+#define CSR_SEDELEG 0x102
+#define CSR_SIDELEG 0x103
 #define CSR_SIE 0x104
 #define CSR_STVEC   0x105
 #define CSR_SCOUNTEREN  0x106
@@ -153,9 +159,12 @@
 #define CSR_SSCRATCH0x140
 #define CSR_SEPC0x141
 #define CSR_SCAUSE  0x142
-#define CSR_SBADADDR0x143
+#define CSR_STVAL   0x143
 #define CSR_SIP 0x144
 
+/* Legacy Supervisor Trap Handling (priv v1.9.1) */
+#define CSR_SBADADDR0x143
+
 /* Supervisor Protection and Translation */
 #define CSR_SPTBR   0x180
 #define CSR_SATP0x180
@@ -282,6 +291,28 @@
 #define CSR_MHPMCOUNTER30H  0xb9e
 #define CSR_MHPMCOUNTER31H  0xb9f
 
+/* Legacy Hypervisor Trap Setup (priv v1.9.1) */
+#define CSR_HSTATUS 0x200
+#define CSR_HEDELEG 0x202
+#define CSR_HIDELEG 0x203
+#define CSR_HIE 0x204
+#define CSR_HTVEC   0x205
+
+/* Legacy Hypervisor Trap Handling (priv v1.9.1) */
+#define CSR_HSCRATCH0x240
+#define CSR_HEPC0x241
+#define CSR_HCAUSE  0x242
+#define CSR_HBADADDR0x243
+#define CSR_HIP 0x244
+
+/* Legacy Machine Protection and Translation (priv v1.9.1) */
+#define CSR_MBASE   0x380
+#define CSR_MBOUND  0x381
+#define CSR_MIBASE  0x382
+#define CSR_MIBOUND 0x383
+#define CSR_MDBASE  0x384
+#define CSR_MDBOUND 0x385
+
 /* mstatus CSR bits */
 #define MSTATUS_UIE 0x0001
 #define MSTATUS_SIE 0x0002
diff --git a/target/riscv/csr-map.h b/target/riscv/csr-map.h
new file mode 100644
index 000..cce32fd
--- /dev/null
+++ b/target/riscv/csr-map.h
@@ -0,0 +1,248 @@
+/*
+ * 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.
+ */
+
+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,
+