Re: [PATCH v6 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)

[Richard Henderson]

On 6/1/22 03:25, Xiaojuan Yang wrote:
> +static uint64_t extioi_readw(void *opaque, hwaddr addr, unsigned size)
> +{
> +    LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
> +    unsigned long offset = addr & 0xffff;
> +    uint32_t index, cpu, ret = 0;
> +
> +    switch (offset) {
> +    case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
> +        index = (offset - EXTIOI_NODETYPE_START) >> 2;
> +        ret = s->nodetype[index];
> +        break;
> +    case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
> +        index = offset - EXTIOI_IPMAP_START;
> +        ret = *(uint32_t *)&s->ipmap[index];

This...

> +        break;
> +    case EXTIOI_ENABLE_START ... EXTIOI_ENABLE_END - 1:
> +        index = (offset - EXTIOI_ENABLE_START) >> 2;
> +        ret = s->enable[index];
> +        break;
> +    case EXTIOI_BOUNCE_START ... EXTIOI_BOUNCE_END - 1:
> +        index = (offset - EXTIOI_BOUNCE_START) >> 2;
> +        ret = s->bounce[index];
> +        break;
> +    case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
> +        index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
> +        cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
> +        ret = s->coreisr[cpu][index];
> +        break;
> +    case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1:
> +        index = offset - EXTIOI_COREMAP_START;
> +        ret = *(uint32_t *)&s->coremap[index];

... and this are points of concern.  You can't simply re-interpret an array of uint8_t as 
uint32_t without running into host endian issues.

I wonder why you've declared them as uint8_t at all?  Both read and write use this cast. 
Was this some attempt to avoid

    s->coremap[index / 4]?

or what?

> +static const VMStateDescription vmstate_loongarch_extioi = {
> +    .name = TYPE_LOONGARCH_EXTIOI,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(bounce, LoongArchExtIOI, EXTIOI_IRQS_GROUP_COUNT),
> +        VMSTATE_UINT32_2DARRAY(coreisr, LoongArchExtIOI, LOONGARCH_MAX_VCPUS,
> +                               EXTIOI_IRQS_GROUP_COUNT),
> +        VMSTATE_UINT32_ARRAY(nodetype, LoongArchExtIOI,
> +                             EXTIOI_IRQS_NODETYPE_COUNT / 2),
> +        VMSTATE_UINT32_ARRAY(enable, LoongArchExtIOI, EXTIOI_IRQS / 32),
> +        VMSTATE_UINT32_ARRAY(isr, LoongArchExtIOI, EXTIOI_IRQS / 32),
> +        VMSTATE_UINT8_ARRAY(ipmap, LoongArchExtIOI, EXTIOI_IRQS_IPMAP_SIZE),
> +        VMSTATE_UINT8_ARRAY(coremap, LoongArchExtIOI, EXTIOI_IRQS),
> +        VMSTATE_END_OF_LIST()
> +    }

Missing the sw_* members.


r~