[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 02/30] target/loongarch: Add CSR registers definition
From: |
yangxiaojuan |
Subject: |
Re: [RFC PATCH v2 02/30] target/loongarch: Add CSR registers definition |
Date: |
Fri, 12 Nov 2021 10:14:49 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux mips64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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~