Re: [PATCH v3 08/39] xen/riscv: implement virtual APLIC MMIO emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2026 18:18, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/aplic.h
> +++ b/xen/arch/riscv/include/asm/aplic.h
> @@ -39,6 +39,13 @@
>  #define  APLIC_DOMAINCFG_IE             BIT(8, U)
>  #define  APLIC_DOMAINCFG_DM             BIT(2, U)
>  #define  APLIC_DOMAINCFG_BE             BIT(0, U)
> +/*
> + * The bits a write may change. Everything else, including the read-only zero
> + * bit 7 and the reserved bits, has to read back as zero, and BE is WARL and
> + * hardwired to 0 as Xen is little-endian only.
> + */
> +#define  APLIC_DOMAINCFG_WMASK          (APLIC_DOMAINCFG_IE | \
> +                                         APLIC_DOMAINCFG_DM)
>  
>  #define APLIC_SOURCECFG_BASE            0x0004
>  #define APLIC_SOURCECFG_LAST            0x0ffc
> @@ -58,6 +65,12 @@
>  #define   APLIC_SOURCECFG_SM_EDGE_FALL  0x5
>  #define   APLIC_SOURCECFG_SM_LEVEL_HIGH 0x6
>  #define   APLIC_SOURCECFG_SM_LEVEL_LOW  0x7
> +/*
> + * All other bits of sourcecfg[] are reserved and read as zero, so drop them
> + * on a write.
> + */
> +#define  APLIC_SOURCECFG_WMASK          (APLIC_SOURCECFG_D | \
> +                                         APLIC_SOURCECFG_SM)

You don't drop any bits here; that would happen at the use site of this
constant. The comment would better be worded accordingly. (The comment
a few lines up, for APLIC_DOMAINCFG_WMASK, is better in this regard.)

> @@ -89,6 +102,9 @@
>  #define  APLIC_TARGET_GUEST_IDX         GENMASK(17, 12)
>  /* Bit 11 is reserved and reads as zero */
>  #define  APLIC_TARGET_EIID              GENMASK(10, 0)
> +/* If target is in direct delivery mode (domaincfg.DM = 0) */
> +#define  APLIC_TARGET_IPRIO             GENMASK(7, 0)
> +#define   APLIC_TARGET_IPRIO_DEFAULT    1U

With this overlap of bits with EEID, should that one gain a similar
comment?

> @@ -98,6 +114,24 @@
>  #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)

As previously indicated (perhaps in other contexts): Pairs of masks and
shifts are almost always ambiguous. Hence why we have MASK_EXTR() and
MASK_INSR(), requiring only a mask: The shift value can be derived from
the mask.

> --- 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,313 @@ 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.sources[irqn].target);
> +}

I wonder whether in cases like this one, where you work with guest controlled
array indexes, you wouldn't be better off using array_{access,index}_nospec()
right away.

