Re: [RFC PATCH v2 08/30] target/loongarch: Add LoongArch CSR/IOCSR instruction

[yangxiaojuan]

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~