Re: [RFC PATCH v2 02/30] target/loongarch: Add CSR registers definition

[Richard Henderson <richard.henderson@linaro.org>]

On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
> +#define LOONGARCH_CSR_MISC           0x3 /* Misc config */
> +

Missing bitfield definitions for misc.

> +#define  EXCODE_IP                   64

What's this?

> +#define  EXCCODE_INT                 0
> +#define  EXCCODE_PIL                 1
> +#define  EXCCODE_PIS                 2
> +#define  EXCCODE_PIF                 3
> +#define  EXCCODE_PME                 4
> +#define  EXCCODE_PNR                 5
> +#define  EXCCODE_PNX                 6
> +#define  EXCCODE_PPI                 7
> +#define  EXCCODE_ADE                 8

ADEF vs ADEM?

> +#define  EXCCODE_ALE                 9
> +#define  EXCCODE_BCE                 10
> +#define  EXCCODE_SYS                 11
> +#define  EXCCODE_BRK                 12
> +#define  EXCCODE_INE                 13
> +#define  EXCCODE_IPE                 14
> +#define  EXCCODE_FPD                 15
> +#define  EXCCODE_SXD                 16
> +#define  EXCCODE_ASXD                17
> +#define  EXCCODE_FPE                 18 /* Have different expsubcode */
> +#define  EXCCODE_VFPE                18
> +#define  EXCCODE_WPEF                19 /* Have different expsubcode */
> +#define  EXCCODE_WPEM                19
> +#define  EXCCODE_BTD                 20
> +#define  EXCCODE_BTE                 21

Missing BSPR, HVC, GCSC, GCHC.

> +#define LOONGARCH_CSR_ERA            0x6 /* ERA */

Not really helpful to name the acronym with the acronym.
"Exception return address".

> +#define LOONGARCH_CSR_TLBELO0        0x12 /* TLB EntryLo0 */
> +FIELD(CSR_TLBELO0, V, 0, 1)
> +FIELD(CSR_TLBELO0, D, 1, 1)
> +FIELD(CSR_TLBELO0, PLV, 2, 2)
> +FIELD(CSR_TLBELO0, MAT, 4, 2)
> +FIELD(CSR_TLBELO0, G, 6, 1)
> +FIELD(CSR_TLBELO0, PPN, 12, 36)
> +FIELD(CSR_TLBELO0, NR, 61, 1)
> +FIELD(CSR_TLBELO0, NX, 62, 1)
> +FIELD(CSR_TLBELO0, RPLV, 63, 1)
> +
> +#define LOONGARCH_CSR_TLBELO1        0x13 /* 64 TLB EntryLo1 */
> +FIELD(CSR_TLBELO1, V, 0, 1)
> +FIELD(CSR_TLBELO1, D, 1, 1)
> +FIELD(CSR_TLBELO1, PLV, 2, 2)
> +FIELD(CSR_TLBELO1, MAT, 4, 2)
> +FIELD(CSR_TLBELO1, G, 6, 1)
> +FIELD(CSR_TLBELO1, PPN, 12, 36)
> +FIELD(CSR_TLBELO1, NR, 61, 1)
> +FIELD(CSR_TLBELO1, NX, 62, 1)
> +FIELD(CSR_TLBELO1, RPLV, 63, 1)

Better to define the fields once, dropping the 0/1 from the name.

> +#define LOONGARCH_CSR_PWCL           0x1c /* PWCl */

"Page walk controller, low addr"

> +#define LOONGARCH_CSR_PWCH           0x1d /* PWCh */

"Page walk controller, high addr"

> +#define LOONGARCH_CSR_STLBPS     0x1e /* 64 */

64?  "STLB Page size".

> +#define LOONGARCH_CSR_RVACFG         0x1f

"Reduced virtual address config"

> +/* Save registers */
> +#define LOONGARCH_CSR_SAVE0            0x30
> +#define LOONGARCH_CSR_SAVE1            0x31
> +#define LOONGARCH_CSR_SAVE2            0x32
> +#define LOONGARCH_CSR_SAVE3            0x33
> +#define LOONGARCH_CSR_SAVE4            0x34
> +#define LOONGARCH_CSR_SAVE5            0x35
> +#define LOONGARCH_CSR_SAVE6            0x36
> +#define LOONGARCH_CSR_SAVE7            0x37

Might as well must define SAVE0, and comment that the count is in PRCFG1.SAVE_NUM.

> +#define  CSR_DMW_BASE_SH             48

What's this?  It looks like you should be using TARGET_VIRT_ADDR_SPACE_BITS anyway.

> +#define dmwin_va2pa(va) \
> +    (va & (((unsigned long)1 << CSR_DMW_BASE_SH) - 1))

Using unsigned long is wrong, breaking 32-bit hosts.
You want

     ((va) & MAKE_64BIT_MASK(0, TARGET_VIRT_ADDR_SPACE_BITS))

