Re: [RFC PATCH v7 16/29] hw/loongarch: Add LoongArch ipi interrupt support(IPI)

[Richard Henderson]

On 3/28/22 06:57, Xiaojuan Yang wrote:
> +static void loongarch_ipi_writel(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned size)
> +{
> +    IPICore *s = opaque;
> +    int index = 0;
> +
> +    addr &= 0xff;
> +    trace_loongarch_ipi_write(size, (uint64_t)addr, val);
> +    switch (addr) {
> +    case CORE_STATUS_OFF:
> +        qemu_log_mask(LOG_GUEST_ERROR, "can not be written");
> +        break;
> +    case CORE_EN_OFF:
> +        s->en = val;
> +        break;

Changes to s->en should affect irq.

> +    case CORE_SET_OFF:
> +        s->status |= val;
> +        if (s->status != 0) {
> +            qemu_irq_raise(s->irq);
> +        }

I think s->en should be taken into account when raising irq.

> +        break;
> +    case CORE_CLEAR_OFF:
> +        s->status ^= val;

Incorrect: status &= ~val.

> +        if (s->status == 0) {
> +            qemu_irq_lower(s->irq);
> +        }

Likewise, s->en.

> +        break;
> +    case CORE_BUF_20 ... CORE_BUF_38 + 4:
> +        index = (addr - CORE_BUF_20) >> 2;
> +        s->buf[index] = val;
> +        break;
> +    case IOCSR_IPI_SEND:
> +        s->status |= val;

I can't see where this comes from, but helper_iocsr_write is very confusing.  It *appears* 
as if this is never invoked, because IPI_SEND is handled directly in helper_iocsr_write 
(which also seems wrong).


r~