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

[maobibo]

On 2024/7/15 下午11:31, Philippe Mathieu-Daudé wrote:
> 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.

Good catch, there is compile warning when address_space_ldl() is used, 
so address_space_ldl_le() is used to remove compile warning. I will 
double check the compile warning issue.

Regard
Bibo Mao
> > +        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;
> > +}