Re: [PATCH v1 26/43] target/loongarch: Add LoongArch IOCSR instruction

[Richard Henderson]

On 4/15/22 02:40, Xiaojuan Yang wrote:
> +static bool trans_iocsrrd_b(DisasContext *ctx, arg_iocsrrd_b *a)
> +static bool trans_iocsrrd_h(DisasContext *ctx, arg_iocsrrd_h *a)
> +static bool trans_iocsrrd_w(DisasContext *ctx, arg_iocsrrd_w *a)
> +static bool trans_iocsrrd_d(DisasContext *ctx, arg_iocsrrd_d *a)

You have all of these split apart, then pass an integer to a common routine...

> +uint64_t helper_iocsr_read(CPULoongArchState *env, target_ulong r_addr,
> +                           uint32_t size)
> +{
> +    int cpuid = env_cpu(env)->cpu_index;
> +    CPUState  *cs = qemu_get_cpu(cpuid);
> +    env = cs->env_ptr;
> +    uint64_t ret = 0;
> +
> +    /*
> +     * Adjust the per core address such as 0x10xx(IPI)/0x18xx(EXTIOI)
> +     */
> +    if (((r_addr & 0xff00) == 0x1000) || ((r_addr & 0xff00) == 0x1800)) {
> +        r_addr = r_addr + ((target_ulong)(cpuid & 0x3) << 8);
> +    }
> +
> +    switch (size) {
> +    case 1:
> +        ret = address_space_ldub(&env->address_space_iocsr, r_addr,
> +                                 MEMTXATTRS_UNSPECIFIED, NULL);
> +        break;
> +    case 2:
> +        ret = address_space_lduw(&env->address_space_iocsr, r_addr,
> +                                 MEMTXATTRS_UNSPECIFIED, NULL);
> +        break;
> +    case 4:
> +        ret = address_space_ldl(&env->address_space_iocsr, r_addr,
> +                                MEMTXATTRS_UNSPECIFIED, NULL);
> +        break;
> +    case 8:
> +        ret = address_space_ldq(&env->address_space_iocsr, r_addr,
> +                                MEMTXATTRS_UNSPECIFIED, NULL);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }

... then have to split them apart again.  It would be cleaner to have 4 helpers, one for 
each size.

I'm concerned about the address adjustment.  My thinking is that this should be handled by 
the address space (via a MemoryRegionOps entry).  I say this because there is nothing 
about this adjustment in "LoongArch Reference Manual", but rather in the "LoongArch 3A5000 
Registers Technical Reference Manual".  Which means that baking this cpu specific 
behaviour into the generic architecture is incorrect.


> +void helper_iocsr_write(CPULoongArchState *env, target_ulong w_addr,
> +                        target_ulong val, uint32_t size)
> +{
> +    int cpuid = env_cpu(env)->cpu_index;
> +    CPUState *cs = qemu_get_cpu(cpuid);
> +    int mask, i;
> +    env = cs->env_ptr;
> +
> +    /*
> +     * For IPI send, Mailbox send and ANY send, adjust the addr and
> +     * val accordingly. The IOCSR writes are turned to different
> +     * MMIO writes respectively
> +     */
> +    switch (w_addr) {
> +    case 0x1040: /* IPI send */

Likewise.


r~