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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? (Also again naming of the parameter: Generally it wants to be "v"
or "curr"; only very special cases may use other names.)

> +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?

> @@ -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?

> --- 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.

> --- 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?

> +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()?

> +            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.

> +            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.

> @@ -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.

Jan