Re: [PATCH v2 09/39] xen/riscv: implement virtual APLIC MMIO emulation

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/9/26 4:26 PM, Jan Beulich wrote:
> On 27.08.2026 17:20, Oleksii Kurochko wrote:
> > +uint32_t aplic_msi_target_gen(const struct vcpu *target_vcpu,
> > +                              uint32_t base_val)
> > +{
> > +    unsigned int guest_id = vcpu_guest_file_id(target_vcpu);
> > +    unsigned long hart_field = aplic_hart_field(target_vcpu->processor);
> What guarantees target_vcpu's ->processor field to be meaningful at this
> point? 
Good question. Considering the places where it is called, I would expect 
target_vcpu->processor to have something meaningful, since it is called 
from a place that can only be reached when the guest is running.
Even without that, I don't think there is a big issue here, as 
->processor is initialized to 0 at allocation time. This means that the 
target register will be configured in such a way that CPU 0 will handle 
such IRQs.
> (Also again naming of the parameter: Generally it wants to be "v"
> or "curr"; only very special cases may use other names.)
I will use just `v` then here.

> > +uint32_t aplic_hw_read_reg(unsigned int offset)
> > +{
> > +    unsigned long flags;
> > +    uint32_t val;
> > +
> > +    ASSERT((offset < aplic.size) && IS_ALIGNED(offset, sizeof(uint32_t)));
> > +
> > +    spin_lock_irqsave(&aplic.lock, flags);
> > +    val = readl((volatile void __iomem *)aplic.regs + offset);
> Please can this have const alongside volatile?
Sure, I will add.

> > @@ -98,6 +108,27 @@
> >   #define APLIC_SIZE(nr_cpus) \
> >       (APLIC_MIN_SIZE + APLIC_SIZE_ALIGN(APLIC_IDC_SIZE * (nr_cpus)))
> >   
> > +/*
> > + * Using setip is fine here, as all SET* and CLR* register groups consist of 32
> > + * registers and therefore have identical sizes.
> > + *
> > + * Lowest 2 bits are always zero for SET* and CLR* registers.
> > + */
> > +#define APLIC_SETCLR_OFFSET_MASK \
> > +    (sizeof_field(struct aplic_regs, setip) - sizeof(uint32_t))
> > +
> > +#define APLIC_xMSICFGADDR_PPN_SHIFT IMSIC_MMIO_PAGE_SHIFT
> > +
> > +#define APLIC_xMSICFGADDR_PPN_HHX_MASK(hhxw) \
> > +    (BIT(hhxw, UL) - 1)
> > +#define APLIC_xMSICFGADDR_PPN_HHX_SHIFT(hhxs) \
> > +    ((hhxs) + APLIC_xMSICFGADDR_PPN_SHIFT)
> > +
> > +#define APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw) \
> > +    (BIT(lhxw, UL) - 1)
> > +#define APLIC_xMSICFGADDR_PPN_LHX_SHIFT(lhxs) \
> > +    (lhxs)
> May I ask to avoid unnecessary line splitting here as well?
I will fix that for APLIC_xMSICFGADDR_PPN_HHX_MASK, 
APLIC_xMSICFGADDR_PPN_LHX_MASK and APLIC_xMSICFGADDR_PPN_LHX_SHIFT.
> > --- a/xen/arch/riscv/include/asm/vaplic.h
> > +++ b/xen/arch/riscv/include/asm/vaplic.h
> > @@ -21,11 +21,16 @@ struct domain;
> >   
> >   struct vaplic_regs {
> >       uint32_t domaincfg;
> > +
> > +    uint32_t *target;
> >   };
> A pointer in this structure is odd, as this (supposedly) is a set of
> guest register values. The field name also doesn't clarify its purpose.
> All in all: Likely a comment is needed here.
It is a pointer because I tried to save some memory and allocate 
register target based on how many irqs a guest really needed.
As an option I can allocate that statically and use the value from AIA
spec: target[1]-target[1023]. Would it better?

If a dynamic allocation is still fine then I will add the following comment:

    /*
     * Guest's view of the APLIC target registers, indexed by IRQ number.
     *
     * The array holds d->arch.vintc->nr_virqs elements; target[0] is 
unused
     * as APLIC interrupt sources start from 1.
     */
    uint32_t *target;


> > --- a/xen/arch/riscv/vaplic.c
> > +++ b/xen/arch/riscv/vaplic.c
> > @@ -17,6 +17,7 @@
> >   #include <asm/aia.h>
> >   #include <asm/imsic.h>
> >   #include <asm/intc.h>
> > +#include <asm/mmio.h>
> >   #include <asm/vaplic.h>
> >   
> >   #include "aplic-priv.h"
> > @@ -27,6 +28,279 @@ unsigned int __ro_after_init guest_aplic_num_sources;
> >   
> >   #define FDT_VAPLIC_INT_CELLS 2
> >   
> > +#define AUTH_IRQ_BIT(d, irqn) \
> > +    (((irqn) < (d)->arch.vintc->nr_virqs) && \
> > +     test_bit(irqn, (d)->arch.vintc->used_irqs))
> > +
> > +/*
> > + * Convert a byte offset (within a SETIP/CLRIP/SETIE/CLRIE register group) to
> > + * a 32-bit word index into the used_irqs bitmap. Each word covers 32
> > + * interrupt sources. For SOURCECFG and TARGET groups the same division also
> > + * yields the interrupt number directly, because those arrays store one 32-bit
> > + * register per source.
> > + */
> > +#define regoffset_to_word_idx(reg_val) ((reg_val) / sizeof(uint32_t))
> > +
> > +static uint32_t vaplic_target_read(const struct domain *d, unsigned int irqn)
> > +{
> > +    const struct vaplic *vaplic = to_vaplic(d);
> > +
> > +    /* target[0] doesn't exist so irqn == 0 should be impossible */
> > +    if ( !irqn || irqn >= vaplic->vintc.nr_virqs )
> > +        return 0;
> > +
> > +    return read_atomic(&vaplic->regs.target[irqn]);
> > +}
> > +
> > +static inline uint32_t generate_auth_mask(const struct domain *currd,
> > +                                          unsigned int word_idx)
> > +{
> > +    unsigned int first_bit = word_idx * sizeof(uint32_t) * BITS_PER_BYTE;
> > +
> > +    if ( word_idx >= DIV_ROUND_UP(currd->arch.vintc->nr_virqs,
> > +                                  sizeof(uint32_t) * BITS_PER_BYTE) )
> > +    {
> > +        gdprintk(XENLOG_DEBUG, "incorrect word_idx(%u) is passed\n", word_idx);
> Along the lines of earlier remarks: What value does "is passed" add?
Probably not too much sense. I will drop it.

> > +static bool vaplic_emulate_store(const struct vcpu *curr, paddr_t addr,
> > +                                 uint32_t value)
> > +{
> > +    const struct domain *currd = curr->domain;
> > +    unsigned int offset = addr & APLIC_CTRL_REGION_OFFSET_MASK;
> > +
> > +    ASSERT(curr == current);
> > +
> > +    switch ( offset )
> > +    {
> > +    case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
> > +    case APLIC_CLRIP_BASE ... APLIC_CLRIP_LAST:
> > +    case APLIC_SETIE_BASE ... APLIC_SETIE_LAST:
> > +    case APLIC_CLRIE_BASE ... APLIC_CLRIE_LAST:
> > +    {
> > +        unsigned int word_idx =
> > +            regoffset_to_word_idx(offset & APLIC_SETCLR_OFFSET_MASK);
> > +
> > +        value &= generate_auth_mask(currd, word_idx);
> > +
> > +        break;
> > +    }
> > +
> > +    case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
> > +        if ( value & APLIC_SOURCECFG_D )
> > +        {
> > +            dprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
> gdprintk()?
Agree, it would be better to be gdprintk() here.

> > +            goto fail;
> > +        }
> > +
> > +        /*
> > +         * As sourcecfg register starts from 1:
> > +         *   0x0000 domaincfg
> > +         *   0x0004 sourcecfg[1]
> > +         *   0x0008 sourcecfg[2]
> > +         *    ...
> > +         *   0x0FFC sourcecfg[1023]
> > +         * It is necessary to calculate an interrupt number by subtracting
> > +         * APLIC_DOMAINCFG instead of APLIC_SOURCECFG_BASE.
> > +         */
> > +        if ( !AUTH_IRQ_BIT(currd,
> > +                           regoffset_to_word_idx(offset - APLIC_DOMAINCFG)) )
> > +            /* Interrupt not enabled, ignore it */
> > +            return true;
> > +
> > +        if ( value > APLIC_SOURCECFG_SM_LEVEL_LOW )
> > +        {
> > +            gdprintk(XENLOG_ERR,
> > +                     "value(%#x) is incorrect for sourcecfg register\n",
> > +                     value);
> > +
> > +            return true;
> > +        }
> > +
> > +        break;
> > +
> > +    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
> > +    {
> > +        struct vaplic *vaplic = to_vaplic(currd);
> > +        struct vcpu *target_vcpu;
> This, btw, is a case where I think not using "v" is warranted.
>
> > +        unsigned int guest_hart_idx = MASK_EXTR(value, APLIC_TARGET_HART_IDX);
> > +        /*
> > +         * Look at vaplic_emulate_load() for explanation why APLIC_GENMSI is
> > +         * subtracted.
> > +         */
> > +        unsigned int srcn = regoffset_to_word_idx(offset - APLIC_GENMSI);
> > +
> > +        if ( !AUTH_IRQ_BIT(currd, srcn) )
> > +            /* Interrupt not enabled, ignore it */
> > +            return true;
> > +
> > +        target_vcpu = domain_vcpu(currd, guest_hart_idx);
> > +
> > +        if ( !target_vcpu )
> > +        {
> > +            dprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
> > +
> > +            /* Ignore such writings */
> > +            return true;
> > +        }
> > +
> > +        if ( vaplic->regs.domaincfg & APLIC_DOMAINCFG_DM )
> > +        {
> > +            /*
> > +             * A non-zero guest index asks for delivery to an interrupt file of
> > +             * nested guest. The vIMSIC node has no riscv,guest-index-bits
> > +             * property, so a guest is told its harts have no guest interrupt
> > +             * files and the field is read-only zero for them. The write isn't
> > +             * rejected (that would throw away a valid hart index and EIID);
> > +             * instead the field is dropped, which is also what
> > +             * aplic_msi_target_gen() does with it when programming the h/w.
> > +             */
> > +            if ( MASK_EXTR(value, APLIC_TARGET_GUEST_IDX) )
> > +            {
> > +                printk_once(XENLOG_WARNING
> > +                            "%pd: vAPLIC target guest index != 0 is 
> > unsupported\n",
> > +                            currd);
> > +
> > +                /* Ignore such writes ... */
> > +                return true;
> > +            }
> > +
> > +            write_atomic(&vaplic->regs.target[srcn], value);
> > +
> > +            value = aplic_msi_target_gen(target_vcpu, value);
> > +        }
> > +        else
> > +        {
> > +            /*
> > +             * IPRIO is WARL and zero isn't a legal value for it, so normalize
> > +             * it once: the guest then reads back exactly what it gets.
> > +             */
> What is "reads back exactly what it gets" supposed to express? The use of
> "once" there also isn't quite clear to me.
"once" meant the substitution is done in a single place (before the 
value is stored) so nothing has to be adjusted again on the read path; 
"reads back exactly what it gets" meant the shadow copy holds the same 
legal priority that is programmed into the hardware, not the zero the 
guest wrote.
I will re-word the comment to:

/*
 * IPRIO is WARL and zero isn't a legal value for it, so a write
 * of zero is replaced by APLIC_TARGET_IPRIO_DEFAULT. This is done
 * before the value is stored, so the guest-visible copy and what
 * is programmed into the h/w hold the same legal priority and no
 * fix-up is needed when the guest reads the register back.
 */

> > +            unsigned int iprio = MASK_EXTR(value, APLIC_TARGET_IPRIO) ?:
> > +                                 APLIC_TARGET_IPRIO_DEFAULT;
> > +            unsigned long h = cpuid_to_hartid(guest_hart_idx);
> > +
> > +            value = MASK_INSR(guest_hart_idx, APLIC_TARGET_HART_IDX) |
> > +                    MASK_INSR(iprio, APLIC_TARGET_IPRIO);
> > +
> > +            write_atomic(&vaplic->regs.target[srcn], value);
> > +
> > +            value = MASK_INSR(h, APLIC_TARGET_HART_IDX) |
> > +                    MASK_INSR(iprio, APLIC_TARGET_IPRIO);
> > +        }
> > +
> > +        break;
> > +    }
> > +
> > +    case APLIC_SETIPNUM:
> > +    case APLIC_SETIPNUM_LE:
> > +    case APLIC_CLRIPNUM:
> > +    case APLIC_SETIENUM:
> > +    case APLIC_CLRIENUM:
> > +        if ( !value || !AUTH_IRQ_BIT(currd, value) )
> > +            return true;
> > +
> > +        break;
> > +
> > +    case APLIC_DOMAINCFG:
> > +    {
> > +        struct vaplic *vaplic = to_vaplic(currd);
> > +
> > +        vaplic->regs.domaincfg = APLIC_DOMAINCFG_RO |
> > +                                 (value & APLIC_DOMAINCFG_WMASK);
> > +
> > +        return true;
> > +    }
> May I suggest that you arrange case blocks (primarily) by offset? This would
> then mean for DOMAINCFG handling to move to the top, helping at least a little
> with SOURCECFG_{BASE,LAST} handling (slightly oddly) using APLIC_DOMAINCFG.
I will do that.

> > @@ -122,7 +447,29 @@ int domain_vaplic_init(struct domain *d)
> >        */
> >       d->arch.vintc->nr_virqs = guest_aplic_num_sources + 1;
> >   
> > -    return 0;
> > +    /* Slot 0 is unused: APLIC source numbering starts at 1 (see used_irqs). */
> > +    vaplic->regs.target = xvzalloc_array(uint32_t, d->arch.vintc->nr_virqs);
> > +    if ( !vaplic->regs.target )
> > +    {
> > +        d->arch.vintc = NULL;
> > +        xvfree(vaplic);
> > +
> > +        return -ENOMEM;
> > +    }
> > +
> > +    vaplic->regs_start = GUEST_APLIC_S_BASE;
> > +    vaplic->regs_size = APLIC_SIZE(d->max_vcpus);
> > +
> > +    rc = register_mmio_handler(d, &vaplic_mmio_ops,
> > +                               vaplic->regs_start, vaplic->regs_size);
> > +    if ( rc )
> > +    {
> > +        d->arch.vintc = NULL;
> > +        xvfree(vaplic->regs.target);
> > +        xvfree(vaplic);
> > +    }
> Could you perhaps arrange for it to be possible to simply call
> domain_vaplic_deinit() here (and maybe also on at least some of the earlier
> error paths)? The code above loks very similar to ...
>
> > @@ -134,5 +481,6 @@ void domain_vaplic_deinit(struct domain *d)
> >   
> >       vaplic = to_vaplic(d);
> >       d->arch.vintc = NULL;
> > +    xvfree(vaplic->regs.target);
> >       xvfree(vaplic);
> >   }
> ... what's here.
Agree, domain_vaplic_deinit() could be used instead of open-coding when 
recieved rc is handled from  register_mmio_handler() and error path above.
Thanks!

~ Oleksii