Re: [PATCH v1 05/17] xen/riscv: implement virtual APLIC MMIO emulation

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/12/26 4:03 PM, Baptiste Le Duc wrote:
> > +
> > +static int cf_check vaplic_emulate_load(const struct vcpu *v,
>
> > +                                        const unsigned long addr,
> > +                                        uint32_t *out)
> > +{
> > +    const struct domain *d = v->domain;
> > +    const struct vaplic *vaplic = to_vaplic(d);
> > +    const unsigned int offset = addr & APLIC_REG_OFFSET_MASK;
>
> > +    uint32_t auth_mask;
> > +    unsigned int i;
> > +
> > +    switch ( offset )
> > +    {
> > +    case APLIC_DOMAINCFG:
> > +        *out = vaplic->regs.domaincfg;
> > +
> > +        return 0;
> > +
> > +    case APLIC_SETIPNUM:
> > +    case APLIC_SETIPNUM_LE:
> > +    case APLIC_CLRIPNUM:
> > +    case APLIC_SETIENUM:
> > +    case APLIC_CLRIENUM:
> > +    case APLIC_CLRIE_BASE ... APLIC_CLRIE_LAST:
> > +        /*
> > +         * Based on the RISC-V AIA spec a read of these registers
> > +         * always returns zero
> > +         */
> > +        *out = 0;
> > +
> > +        return 0;
> > +
> > +    case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
> > +    case APLIC_CLRIP_BASE ... APLIC_CLRIP_LAST:
> > +    case APLIC_SETIE_BASE ... APLIC_SETIE_LAST:
> > +        i = regoffset_to_word_idx(offset & APLIC_SETCLR_OFFSET_MASK);
> > +        auth_mask = generate_auth_mask(d, i);
> > +
> > +        break;
> > +
> > +    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
> > +        /*
> > +         * As target registers start from 1:
> > +         *  0x3000 genmsi
> > +         *  0x3004 target[1]
> > +         *  0x3008 target[2]
> > +         *   ...
> > +         *  0x3FFC target[1023]
> > +         * It is necessary to calculate an interrupt number by subtracting
> > +         * APLIC_GENMSI instead of APLIC_TARGET_BASE.
> > +         */
> > +        i = regoffset_to_word_idx(offset - APLIC_GENMSI);
> > +
> > +        if ( !AUTH_IRQ_BIT(d, i) )
> > +        {
> > +            *out = 0;
> > +
> > +            return 0;
> > +        }
> > +
> > +        auth_mask = ~0U;
> > +
> > +        break;
> > +
> > +    default:
> > +        gdprintk(XENLOG_WARNING, "Unhandled APLIC read at offset %#x\n",
> > +                 offset);
> > +
> > +        return -EINVAL;
> > +    }
> > +
> > +    *out = aplic_hw_read_reg(offset, auth_mask);
> I think there is a problem here for the target registers: a read does not
> return what the guest wrote.
>
> Consider domU calling request_irq() for source 10, with the interrupt
> affinity to vCPU1:
>
>        writel(0x0004000A, GUEST_APLIC_BASE + 0x3028)
>        /* hart_idx = 1 (vCPU1), guest_idx = 0, EIID = 10 */
>
> vaplic_emulate_store() passes this through aplic_msi_target_gen(), which
> keeps only the EIID and substitutes the physical hart field and the
> vCPU's guest interrupt file index, so we write target[10] = 0x001C100A
>
> Therefore, a readl() of the same address returns that raw value (0x001C100A) 
> instead of 0x0004000A, since
> auth_mask is ~0U here.
I found this issue while working on support for the IMSIC software 
interrupt file. I already have a fix that I need to port to this code. 
However, I completely missed that this was already an issue and that the 
fix should have been ported earlier.
Thanks!

~ Oleksii