> +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_DOMAINCFG:
> +    {
> +        struct vaplic *vaplic = to_vaplic(currd);

In vaplic_emulate_load() you have this at function scope? Why do you do
it differently here, despite there being at least one more instance
later in the function? With this moved, the oddly placed (yet still
style conforming) figure braces also wouldn't be needed.

> +        vaplic->regs.domaincfg = APLIC_DOMAINCFG_RO |
> +                                 (value & APLIC_DOMAINCFG_WMASK);
> +
> +        return true;
> +    }
> +
> +    case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
> +        /*
> +         * Only D (bit 10) and SM (bits 2:0) are implemented, the rest of the
> +         * bits are reserved and read as zero, so ignore what a guest writes
> +         * to them.
> +         */
> +        value &= APLIC_SOURCECFG_WMASK;
> +
> +        if ( value & APLIC_SOURCECFG_D )
> +        {
> +            gdprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
> +
> +            goto fail;
> +        }
> +
> +        /*
> +         * SM is WARL, so the reserved values 0x2 and 0x3 need no handling
> +         * here: vAPLIC keeps no shadow copy of sourcecfg[], the value is
> +         * written straight to the h/w register and is read back from it, so
> +         * it is the h/w which substitutes a legal value for an illegal one.
> +         *
> +         * TODO: when vAPLIC starts to shadow sourcecfg[], the WARL behaviour
> +         * will have to be emulated here instead, e.g.:
> +         *   if ( value == 0x2 || value == 0x3 )
> +         *       value = APLIC_SOURCECFG_SM_INACTIVE;
> +         */
> +
> +        /*
> +         * 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;
> +
> +        break;
> +
> +    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_SETIPNUM:
> +    case APLIC_CLRIPNUM:
> +    case APLIC_SETIENUM:
> +    case APLIC_CLRIENUM:
> +    case APLIC_SETIPNUM_LE:
> +        if ( !value || !AUTH_IRQ_BIT(currd, value) )
> +            return true;
> +
> +        break;
> +
> +    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
> +    {
> +        struct vaplic *vaplic = to_vaplic(currd);
> +        /*
> +         * 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;
> +
> +        if ( vaplic->regs.domaincfg & APLIC_DOMAINCFG_DM )
> +        {
> +            struct vaplic_source *src = &vaplic->regs.sources[srcn];
> +            struct vcpu *target_vcpu;
> +            unsigned int guest_hart_idx =
> +                MASK_EXTR(value, APLIC_TARGET_HART_IDX);
> +            unsigned long flags;
> +            struct vimsic_state *vimsic_state;
> +
> +            target_vcpu = domain_vcpu(currd, guest_hart_idx);
> +
> +            if ( !target_vcpu )
> +            {
> +                gdprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
> +
> +                /* Ignore such writings */
> +                return true;
> +            }
> +
> +            vimsic_state = target_vcpu->arch.vimsic_state;
> +
> +            /*
> +             * A non-zero guest index asks for delivery to an interrupt file
> +             * of a 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 reads as zero for them. Such a write is
> +             * illegal and is therefore ignored as a whole: the stored copy
> +             * keeps the zero it was allocated with, so the field never needs
> +             * to be masked out here.
> +             * What ends up in the h/w register is Xen's own value anyway:
> +             * aplic_msi_target_gen() overwrites the field with
> +             * vcpu_guest_file_id() of the target vCPU.
> +             */
> +            if ( MASK_EXTR(value, APLIC_TARGET_GUEST_IDX) )
> +            {
> +                printk_once(XENLOG_WARNING
> +                            "%pd: vAPLIC target guest index != 0 is 
> unsupported\n",
> +                            currd);
> +
> +                return true;
> +            }
> +
> +            /*
> +             * src->lock keeps the stored copy and the h/w register updated
> +             * as a pair.
> +             */
> +            spin_lock_irqsave(&src->lock, flags);
> +
> +            write_atomic(&src->target, value);
> +
> +            /* IRQs are already off under src->lock. */
> +            read_lock(&vimsic_state->vsfile_lock);
> +
> +            /*
> +             * The h/w register names the hart and the guest interrupt file
> +             * MSIs are sent to, and there is none of either until the target
> +             * vCPU is given a h/w guest interrupt file. The stored copy
> +             * reaches the h/w at that point.
> +             *
> +             * TODO: once s/w guest interrupt files are supported, a vCPU 
> using
> +             * one still needs its MSIs, so the h/w register will have to be
> +             * written for it too, pointing at an interrupt file Xen receives
> +             * them in on the vCPU's behalf.
> +             */
> +            if ( vimsic_state->vsfile_cpu != CPU_NONE )
> +                aplic_hw_write_reg(offset,
> +                                   aplic_msi_target_gen(target_vcpu, value));
> +
> +            read_unlock(&vimsic_state->vsfile_lock);
> +
> +            spin_unlock_irqrestore(&src->lock, flags);
> +
> +            return true;
> +        }
> +
> +        /* printk_once(), as a guest could otherwise flood the log. */
> +        printk_once(XENLOG_WARNING
> +                    "%pd: vAPLIC direct delivery mode isn't supported\n",
> +                    currd);
> +
> +        return true;
> +    }
> +
> +    default:
> +    fail:
> +        gdprintk(XENLOG_WARNING,
> +                 "Unhandled APLIC write at offset %#x (value %#x)\n", offset,
> +                 value);
> +
> +        return false;
> +    }
> +
> +    aplic_hw_write_reg(offset, value);

I continue to struggle with this passing through of writes (reads are
passed through as well, but at least those don't alter state). Aren't
different guests going to interfere with their writes to setip, clrip,
setie, clrie, and apparently also {set,clr}i{p,e}num?

> @@ -95,6 +403,56 @@ static const struct vintc_init_ops __initconstrel 
> init_ops = {
>      .make_domu_dt_node = vaplic_make_domu_dt_node,
>  };
>  
> +static enum io_state cf_check vaplic_mmio_read(struct vcpu *v,
> +                                               mmio_info_t *info)
> +{
> +    uint32_t val;
> +
> +    ASSERT(v == current);
> +
> +    if ( info->len != sizeof(uint32_t) ||
> +         !IS_ALIGNED(info->gpa, sizeof(uint32_t)) )
> +    {
> +        gdprintk(XENLOG_DEBUG,
> +                 "VAPLIC: unaligned/wrong-width read gpa=%"PRIpaddr" 
> len=%u\n",
> +                 info->gpa, info->len);
> +        return IO_ABORT;
> +    }
> +
> +    if ( !vaplic_emulate_load(v, info->gpa, &val) )
> +        return IO_ABORT;
> +
> +    /* APLIC registers are 32-bit; zero-extend to the guest register width. 
> */
> +    info->data = val;

How do you know info->data has the size of registers as seen by guests? Plus,
how does this matter anyway? You reject info->len being other than 4, which
means the originating insn will necessarily have been a 4-byte load. What
happens to the upper bits (if there are any) depends on the behavior of the
insn that's being emulated, not on what you do here when storing into
info->data.

Jan