Re: [PATCH v2 1/4] hw/intc/loongson_ipi_common: Add loongson ipi common class

[Philippe Mathieu-Daudé]

On 4/7/24 05:37, Bibo Mao wrote:
> Loongson ipi common class and instance is created here, it comes
> from file loongson_ipi mostly. For the new added loongson ipi
> common class, there is four interfaces defined here:
>   1. Interfaces pre_save/post_load are used for future kvm child class
>   2. Interface get_iocsr_as can be used for different architectures,
> now MIPS 3A4000 and LoongArch 3A5000 machine use this ip, can inherit
> this common class.
>   3. Interace cpu_by_arch_id is added, by default generic function
> cpu_by_arch_id() is used to search vcpu from physical cpuid, it is
> generic searching method. Different machine may define other search
> methods such binary searching method.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   hw/intc/loongson_ipi_common.c         | 394 ++++++++++++++++++++++++++
>   include/hw/intc/loongson_ipi_common.h |  77 +++++
>   2 files changed, 471 insertions(+)
>   create mode 100644 hw/intc/loongson_ipi_common.c
>   create mode 100644 include/hw/intc/loongson_ipi_common.h


> +static MemTxResult send_ipi_data(LoongsonIPICommonState *ipi, CPUState *cpu,
> +                                 uint64_t val,
> +                                 hwaddr addr, MemTxAttrs attrs)
> +{
> +    int i, mask = 0, data = 0;
> +    AddressSpace *iocsr_as;
> +    LoongsonIPICommonClass *licc = LOONGSON_IPI_COMMON_GET_CLASS(ipi);
> +
> +    iocsr_as = NULL;
> +    if (licc->get_iocsr_as) {
> +        iocsr_as = licc->get_iocsr_as(cpu);
> +    }
> +
> +    if (!iocsr_as) {
> +        return MEMTX_DECODE_ERROR;
> +    }
> +
> +    /*
> +     * bit 27-30 is mask for byte writing,
> +     * if the mask is 0, we need not to do anything.
> +     */
> +    if ((val >> 27) & 0xf) {
> +        data = address_space_ldl_le(iocsr_as, addr, attrs, NULL);

Adding/removing files make noticing the uncommented changes very hard
(see my v3).

Here you use little-endian instead of host one. This device is L-E only
so this makes sense, but I'd have appreciate at least a comment about
it.

> +        for (i = 0; i < 4; i++) {
> +            /* get mask for byte writing */
> +            if (val & (0x1 << (27 + i))) {
> +                mask |= 0xff << (i * 8);
> +            }
> +        }
> +    }
> +
> +    data &= mask;
> +    data |= (val >> 32) & ~mask;
> +    address_space_stl_le(iocsr_as, addr, data, attrs, NULL);

Ditto.

> +    return MEMTX_OK;
> +}