Hi, Richard:
On 11/12/2021 01:43 AM, Richard Henderson wrote:
> On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
>> This includes:
>> - CSRRD
>> - CSRWR
>> - CSRXCHG
>> - IOCSR{RD/WR}.{B/H/W/D}
>
> I think IOCSR should be in a separate patch.
> It's completely unrelated to the other CSRs.
>
>> +target_ulong helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
>> +{
>> + int64_t v;
>> +
>> + switch (csr) {
>> + case LOONGARCH_CSR_PGD:
>> + if (env->CSR_TLBRERA & 0x1) {
>> + v = env->CSR_TLBRBADV;
>> + } else {
>> + v = env->CSR_BADV;
>> + }
>> +
>> + if ((v >> 63) & 0x1) {
>> + v = env->CSR_PGDH;
>> + } else {
>> + v = env->CSR_PGDL;
>> + }
>> + v = v & TARGET_PHYS_MASK;
>
> This csr is defined to be GRLEN bits; I this mask looks wrong.
>
>> + default:
>> + assert(0);
>
> g_assert_not_reached.
>
>> + switch (csr) {
>> + case LOONGARCH_CSR_ASID:
>> + old_v = env->CSR_ASID;
>> + env->CSR_ASID = val;
>
> Mask the write to the field; you don't want to corrupt ASIDBITS, or the other
> read-only bits.
Ok, all above have been modified.
>
>> + case LOONGARCH_CSR_TCFG:
>> + old_v = env->CSR_TCFG;
>> + cpu_loongarch_store_stable_timer_config(env, val);
>> + break;
>> + case LOONGARCH_CSR_TINTCLR:
>> + old_v = 0;
>> + qemu_irq_lower(env->irq[IRQ_TIMER]);
>
> The interrupt is not documented to clear on any write; only writes of 1 to
> bit 0.
I think the manual has mentioned at 7.6.5 which says when 1 is written to this
bit, the clock interrupt
flag is cleared.
>
>> + default:
>> + assert(0);
>
> g_assert_not_reached.
>
>> + }
>> +
>> + return old_v;
>> +}
>> +
>> +target_ulong helper_csr_xchgq(CPULoongArchState *env, target_ulong val,
>> + target_ulong mask, uint64_t csr)
>> +{
>> + target_ulong tmp;
>> + target_ulong v = val & mask;
>
> I think it would be less confusing to name the input parameter new_val, and
> the local temporary old_val.
>
>> +#define CASE_CSR_XCHGQ(csr) \
>> + case LOONGARCH_CSR_ ## csr: \
>> + { \
>> + val = env->CSR_ ## csr; \
>> + env->CSR_ ## csr = (env->CSR_ ## csr) & (~mask); \
>> + env->CSR_ ## csr = (env->CSR_ ## csr) | v; \
>
> old_val = env->CSR_##csr;
> env->CSR_##csr = (old_val & ~mask) | (new_val & mask);
>
>
>> + switch (csr) {
>> + CASE_CSR_XCHGQ(CRMD)
>
> I wonder if all of this would be better with a table of offsets, which could
> be shared with the translator.
>
> #define CSR_OFF(X) [LOONGARCH_CSR_##X] = offsetof(CPUArchState, CSR_##X)
>
> static const int csr_offsets[] = {
> CSR_OFF(CRMD),
> ...
> };
>
> int cpu_csr_offset(unsigned csr_num)
> {
> if (csr_num < ARRAY_SIZE(csr_offsets)) {
> return csr_offsets[csr_num];
> }
> return 0;
> }
>
> Which reduces this function to
>
> unsigned csr_offset = cpu_csr_offset(csr_num);
> if (csr_offset == 0) {
> /* CSR is undefined: read as 0, write ignored. */
> return 0;
> }
>
> uint64_t *csr = (void *)env + csr_offset;
> uint64_t old_val = *csr;
>
> new_val = (new_val & mask) | (old_val & ~mask);
>
> *csr = (old_val & ~mask) | (new_val & mask);
>
> if (csr_num == LOONGARCH_CSR_TCFG) {
> cpu_loongarch_store_stable_timer_config(env, new_val);
> } else {
> *csr = new_val;
> }
> return old_val;
>
>> +uint64_t helper_iocsr_read(CPULoongArchState *env, target_ulong r_addr,
>> + uint32_t size)
>> +{
>> + LoongArchMachineState *lams = LOONGARCH_MACHINE(qdev_get_machine());
>> + int cpuid = env_cpu(env)->cpu_index;
>> +
>> + if (((r_addr & 0xff00) == 0x1000) || ((r_addr & 0xff00) == 0x1800)) {
>> + r_addr = r_addr + ((target_ulong)(cpuid & 0x3) << 8);
>> + }
>
> This looks to be something that should be controlled by the address space
> assigned to each cpu.
>
> But it's hard to tell.
>
> Where is the documentation for this? I didn't immediately find it in 3A5000
> Technical Reference Manual, Chapter 10.
Yes, most iocsr instructions introduced on 3A5000 Technical Reference Manual,
Chapter 10.
Table 10-2, 10-3, 10-4, 10-5 and 11-10 lists per core iocsr
>
>> +void helper_iocsr_write(CPULoongArchState *env, target_ulong w_addr,
>> + target_ulong val, uint32_t size)
>> +{
>> + LoongArchMachineState *lams = LOONGARCH_MACHINE(qdev_get_machine());
>> + int cpuid = env_cpu(env)->cpu_index;
>> + int mask, i;
>> +
>> + /*
>> + * For IPI send, Mail send, ANY send adjust addr and val
>> + * according to their real meaning
>> + */
>> + if (w_addr == 0x1040) { /* IPI send */
>> + cpuid = (val >> 16) & 0x3ff;
>> + val = 1UL << (val & 0x1f);
>> + w_addr = 0x1008;
>
> I don't see any interrupts actually being raised?
I define the memory region ops at the IPI interrupt controller, the ipi write
here will lead to the ops and raise the
interrupt at the read/write function. I don't know if this is appropriate, but
most of the iocsr read/write are closely
related to interrupt controller.
>
>> +static bool trans_csrrd(DisasContext *ctx, arg_csrrd *a)
>> +{
>> + TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
>> +
>> + gen_helper_check_plv(cpu_env);
>
> You don't need an external call. PLV should be part of TB_FLAGS, so you can
> check this during translation.
>
>> + case LOONGARCH_CSR_TVAL:
>> + gen_helper_csr_rdq(dest, cpu_env, tcg_constant_i64(a->csr));
>> + break;
>> + default:
>> + assert(0);
>
> The assert was definitely wrong, as it allows incorrect programs to crash
> qemu. In addition, unknown csr read as 0.
>
>> + CASE_CSR_WRQ(MISC)
>
> You don't actually support any of the MISC bits yet.
> You should make this register read-only until you do.
>
> How many more registers are read-only, or have read-only fields that you are
> not checking?
>
Yes, I will check all of them.
Futher, Can you help review the remaining patches? since the board and machine
code needs rewrite. A new version
is need. The remaining patches are:
0011-target-loongarch-Add-LoongArch-interrupt-and-excepti.patch
0012-target-loongarch-Add-timer-related-instructions-supp.patch
0013-target-loongarch-Add-gdb-support.patch
0014-target-loongarch-Implement-privilege-instructions-di.patch
Thank you for all your work.
Xiaojuan
>
> r~
