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

From: yangxiaojuan <yangxiaojuan@loongson.cn>
To: Richard Henderson <richard.henderson@linaro.org>, qemu-devel@nongnu.org
Cc: Song Gao <gaosong@loongson.cn>
Subject: Re: [RFC PATCH v2 02/30] target/loongarch: Add CSR registers definition
Date: Fri, 12 Nov 2021 10:14:49 +0800	[thread overview]
Message-ID: <bd00cd00-2dea-d6dc-f151-62ca3d90003d@loongson.cn> (raw)
In-Reply-To: <7bb438bb-e4ae-8f28-8f9e-7baecbccc1ac@linaro.org>

Hi, Richard,

On 11/11/2021 09:29 PM, Richard Henderson wrote:
> On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
>> +#define LOONGARCH_CSR_MISC           0x3 /* Misc config */
>> +
> 
> Missing bitfield definitions for misc.
> 

 At present, there is no read/write to each field of misc register so it is not defined.

>> +#define  EXCODE_IP                   64
> 
> What's this?
> 

The manual 6.1.3 says the exception number of an interrupt is the exception number plus an offset of 64.
This defines the offset, sorry, the name is bad, maybe change a name or just use 64 directly in the
exception handle is better.

>> +#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?
> 
Yes, ADEF and ADEM have different esubcode, maybe I should add a description.

>> +#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.
> 

 At present, these exceptions are not triggered, so they are not defined.
 They are used by kvm. 

>> +#define LOONGARCH_CSR_ERA            0x6 /* ERA */
> 
> Not really helpful to name the acronym with the acronym.
> "Exception return address".
> 

OK, I will use a more detailed description for all the registers.

>> +#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.
OK, I will modify.
> 
>> +#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.
Yes, TARGET_VIRT_ADDR_SPACE_BITS is better.
> 
>> +#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.
> 
> 
  I will modify the define. Thanks
> r~