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~