> +/* Performance Counter registers */
> +#define LOONGARCH_CSR_PERFCTRL0      0x200 /* 32 perf event 0 config */
> +#define LOONGARCH_CSR_PERFCNTR0      0x201 /* 64 perf event 0 count value */
> +#define LOONGARCH_CSR_PERFCTRL1      0x202 /* 32 perf event 1 config */
> +#define LOONGARCH_CSR_PERFCNTR1      0x203 /* 64 perf event 1 count value */
> +#define LOONGARCH_CSR_PERFCTRL2      0x204 /* 32 perf event 2 config */
> +#define LOONGARCH_CSR_PERFCNTR2      0x205 /* 64 perf event 2 count value */
> +#define LOONGARCH_CSR_PERFCTRL3      0x206 /* 32 perf event 3 config */
> +#define LOONGARCH_CSR_PERFCNTR3      0x207 /* 64 perf event 3 count value */

Perhaps better to define

#define LOONGARCH_CSR_PERFCTRL(N)  (0x200 + 2 * N)

etc.

> +#define LOONGARCH_CSR_DB0ADDR        0x310 /* data breakpoint 0 address */
> +#define LOONGARCH_CSR_DB0MASK        0x311 /* data breakpoint 0 mask */
> +#define LOONGARCH_CSR_DB0CTL         0x312 /* data breakpoint 0 control */
> +#define LOONGARCH_CSR_DB0ASID        0x313 /* data breakpoint 0 asid */

Likewise.

> +    uint64_t CSR_SAVE0;
> +    uint64_t CSR_SAVE1;
> +    uint64_t CSR_SAVE2;
> +    uint64_t CSR_SAVE3;
> +    uint64_t CSR_SAVE4;
> +    uint64_t CSR_SAVE5;
> +    uint64_t CSR_SAVE6;
> +    uint64_t CSR_SAVE7;

Better as an array.

> +    uint64_t CSR_PERFCTRL0;
> +    uint64_t CSR_PERFCNTR0;
> +    uint64_t CSR_PERFCTRL1;
> +    uint64_t CSR_PERFCNTR1;
> +    uint64_t CSR_PERFCTRL2;
> +    uint64_t CSR_PERFCNTR2;
> +    uint64_t CSR_PERFCTRL3;
> +    uint64_t CSR_PERFCNTR3;

Likewise.

> +    uint64_t CSR_DB0ADDR;
> +    uint64_t CSR_DB0MASK;
> +    uint64_t CSR_DB0CTL;
> +    uint64_t CSR_DB0ASID;
> +    uint64_t CSR_DB1ADDR;
> +    uint64_t CSR_DB1MASK;
> +    uint64_t CSR_DB1CTL;
> +    uint64_t CSR_DB1ASID;
> +    uint64_t CSR_DB2ADDR;
> +    uint64_t CSR_DB2MASK;
> +    uint64_t CSR_DB2CTL;
> +    uint64_t CSR_DB2ASID;
> +    uint64_t CSR_DB3ADDR;
> +    uint64_t CSR_DB3MASK;
> +    uint64_t CSR_DB3CTL;
> +    uint64_t CSR_DB3ASID;

Likewise.

> +    uint64_t CSR_IB0ADDR;
> +    uint64_t CSR_IB0MASK;
> +    uint64_t CSR_IB0CTL;
> +    uint64_t CSR_IB0ASID;
> +    uint64_t CSR_IB1ADDR;
> +    uint64_t CSR_IB1MASK;
> +    uint64_t CSR_IB1CTL;
> +    uint64_t CSR_IB1ASID;
> +    uint64_t CSR_IB2ADDR;
> +    uint64_t CSR_IB2MASK;
> +    uint64_t CSR_IB2CTL;
> +    uint64_t CSR_IB2ASID;
> +    uint64_t CSR_IB3ADDR;
> +    uint64_t CSR_IB3MASK;
> +    uint64_t CSR_IB3CTL;
> +    uint64_t CSR_IB3ASID;
> +    uint64_t CSR_IB4ADDR;
> +    uint64_t CSR_IB4MASK;
> +    uint64_t CSR_IB4CTL;
> +    uint64_t CSR_IB4ASID;
> +    uint64_t CSR_IB5ADDR;
> +    uint64_t CSR_IB5MASK;
> +    uint64_t CSR_IB5CTL;
> +    uint64_t CSR_IB5ASID;
> +    uint64_t CSR_IB6ADDR;
> +    uint64_t CSR_IB6MASK;
> +    uint64_t CSR_IB6CTL;
> +    uint64_t CSR_IB6ASID;
> +    uint64_t CSR_IB7ADDR;
> +    uint64_t CSR_IB7MASK;
> +    uint64_t CSR_IB7CTL;
> +    uint64_t CSR_IB7ASID;

Likewise.


r~