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

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