[PATCH v1 00/17] [RISC-V] virtual interrupt controller (vAPLIC/vIMSIC) support

[PATCH v1 00/17] [RISC-V] virtual interrupt controller (vAPLIC/vIMSIC) support

[Oleksii Kurochko]

Hi all,

This series adds the initial virtual interrupt controller (vINTC) support
for RISC-V guests in Xen, based on the Advanced Interrupt Architecture
(AIA): a virtual APLIC (vAPLIC) in MSI mode backed by a virtual IMSIC
(vIMSIC) using hardware guest interrupt files.

Rather than emulating APLIC in direct-delivery mode (which requires
trap-and-emulate for every interrupt and is costly), the series targets
IMSIC from the start. AIA lets a hart implement several "guest interrupt
files" (up to GEILEN), so external interrupts can be delivered to a vCPU
directly by hardware via the VGEIN field of hstatus, without a hypervisor
round-trip. Xen only has to emulate the APLIC MMIO programming interface
and route the guest's intent onto the physical MSI topology; interrupt
delivery itself stays in hardware.

The work breaks down into a few logical blocks:

Physical APLIC/AIA groundwork (patches 1-3)
  - Correctly track IRQ_DISABLED across runtime enable/disable and order
    the ->status update against the IMSIC CSR write.
  - Per-pCPU VGEIN (guest interrupt file) allocator: assign/release a
    hardware guest file to a vCPU and program hstatus.VGEIN.
  - Add the missing APLIC register offsets/masks needed by both the
    physical and virtual APLIC code (no functional change).

Device-agnostic MMIO dispatch + vAPLIC emulation (patches 4-5)
  - A per-domain MMIO handler table modelled on Arm's framework, so
    emulated devices self-register their GPA ranges and the fault path
    stays agnostic via a single try_handle_mmio() entry point.
  - vAPLIC MMIO read/write emulation. Writes are gated by the domain's
    authorised-IRQ bitmap so a guest cannot touch interrupts it does not
    own, and TARGET writes are translated from virtual to physical
    hart/guest-file indices. Delegation (SOURCECFG.D) is not yet
    supported.

vIMSIC guest interrupt files and state (patches 6-9)
  - Stage-2 map a vCPU's physical guest interrupt file to the fixed
    per-vCPU GPA page the guest expects at offset 0.
  - vcpu_aia_init(): assign a VGEIN, map its guest file and record the
    IMSIC state as a consistent unit.
  - IMSIC state save/restore (currently tracking which pCPU owns the
    guest file, needed because the pCPU id is part of the MSI address).
  - has_msi_support() helper to decide whether IMSIC state has to be
    saved/restored.

vINTC state save/restore plumbing (patches 10-11)
  - vintc_state_{save,restore}() wrappers over new store/restore hooks in
    struct vintc_ops, and the vAPLIC implementation of those hooks. No
    callers are wired up yet; the context-switch calls arrive with vCPU
    context switch support.

Trap and instruction emulation infrastructure (patches 12-17)
  - Extend the exception-table format with type/data fields and add
    EX_TYPE_TRAP_INFO so fixups can capture sepc/scause/stval.
  - riscv_vcpu_unpriv_read() (HLV/HLVX) to read guest memory/instructions
    safely, and riscv_vcpu_trap_redirect() to forward a synchronous trap
    back into the guest's VS-mode handler.
  - A guest page-fault handler that decodes the trapped load/store
    instruction (HTINST, or an unprivileged fetch as a fallback) and
    dispatches the MMIO access through try_handle_mmio().

Note on versioning: the series is posted as v1 as a freshly split-out
series, but several patches carry v2/v3 changelogs because they were
previously circulated as part of a larger another patches [1] from which the
current depends.

CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2690874319

[1] https://lore.kernel.org/xen-devel/cover.1784559209.git.oleksii.kurochko@gmail.com/T/#t

Oleksii Kurochko (17):
  xen/riscv: manage IRQ_DISABLED flag in APLIC irq enable/disable
    callbacks
  xen/riscv: add basic VGEIN management for AIA guests
  xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h
  xen/riscv: introduce device-agnostic MMIO emulation dispatch
  xen/riscv: implement virtual APLIC MMIO emulation
  xen/riscv: map IMSIC interrupt file for vCPUs
  xen/riscv: introduce vCPU AIA initialization
  xen/riscv: add IMSIC state save/restore
  xen/riscv: add helper to check APLIC MSI mode
  xen/riscv: introduce vintc_state_{save,restore}()
  xen/riscv: add vAPLIC state save/restore hooks
  xen/riscv: extend exception tables with type and data fields
  xen/riscv: add unprivileged guest memory read helper
  xen/riscv: add guest page fault handling stub
  xen/riscv: implement trap redirection to a guest
  xen/riscv: add guest load emulation for trapped MMIO accesses
  xen/riscv: add guest store emulation for trapped MMIO accesses

 xen/arch/riscv/Makefile                   |   1 +
 xen/arch/riscv/aia.c                      | 175 ++++++++++++
 xen/arch/riscv/aplic-priv.h               |   2 +
 xen/arch/riscv/aplic.c                    |  83 +++++-
 xen/arch/riscv/domain.c                   |   4 +
 xen/arch/riscv/extable.c                  |  60 +++-
 xen/arch/riscv/guestcopy.c                | 145 ++++++++++
 xen/arch/riscv/imsic.c                    |  99 +++++++
 xen/arch/riscv/include/asm/aia.h          |  10 +
 xen/arch/riscv/include/asm/aplic.h        |  61 +++++
 xen/arch/riscv/include/asm/domain.h       |   3 +
 xen/arch/riscv/include/asm/extable.h      |  62 +++--
 xen/arch/riscv/include/asm/gpr-num.h      |  33 +++
 xen/arch/riscv/include/asm/guest_access.h |   8 +
 xen/arch/riscv/include/asm/imsic.h        |  17 ++
 xen/arch/riscv/include/asm/intc.h         |   9 +
 xen/arch/riscv/include/asm/mmio.h         |  63 +++++
 xen/arch/riscv/include/asm/processor.h    |   2 +
 xen/arch/riscv/include/asm/traps.h        |  12 +
 xen/arch/riscv/include/asm/vaplic.h       |   6 +
 xen/arch/riscv/intc.c                     |  14 +
 xen/arch/riscv/mmio.c                     | 145 ++++++++++
 xen/arch/riscv/traps.c                    | 279 +++++++++++++++++++
 xen/arch/riscv/vaplic.c                   | 319 ++++++++++++++++++++++
 24 files changed, 1590 insertions(+), 22 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/gpr-num.h
 create mode 100644 xen/arch/riscv/include/asm/mmio.h
 create mode 100644 xen/arch/riscv/mmio.c

-- 
2.54.0

[PATCH v1 01/17] xen/riscv: manage IRQ_DISABLED flag in APLIC irq enable/disable callbacks

[Oleksii Kurochko]

desc->status is only set once during setup_irq(), but interrupts can be
enabled/disabled at runtime, so update it in the corresponding callbacks.

For the purposes of the FENCE instruction, CSR read accesses are
classified as device input (I) and CSR write accesses as device output
(O), while the barriers used by spin locks (fence rw,rw) only order
normal memory accesses. An explicit wmb() (fence ow,ow) is therefore
added in aplic_irq_{enable,disable}() to order the desc->status update
with respect to the IMSIC CSR write.

Fixes: d4676a1398bc5 ("xen/riscv: implementation of aplic and imsic operations")
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v3:
 - Add the comment above wmb() in aplic_irq_enable() and add wmb() also in
   *_disable().
 - Update the commit message: drop info about wmb() and add information why
   it is safe to drop update of desc->status from setup_irq().
---
Changes in v2:
 - New patch
---
---
 xen/arch/riscv/aplic.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index fe319041cece..3681f0669efb 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -136,6 +136,15 @@ static void cf_check aplic_irq_enable(struct irq_desc *desc)
 
     spin_lock(&aplic.lock);
 
+    desc->status &= ~IRQ_DISABLED;
+    /*
+     * wmb() (fence ow,ow) orders the ->status memory write (w) before the
+     * CSR write inside imsic_irq_enable() (device output, o on RISC-V).
+     * arch_lock_release_barrier() uses fence rw,rw which does not cover
+     * device output (o), so wmb() is required to close that gap.
+     */
+    wmb();
+
     /* Enable interrupt in IMSIC */
     imsic_irq_enable(desc->irq);
 
@@ -163,6 +172,16 @@ static void cf_check aplic_irq_disable(struct irq_desc *desc)
 
     /* Disable interrupt in IMSIC */
     imsic_irq_disable(desc->irq);
+    /*
+     * wmb() (fence ow,ow) ensures the CSR write (device output, o) inside
+     * imsic_irq_disable() is globally visible before ->status is marked
+     * IRQ_DISABLED. imsic_irq_disable()'s spin_unlock uses fence rw,rw
+     * which does not order device output (o) writes before subsequent
+     * memory writes (w), so an explicit wmb() is needed here.
+     */
+    wmb();
+
+    desc->status |= IRQ_DISABLED;
 
     spin_unlock(&aplic.lock);
 }
-- 
2.54.0

Re: [PATCH v1 01/17] xen/riscv: manage IRQ_DISABLED flag in APLIC irq enable/disable callbacks

[Jan Beulich]

On 20.07.2026 18:01, Oleksii Kurochko wrote:
> desc->status is only set once during setup_irq(), but interrupts can be
> enabled/disabled at runtime, so update it in the corresponding callbacks.
> 
> For the purposes of the FENCE instruction, CSR read accesses are
> classified as device input (I) and CSR write accesses as device output
> (O), while the barriers used by spin locks (fence rw,rw) only order
> normal memory accesses. An explicit wmb() (fence ow,ow) is therefore
> added in aplic_irq_{enable,disable}() to order the desc->status update
> with respect to the IMSIC CSR write.
> 
> Fixes: d4676a1398bc5 ("xen/riscv: implementation of aplic and imsic operations")
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Oleksii Kurochko]

It was decided to add support for IMSIC from the start instead of having APLIC
operate in direct delivery mode, as it requires a trap-and-emulation approach,
which is not optimal from a performance standpoint.

AIA provides a hardware-accelerated mechanism for delivering external
interrupts to domains via "guest interrupt files" located in IMSIC.
A single physical hart can implement multiple such files (up to GEILEN),
allowing several virtual harts to receive interrupts directly from hardware.

Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
for systems implementing AIA specification. Each CPU maintains
a bitmap describing which guest interrupt files are currently in use.

Add helpers to initialize the bitmap based on the number of available
guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
when no longer needed. When assigning a VGEIN, the corresponding value
is written to the VGEIN field of the guest hstatus register so that
VS-level external interrupts are delivered from the selected interrupt
file.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v3:
 - Rephrase per-CPU comment from "Bitmap for each physical cpus..." to
   "VGEIN control structure for each physical CPU...".
 - Use %u for cpu in printk("cpu%u.geilen=%u\n").
 - Fix missing \n in "AIA: failed to init vgein for CPU%u\n".
 - Shorten dprintk message to "vgein_init() failed: %d\n".
 - Use %u instead of %d for v->processor in gprintk() calls.
 - Drop update of ->hstatus in vgein_{assign,release} and let the caller
   to do that. For example, it could be useful in the case of migration
   vCPU from one pCPU to another and so vcpu->hstatus will be updated as a
   last step of this process.
 - Fill ->owners in vgein_{assign,release} just from the start.
---
Changes in v2:
 - add static for defintion of vgein_bmp;
 - Drop declarartion of vgein_bmp from aia.h.
 - Move declaration of 'struct vgein_bmp' from aia.h to aia.c as all the
   management is inside aia.c.
 - Instead of decrement of vgein->geilen just update the wait how it is
   initialized.
 - Return -EOPNOTSUPP in vgein_init() instead of BUG_ON().
 - Use %u to print unsigned int.
 - make bmp field in vgein_bmp not a pointer.
 - allocate owners dynamically.
 - Drop unnessary blank lines.
 - use find_first_zero_bit() instead of bitmap_weight() to find a free slot
   for vgein number.
 - Drop the section number for the comment.
 - Start to search from bitnum 1 for free vgein_id, as bitnum 0 is reserved to
   tell that no guest extrenal interrupt number is used. Thereby drop vgein_id++
   at the end of vgein_assign().
 - s/bitmap_set/__set_bit.
 - s/bitmap_clear/__clear_bit.
 - as vgein_init() is needed to be invoked once per CPU being brought up, drop
   __init for it.
 - Return vgein_id == 0 if vgein_id is higher then maximun supported by h/w
   VGEIN.
 - Add check in vgein_relase() that vgein is 0 and if it is there is nothing
   is needed to do.
 - Use gdprintk instead of printk() in vgein_{assign,release}.
 - Add the claryfing comment above geilen field.
 - Drop ASSERT in vgein_assign() and return just vgein_id = 0 in the case when
   there is no aviablable h/w VGEINs.
 - Make vgein_init() static.
---
---
 xen/arch/riscv/aia.c             | 145 +++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/aia.h |   8 ++
 2 files changed, 153 insertions(+)

diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
index e31c9c2d24b6..4f7f46f58f0b 100644
--- a/xen/arch/riscv/aia.c
+++ b/xen/arch/riscv/aia.c
@@ -1,11 +1,33 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/bitmap.h>
+#include <xen/cpu.h>
 #include <xen/errno.h>
 #include <xen/init.h>
 #include <xen/sections.h>
+#include <xen/sched.h>
+#include <xen/spinlock.h>
 #include <xen/types.h>
+#include <xen/xvmalloc.h>
 
+#include <asm/aia.h>
 #include <asm/cpufeature.h>
+#include <asm/csr.h>
+#include <asm/current.h>
+
+struct vgein_ctrl {
+    unsigned long bmp;
+    spinlock_t lock;
+    struct vcpu **owners;
+    /* The least-significant bits are implemented first, apart from bit 0 */
+    unsigned int geilen;
+};
+
+/*
+ * VGEIN control structure for each physical CPU to track which VS (guest)
+ * interrupt file IDs are in use.
+ */
+static DEFINE_PER_CPU(struct vgein_ctrl, vgein);
 
 static bool __ro_after_init _aia_usable;
 
@@ -14,10 +36,133 @@ bool aia_usable(void)
     return _aia_usable;
 }
 
+static int vgein_init(unsigned int cpu)
+{
+    struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
+
+    csr_write(CSR_HGEIE, -1UL);
+    vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);
+    csr_write(CSR_HGEIE, 0);
+
+    printk("cpu%u.geilen=%u\n", cpu, vgein->geilen);
+
+    if ( !vgein->geilen )
+        return -EOPNOTSUPP;
+
+    vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen);
+    if ( !vgein->owners )
+        return -ENOMEM;
+
+    spin_lock_init(&vgein->lock);
+
+    return 0;
+}
+
+static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long action,
+                        void *hcpu)
+{
+    unsigned int cpu = (unsigned long)hcpu;
+    int rc = 0;
+
+    switch ( action )
+    {
+    case CPU_STARTING:
+        rc = vgein_init(cpu);
+        if ( rc )
+            printk("AIA: failed to init vgein for CPU%u\n", cpu);
+        break;
+    }
+
+    return notifier_from_errno(rc);
+}
+
+static struct notifier_block cpu_nfb = {
+    .notifier_call = cpu_callback,
+};
+
 void __init aia_init(void)
 {
+    int rc;
+
     if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
+    {
+        dprintk(XENLOG_WARNING, "SSAIA isn't present in riscv,isa\n");
         return;
+    }
+
+    if ( (rc = vgein_init(0)) )
+    {
+        dprintk(XENLOG_ERR, "vgein_init() failed: %d\n", rc);
+        return;
+    }
 
     _aia_usable = true;
+
+    register_cpu_notifier(&cpu_nfb);
+}
+
+unsigned int vgein_assign(struct vcpu *v)
+{
+    unsigned int vgein_id;
+    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
+    unsigned long *bmp = &vgein->bmp;
+    unsigned long flags;
+
+    if ( !vgein->geilen )
+        return 0;
+
+    spin_lock_irqsave(&vgein->lock, flags);
+    /*
+     * The vgein_id shouldn't be zero, as it will indicate that no guest
+     * external interrupt source is selected for VS-level external interrupts
+     * according to RISC-V privileged spec:
+     *   Hypervisor Status Register (hstatus) in RISC-V privileged spec:
+     *
+     *   The VGEIN (Virtual Guest External Interrupt Number) field selects
+     *   a guest external interrupt source for VS-level external interrupts.
+     *   VGEIN is a WLRL field that must be able to hold values between zero
+     *   and the maximum guest external interrupt number (known as GEILEN),
+     *   inclusive.
+     *   When VGEIN=0, no guest external interrupt source is selected for
+     *   VS-level external interrupts.
+     *
+     * So start to search from bit number 1.
+     */
+    vgein_id = find_next_zero_bit(bmp, vgein->geilen + 1, 1);
+
+    if ( vgein_id > vgein->geilen )
+        vgein_id = 0;
+    else
+    {
+        __set_bit(vgein_id, bmp);
+        vgein->owners[vgein_id] = v;
+    }
+
+    spin_unlock_irqrestore(&vgein->lock, flags);
+
+#ifdef VGEIN_DEBUG
+    gprintk(XENLOG_DEBUG, "%s: %pv: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
+           __func__, v, vgein_id, v->processor, *bmp);
+#endif
+
+    return vgein_id;
+}
+
+void vgein_release(struct vcpu *v, unsigned int vgein_id)
+{
+    unsigned long flags;
+    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
+
+    if ( !vgein_id )
+        return;
+
+    spin_lock_irqsave(&vgein->lock, flags);
+    __clear_bit(vgein_id, &vgein->bmp);
+    vgein->owners[vgein_id] = NULL;
+    spin_unlock_irqrestore(&vgein->lock, flags);
+
+#ifdef VGEIN_DEBUG
+    gprintk(XENLOG_DEBUG, "%s: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
+           __func__, vgein_id, v->processor, vgein->bmp);
+#endif
 }
diff --git a/xen/arch/riscv/include/asm/aia.h b/xen/arch/riscv/include/asm/aia.h
index aaa4bf91fc75..c67be0069a1d 100644
--- a/xen/arch/riscv/include/asm/aia.h
+++ b/xen/arch/riscv/include/asm/aia.h
@@ -3,8 +3,16 @@
 #ifndef RISCV_AIA_H
 #define RISCV_AIA_H
 
+#include <xen/percpu.h>
+#include <xen/spinlock.h>
+
+struct vcpu;
+
 bool aia_usable(void);
 
 void aia_init(void);
 
+unsigned int vgein_assign(struct vcpu *v);
+void vgein_release(struct vcpu *v, unsigned int vgein_id);
+
 #endif /* RISCV_AIA_H */
-- 
2.54.0

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Jan Beulich]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> It was decided to add support for IMSIC from the start instead of having APLIC
> operate in direct delivery mode, as it requires a trap-and-emulation approach,
> which is not optimal from a performance standpoint.
> 
> AIA provides a hardware-accelerated mechanism for delivering external
> interrupts to domains via "guest interrupt files" located in IMSIC.
> A single physical hart can implement multiple such files (up to GEILEN),
> allowing several virtual harts to receive interrupts directly from hardware.
> 
> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
> for systems implementing AIA specification. Each CPU maintains
> a bitmap describing which guest interrupt files are currently in use.
> 
> Add helpers to initialize the bitmap based on the number of available
> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
> when no longer needed. When assigning a VGEIN, the corresponding value
> is written to the VGEIN field of the guest hstatus register so that
> VS-level external interrupts are delivered from the selected interrupt
> file.

And when exactly is this "assignment" intended to occur? vgein_assign() and
vgein_release() have no callers here, so this remains entirely unclear.

> @@ -14,10 +36,133 @@ bool aia_usable(void)
>      return _aia_usable;
>  }
>  
> +static int vgein_init(unsigned int cpu)
> +{
> +    struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
> +
> +    csr_write(CSR_HGEIE, -1UL);
> +    vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);
> +    csr_write(CSR_HGEIE, 0);
> +
> +    printk("cpu%u.geilen=%u\n", cpu, vgein->geilen);

At most dprintk(), I'd say. Better drop altogether.

> +    if ( !vgein->geilen )
> +        return -EOPNOTSUPP;
> +
> +    vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen);
> +    if ( !vgein->owners )
> +        return -ENOMEM;
> +
> +    spin_lock_init(&vgein->lock);
> +
> +    return 0;
> +}
> +
> +static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long action,

Nit: Line length.

> +                        void *hcpu)

Nit: Indentation.

> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +    int rc = 0;
> +
> +    switch ( action )
> +    {
> +    case CPU_STARTING:
> +        rc = vgein_init(cpu);
> +        if ( rc )
> +            printk("AIA: failed to init vgein for CPU%u\n", cpu);
> +        break;
> +    }
> +
> +    return notifier_from_errno(rc);
> +}

Where's the freeing of the allocation vgein_init(), when CPU bringup fails
or a CPU was brought down?

> +static struct notifier_block cpu_nfb = {
> +    .notifier_call = cpu_callback,
> +};
> +
>  void __init aia_init(void)
>  {
> +    int rc;
> +
>      if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
> +    {
> +        dprintk(XENLOG_WARNING, "SSAIA isn't present in riscv,isa\n");
>          return;
> +    }
> +
> +    if ( (rc = vgein_init(0)) )
> +    {
> +        dprintk(XENLOG_ERR, "vgein_init() failed: %d\n", rc);
> +        return;
> +    }
>  
>      _aia_usable = true;
> +
> +    register_cpu_notifier(&cpu_nfb);
> +}
> +
> +unsigned int vgein_assign(struct vcpu *v)
> +{
> +    unsigned int vgein_id;
> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> +    unsigned long *bmp = &vgein->bmp;
> +    unsigned long flags;
> +
> +    if ( !vgein->geilen )
> +        return 0;
> +
> +    spin_lock_irqsave(&vgein->lock, flags);

Because it's unclear where this is to be called from, it's also unclear whether
a lock is needed here (and if so whether a plain spin lock is appropriate).

> +    /*
> +     * The vgein_id shouldn't be zero, as it will indicate that no guest
> +     * external interrupt source is selected for VS-level external interrupts
> +     * according to RISC-V privileged spec:
> +     *   Hypervisor Status Register (hstatus) in RISC-V privileged spec:
> +     *
> +     *   The VGEIN (Virtual Guest External Interrupt Number) field selects
> +     *   a guest external interrupt source for VS-level external interrupts.
> +     *   VGEIN is a WLRL field that must be able to hold values between zero
> +     *   and the maximum guest external interrupt number (known as GEILEN),
> +     *   inclusive.
> +     *   When VGEIN=0, no guest external interrupt source is selected for
> +     *   VS-level external interrupts.
> +     *
> +     * So start to search from bit number 1.
> +     */
> +    vgein_id = find_next_zero_bit(bmp, vgein->geilen + 1, 1);
> +
> +    if ( vgein_id > vgein->geilen )
> +        vgein_id = 0;
> +    else
> +    {
> +        __set_bit(vgein_id, bmp);
> +        vgein->owners[vgein_id] = v;

Again somewhat related to is being unclear how the function is going to be used,
it also remains unclear what ->owners[] is going to be needed for. Right now the
array is only ever written to.

> +    }
> +
> +    spin_unlock_irqrestore(&vgein->lock, flags);
> +
> +#ifdef VGEIN_DEBUG
> +    gprintk(XENLOG_DEBUG, "%s: %pv: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
> +           __func__, v, vgein_id, v->processor, *bmp);
> +#endif
> +
> +    return vgein_id;
> +}
> +
> +void vgein_release(struct vcpu *v, unsigned int vgein_id)
> +{
> +    unsigned long flags;
> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> +
> +    if ( !vgein_id )
> +        return;
> +
> +    spin_lock_irqsave(&vgein->lock, flags);
> +    __clear_bit(vgein_id, &vgein->bmp);
> +    vgein->owners[vgein_id] = NULL;

If already you track the vCPU, also assert that prior to clearing the array
slot it has the expected value? For the bit being cleared, maybe also

    if ( !__test_and_clear_bit(vgein_id, &vgein->bmp) )
        ASSERT_UNREACHABLE();

? Yet as said - much remains unclear without knowing how all of this is
meant to be used.

Jan

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Oleksii Kurochko]

On 7/27/26 5:41 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> It was decided to add support for IMSIC from the start instead of having APLIC
>> operate in direct delivery mode, as it requires a trap-and-emulation approach,
>> which is not optimal from a performance standpoint.
>>
>> AIA provides a hardware-accelerated mechanism for delivering external
>> interrupts to domains via "guest interrupt files" located in IMSIC.
>> A single physical hart can implement multiple such files (up to GEILEN),
>> allowing several virtual harts to receive interrupts directly from hardware.
>>
>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>> for systems implementing AIA specification. Each CPU maintains
>> a bitmap describing which guest interrupt files are currently in use.
>>
>> Add helpers to initialize the bitmap based on the number of available
>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>> when no longer needed. When assigning a VGEIN, the corresponding value
>> is written to the VGEIN field of the guest hstatus register so that
>> VS-level external interrupts are delivered from the selected interrupt
>> file.
> 
> And when exactly is this "assignment" intended to occur? vgein_assign() and
> vgein_release() have no callers here, so this remains entirely unclear.

[A] Agreed, I should have added that information to the commit message:

VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU 
execution context (in continue_new_vcpu()) and is re-assigned during 
vCPU migration from one pCPU to another.

VGEIN is released (via vgein_release()) on the old pCPU during migration.


> 
>> @@ -14,10 +36,133 @@ bool aia_usable(void)
>>       return _aia_usable;
>>   }
>>   
>> +static int vgein_init(unsigned int cpu)
>> +{
>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
>> +
>> +    csr_write(CSR_HGEIE, -1UL);
>> +    vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);
>> +    csr_write(CSR_HGEIE, 0);
>> +
>> +    printk("cpu%u.geilen=%u\n", cpu, vgein->geilen);
> 
> At most dprintk(), I'd say. Better drop altogether.

I will drop it.

> 
>> +    if ( !vgein->geilen )
>> +        return -EOPNOTSUPP;
>> +
>> +    vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen);
>> +    if ( !vgein->owners )
>> +        return -ENOMEM;
>> +
>> +    spin_lock_init(&vgein->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long action,
> 
> Nit: Line length.
> 
>> +                        void *hcpu)
> 
> Nit: Indentation.
> 
>> +{
>> +    unsigned int cpu = (unsigned long)hcpu;
>> +    int rc = 0;
>> +
>> +    switch ( action )
>> +    {
>> +    case CPU_STARTING:
>> +        rc = vgein_init(cpu);
>> +        if ( rc )
>> +            printk("AIA: failed to init vgein for CPU%u\n", cpu);
>> +        break;
>> +    }
>> +
>> +    return notifier_from_errno(rc);
>> +}
> 
> Where's the freeing of the allocation vgein_init(), when CPU bringup fails
> or a CPU was brought down?
I'll add the following:

case CPU_UP_CANCELED:
case CPU_DEAD:
     vgein_free(cpu);
     break;

and:

static void vgein_free(unsigned int cpu)
{
     struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);

     ASSERT(!vgein->bmp);

     vgein->geilen = 0;
     XVFREE(vgein->owners);
}

I'm also wondering whether vgein_init() should be moved to 
CPU_UP_PREPARE. If vgein_init() fails in CPU_STARTING, the hypervisor 
will stop instead of simply ignoring the CPU.

However, in CPU_UP_PREPARE we don't yet know the value of GEILEN, which 
is needed to allocate vgein->owners. As I understand it, CPU_UP_PREPARE 
is not executed on the CPU that is being brought up.


> 
>> +static struct notifier_block cpu_nfb = {
>> +    .notifier_call = cpu_callback,
>> +};
>> +
>>   void __init aia_init(void)
>>   {
>> +    int rc;
>> +
>>       if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
>> +    {
>> +        dprintk(XENLOG_WARNING, "SSAIA isn't present in riscv,isa\n");
>>           return;
>> +    }
>> +
>> +    if ( (rc = vgein_init(0)) )
>> +    {
>> +        dprintk(XENLOG_ERR, "vgein_init() failed: %d\n", rc);
>> +        return;
>> +    }
>>   
>>       _aia_usable = true;
>> +
>> +    register_cpu_notifier(&cpu_nfb);
>> +}
>> +
>> +unsigned int vgein_assign(struct vcpu *v)
>> +{
>> +    unsigned int vgein_id;
>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>> +    unsigned long *bmp = &vgein->bmp;
>> +    unsigned long flags;
>> +
>> +    if ( !vgein->geilen )
>> +        return 0;
>> +
>> +    spin_lock_irqsave(&vgein->lock, flags);
> 
> Because it's unclear where this is to be called from, it's also unclear whether
> a lock is needed here (and if so whether a plain spin lock is appropriate).

Based on what I wrote in [A] above a lock is defintely needed as it 
could be that vgein_release() is called for old pCPU during migration 
and at the same time old pCPU could call vgein_assign() so we want to 
keep vgein bitmap consistent.

Regarding why _irqsave() it is mostly connected to ...

> 
>> +    /*
>> +     * The vgein_id shouldn't be zero, as it will indicate that no guest
>> +     * external interrupt source is selected for VS-level external interrupts
>> +     * according to RISC-V privileged spec:
>> +     *   Hypervisor Status Register (hstatus) in RISC-V privileged spec:
>> +     *
>> +     *   The VGEIN (Virtual Guest External Interrupt Number) field selects
>> +     *   a guest external interrupt source for VS-level external interrupts.
>> +     *   VGEIN is a WLRL field that must be able to hold values between zero
>> +     *   and the maximum guest external interrupt number (known as GEILEN),
>> +     *   inclusive.
>> +     *   When VGEIN=0, no guest external interrupt source is selected for
>> +     *   VS-level external interrupts.
>> +     *
>> +     * So start to search from bit number 1.
>> +     */
>> +    vgein_id = find_next_zero_bit(bmp, vgein->geilen + 1, 1);
>> +
>> +    if ( vgein_id > vgein->geilen )
>> +        vgein_id = 0;
>> +    else
>> +    {
>> +        __set_bit(vgein_id, bmp);
>> +        vgein->owners[vgein_id] = v;
> 
> Again somewhat related to is being unclear how the function is going to be used,
> it also remains unclear what ->owners[] is going to be needed for. Right now the
> array is only ever written to.

->owners[] is used in IRQ context to wake up a vCPU. For example, if a 
vCPU has been descheduled, we need to set the corresponding CSR_HGEIE[] 
bit so that when an interrupt associated with that vCPU occurs, it traps 
into the hgei_interrupt() handler, which then wakes the vCPU. (all of 
that isn't introduced now but I thought it would be useful to track 
->owners[] just from the start).

Since ->owners[] is accessed from both IRQ-safe (hgei_interrupt()) and 
IRQ-unsafe (vCPU migration) contexts, we specifically need the 
_irqsave() variant of the lock.

To make this clearer, I'll add the following to the commit message (if 
that helps):
```
Along with the bitmap, track which vCPU owns each guest interrupt file
id. Nothing consumes this yet, but it is filled in from the start as
the owner is what a guest external interrupt handler needs: a guest
interrupt file stays enabled in hgeie while its vCPU is descheduled, so
an interrupt targeting that file traps to Xen, which then has to find
the vCPU it belongs to in order to wake it up.

While the tracking is per-CPU data, it isn't accessed only locally: a
guest interrupt file belongs to the pCPU a vCPU is going to run on, so
it is allocated and released by whichever CPU is handling the vCPU at
the time: the release side is even passed the target CPU explicitly.
Hence a lock is needed. It has to be the IRQ-safe variant, as the
tracking is also going to be read from interrupt context on the CPU
owning it.
```
> 
>> +    }
>> +
>> +    spin_unlock_irqrestore(&vgein->lock, flags);
>> +
>> +#ifdef VGEIN_DEBUG
>> +    gprintk(XENLOG_DEBUG, "%s: %pv: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
>> +           __func__, v, vgein_id, v->processor, *bmp);
>> +#endif
>> +
>> +    return vgein_id;
>> +}
>> +
>> +void vgein_release(struct vcpu *v, unsigned int vgein_id)
>> +{
>> +    unsigned long flags;
>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>> +
>> +    if ( !vgein_id )
>> +        return;
>> +
>> +    spin_lock_irqsave(&vgein->lock, flags);
>> +    __clear_bit(vgein_id, &vgein->bmp);
>> +    vgein->owners[vgein_id] = NULL;
> 
> If already you track the vCPU, also assert that prior to clearing the array
> slot it has the expected value? For the bit being cleared, maybe also
> 
>      if ( !__test_and_clear_bit(vgein_id, &vgein->bmp) )
>          ASSERT_UNREACHABLE();
> 
> ? Yet as said - much remains unclear without knowing how all of this is
> meant to be used.

It makes sense. I will add that.

Thanks.

~ Oleksii

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Jan Beulich]

On 29.07.2026 16:55, Oleksii Kurochko wrote:
> On 7/27/26 5:41 PM, Jan Beulich wrote:
>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>> It was decided to add support for IMSIC from the start instead of having APLIC
>>> operate in direct delivery mode, as it requires a trap-and-emulation approach,
>>> which is not optimal from a performance standpoint.
>>>
>>> AIA provides a hardware-accelerated mechanism for delivering external
>>> interrupts to domains via "guest interrupt files" located in IMSIC.
>>> A single physical hart can implement multiple such files (up to GEILEN),
>>> allowing several virtual harts to receive interrupts directly from hardware.
>>>
>>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>>> for systems implementing AIA specification. Each CPU maintains
>>> a bitmap describing which guest interrupt files are currently in use.
>>>
>>> Add helpers to initialize the bitmap based on the number of available
>>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>>> when no longer needed. When assigning a VGEIN, the corresponding value
>>> is written to the VGEIN field of the guest hstatus register so that
>>> VS-level external interrupts are delivered from the selected interrupt
>>> file.
>>
>> And when exactly is this "assignment" intended to occur? vgein_assign() and
>> vgein_release() have no callers here, so this remains entirely unclear.
> 
> [A] Agreed, I should have added that information to the commit message:
> 
> VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU 
> execution context (in continue_new_vcpu()) and is re-assigned during 
> vCPU migration from one pCPU to another.
> 
> VGEIN is released (via vgein_release()) on the old pCPU during migration.

That is, state of that vCPU is held in hardware for perhaps an extended
period of time after the vCPU was last de-scheduled. That's a fair
optimization (we do something similar on x86, albeit that has been
increasingly under question lately). However, doesn't this then require
sync_local_execstate() to become non-empty?

Furthermore, rather than having vgein_assign() fail when
find_next_zero_bit() fails to find an available ID, shouldn't you release
some other vCPU's ID, making it available for re-use?

>>> +static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long action,
>>> +                        void *hcpu)
>>> +{
>>> +    unsigned int cpu = (unsigned long)hcpu;
>>> +    int rc = 0;
>>> +
>>> +    switch ( action )
>>> +    {
>>> +    case CPU_STARTING:
>>> +        rc = vgein_init(cpu);
>>> +        if ( rc )
>>> +            printk("AIA: failed to init vgein for CPU%u\n", cpu);
>>> +        break;
>>> +    }
>>> +
>>> +    return notifier_from_errno(rc);
>>> +}
>>
>> Where's the freeing of the allocation vgein_init(), when CPU bringup fails
>> or a CPU was brought down?
> I'll add the following:
> 
> case CPU_UP_CANCELED:
> case CPU_DEAD:
>      vgein_free(cpu);
>      break;
> 
> and:
> 
> static void vgein_free(unsigned int cpu)
> {
>      struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
> 
>      ASSERT(!vgein->bmp);

Does this hold in all cases? What migrates vCPU-s off of a pCPU going down?
IOW aren't you introducing an ordering problem between your notifier handler
and the scheduler's?

>      vgein->geilen = 0;
>      XVFREE(vgein->owners);
> }
> 
> I'm also wondering whether vgein_init() should be moved to 
> CPU_UP_PREPARE. If vgein_init() fails in CPU_STARTING, the hypervisor 
> will stop instead of simply ignoring the CPU.
> 
> However, in CPU_UP_PREPARE we don't yet know the value of GEILEN, which 
> is needed to allocate vgein->owners. As I understand it, CPU_UP_PREPARE 
> is not executed on the CPU that is being brought up.

But there's an upper bound, isn't there? Use that for preliminary allocation,
and re-alloc (best effort) from CPU_ONLINE?

Yet then I continue to question the presence of this array in the first place.
Something similar isn't needed elsewhere (afaik), and its intended use (as
said) doesn't become obvious here.

>>> +unsigned int vgein_assign(struct vcpu *v)
>>> +{
>>> +    unsigned int vgein_id;
>>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>>> +    unsigned long *bmp = &vgein->bmp;
>>> +    unsigned long flags;
>>> +
>>> +    if ( !vgein->geilen )
>>> +        return 0;
>>> +
>>> +    spin_lock_irqsave(&vgein->lock, flags);
>>
>> Because it's unclear where this is to be called from, it's also unclear whether
>> a lock is needed here (and if so whether a plain spin lock is appropriate).
> 
> Based on what I wrote in [A] above a lock is defintely needed as it 
> could be that vgein_release() is called for old pCPU during migration 
> and at the same time old pCPU could call vgein_assign() so we want to 
> keep vgein bitmap consistent.

Can this really happen? It almost sounds as if you were suspecting
context-switch-in could race with context-switch-out. Yet again - none of
this can sensibly be discussed without seeing how / where the functions are
to be used.

> Regarding why _irqsave() it is mostly connected to ...

Why the mention of _irqsave? My use of "plain spinlock" was meant to contrast
to the possible use of an r/w lock.

Jan

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Oleksii Kurochko]

On 7/30/26 9:42 AM, Jan Beulich wrote:
> On 29.07.2026 16:55, Oleksii Kurochko wrote:
>> On 7/27/26 5:41 PM, Jan Beulich wrote:
>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>> It was decided to add support for IMSIC from the start instead of having APLIC
>>>> operate in direct delivery mode, as it requires a trap-and-emulation approach,
>>>> which is not optimal from a performance standpoint.
>>>>
>>>> AIA provides a hardware-accelerated mechanism for delivering external
>>>> interrupts to domains via "guest interrupt files" located in IMSIC.
>>>> A single physical hart can implement multiple such files (up to GEILEN),
>>>> allowing several virtual harts to receive interrupts directly from hardware.
>>>>
>>>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>>>> for systems implementing AIA specification. Each CPU maintains
>>>> a bitmap describing which guest interrupt files are currently in use.
>>>>
>>>> Add helpers to initialize the bitmap based on the number of available
>>>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>>>> when no longer needed. When assigning a VGEIN, the corresponding value
>>>> is written to the VGEIN field of the guest hstatus register so that
>>>> VS-level external interrupts are delivered from the selected interrupt
>>>> file.
>>>
>>> And when exactly is this "assignment" intended to occur? vgein_assign() and
>>> vgein_release() have no callers here, so this remains entirely unclear.
>>
>> [A] Agreed, I should have added that information to the commit message:
>>
>> VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU
>> execution context (in continue_new_vcpu()) and is re-assigned during
>> vCPU migration from one pCPU to another.
>>
>> VGEIN is released (via vgein_release()) on the old pCPU during migration.
> 
> That is, state of that vCPU is held in hardware for perhaps an extended
> period of time after the vCPU was last de-scheduled. That's a fair
> optimization (we do something similar on x86, albeit that has been
> increasingly under question lately). However, doesn't this then require
> sync_local_execstate() to become non-empty?

IIUC, sync_local_execstate() is needed for the lazy context switch case 
when switching from vCPUA to the idle vCPU. The idea is that if, after 
running the idle vCPU, the next scheduled vCPU is again vCPUA, then 
nothing needs to be done because no real context switch has occurred yet.

IMO, this is not the case for VGEIN. It is perfectly fine for a vCPU to 
keep its previously assigned VGEIN even if the next scheduled vCPU is 
different. In fact, it is necessary to preserve VGEIN because it will be 
needed later, for example, to wake up the vCPU if an interrupt for that 
vCPU occurs.

The idea is that if a vCPU uses a hardware interrupt file, then when the 
vCPU is descheduled, the corresponding CSR_HGEIE bit, where the bit 
number corresponds to the VGEIN value, is set. If an IRQ_S_GEXT trap 
then occurs, the hgei_interrupt() handler can determine which vCPU 
should be woken up:

void hgei_interrupt(void)
{
     unsigned long hgei_mask, flags;
     struct vgein_ctrl *vgein_ = &this_cpu(vgein);

     hgei_mask = csr_read(CSR_HGEIP) & csr_read(CSR_HGEIE);
     csr_clear(CSR_HGEIE, hgei_mask);

     spin_lock_irqsave(&vgein_->lock, flags);

     for_each_set_bit ( vs_guest_file_id, hgei_mask )
     {
        ...
        /* do some logic to call vcpu_kick */
        ...
     }

     spin_unlock_irqrestore(&vgein_->lock, flags);
}

> 
> Furthermore, rather than having vgein_assign() fail when
> find_next_zero_bit() fails to find an available ID, shouldn't you release
> some other vCPU's ID, making it available for re-use?

This is a good question, and it requires a separate investigation to 
determine whether such an approach would actually be beneficial. It 
would require not only changing the VGEIN field in vcpu->hstatus, but 
also synchronizing at least the pending interrupts from one IMSIC 
interrupt file to another, which would also consume time and further 
complicate the logic.

If find_next_zero_bit() fails, the vCPU will simply receive VGEIN=0, 
which means that the software interrupt file will be used. Therefore, 
everything should continue to work correctly, although it will be slower 
than using a hardware interrupt file.

Considering that the maximum value of GEILEN is 31 for RV32 and 63 for 
RV64 (although there is no guarantee that an implementation will support 
the maximum value), let's assume a GEILEN value of 31 for RV64 as well. 
In that case, a system with 4 CPUs would cover the maximum number of 
vCPUs supported by Xen (IIURC, it is 128). Therefore, a software 
interrupt file would not be needed at all, assuming the scheduler 
distributes vCPUs reasonably well.

Even if GEILEN is smaller, I expect that the scheduler will migrate 
vCPUs between pCPUs from time to time. This will free a hardware IMSIC 
interrupt file slot on the previous pCPU, allowing another vCPU to 
obtain a hardware IMSIC interrupt file slot.

For now, I would prefer to keep the current VGEIN allocation strategy as 
it is definitely easier for implementation at least and consider your 
suggestion of releasing another vCPU's VGEIN as a potential 
optimization. I think this optimization should first be evaluated 
through measurements and experiments.

> 
>>>> +static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long action,
>>>> +                        void *hcpu)
>>>> +{
>>>> +    unsigned int cpu = (unsigned long)hcpu;
>>>> +    int rc = 0;
>>>> +
>>>> +    switch ( action )
>>>> +    {
>>>> +    case CPU_STARTING:
>>>> +        rc = vgein_init(cpu);
>>>> +        if ( rc )
>>>> +            printk("AIA: failed to init vgein for CPU%u\n", cpu);
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return notifier_from_errno(rc);
>>>> +}
>>>
>>> Where's the freeing of the allocation vgein_init(), when CPU bringup fails
>>> or a CPU was brought down?
>> I'll add the following:
>>
>> case CPU_UP_CANCELED:
>> case CPU_DEAD:
>>       vgein_free(cpu);
>>       break;
>>
>> and:
>>
>> static void vgein_free(unsigned int cpu)
>> {
>>       struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
>>
>>       ASSERT(!vgein->bmp);
> 
> Does this hold in all cases? What migrates vCPU-s off of a pCPU going down?
> IOW aren't you introducing an ordering problem between your notifier handler
> and the scheduler's?

The migration is done by the scheduler/cpupool notifiers at 
CPU_DOWN_PREPARE (cpupool_cpu_remove_prologue() -> 
cpu_disable_scheduler()) and CPU_DYING (cpupool_cpu_remove()), i.e. in 
actions strictly preceding CPU_DEAD; each unit migration goes through 
sched_move_irqs() -> arch_move_irqs() -> imsic_migrate_vcpu(), which 
releases the VGEIN on the old pCPU. So there's no ordering dependency on 
notifier priority within CPU_DEAD.

However you're right that the assertion doesn't hold in all cases. On 
the suspend path both notifiers bail out for system_state > 
SYS_STATE_active, so no vCPU is migrated off at all and the bits are 
still set at CPU_DEAD — and freeing owners[] there would be actively 
wrong, since the vCPUs still reference that pCPU's VS-file. 
Independently, arch_vcpu_destroy() never releases the VGEIN today, so a 
destroyed vCPU leaks its bit for good.

Given GEILEN <= XLEN-1, I'll drop the allocation altogether and use a 
fixed struct vcpu *owners[BITS_PER_LONG] in the per-CPU structure; that 
removes vgein_free() and the question with it. I'll fix the missing 
release in vcpu teardown separately.

> 
>>       vgein->geilen = 0;
>>       XVFREE(vgein->owners);
>> }
>>
>> I'm also wondering whether vgein_init() should be moved to
>> CPU_UP_PREPARE. If vgein_init() fails in CPU_STARTING, the hypervisor
>> will stop instead of simply ignoring the CPU.
>>
>> However, in CPU_UP_PREPARE we don't yet know the value of GEILEN, which
>> is needed to allocate vgein->owners. As I understand it, CPU_UP_PREPARE
>> is not executed on the CPU that is being brought up.
> 
> But there's an upper bound, isn't there? Use that for preliminary allocation,
> and re-alloc (best effort) from CPU_ONLINE?

Considering that upper bound isn't to big then we could just live with 
that without having re-alloc. Look at what I wrote above.

> 
> Yet then I continue to question the presence of this array in the first place.
> Something similar isn't needed elsewhere (afaik), and its intended use (as
> said) doesn't become obvious here.

I can drop it for now and reintroduce it later when it is actually 
needed. In short, it is intended to be used in hgei_interrupt(), as I 
described above, in the following way (inside the for-loop):

...
for_each_set_bit(vs_guest_file_id, hgei_mask)
{
unsigned int owners_index = vs_guest_file_id /* - 1 */;

```
if ( vgein_->owners[owners_index] )
{
     dprintk("kick ->%pv, hgei_mask(%#lx)\n",
             vgein_->owners[owners_index], hgei_mask);

     vcpu_kick(vgein_->owners[owners_index]);
}
```

}
...

Alternatively, I could introduce this in this series, since it will be 
necessary to set the HGEIE bit in imsic_state_save() anyway to allow the 
vCPU to be woken up. It also seems like the best option, as it addresses 
at least some of the comments you raised.

> 
>>>> +unsigned int vgein_assign(struct vcpu *v)
>>>> +{
>>>> +    unsigned int vgein_id;
>>>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>>>> +    unsigned long *bmp = &vgein->bmp;
>>>> +    unsigned long flags;
>>>> +
>>>> +    if ( !vgein->geilen )
>>>> +        return 0;
>>>> +
>>>> +    spin_lock_irqsave(&vgein->lock, flags);
>>>
>>> Because it's unclear where this is to be called from, it's also unclear whether
>>> a lock is needed here (and if so whether a plain spin lock is appropriate).
>>
>> Based on what I wrote in [A] above a lock is defintely needed as it
>> could be that vgein_release() is called for old pCPU during migration
>> and at the same time old pCPU could call vgein_assign() so we want to
>> keep vgein bitmap consistent.
> 
> Can this really happen? It almost sounds as if you were suspecting
> context-switch-in could race with context-switch-out. Yet again - none of
> this can sensibly be discussed without seeing how / where the functions are
> to be used.

Maybe I didn't explain it clearly, but during migration (which, 
according to my understanding of vcpu_move_irqs(), is executed on 
pCPU1), when vCPU0 is migrated from pCPU0 to pCPU1, its old VGEIN on 
pCPU0 needs to be released. I don't see any reason why, at the same 
time, pCPU0 could not try to assign that VGEIN to another vCPU. Without 
proper protection, this could lead to race conditions.

Also, setting a bit in the VGEIN bitmap and updating the owner array 
should be an atomic operation, at least to correctly handle the 
hgei_interrupt() case mentioned above and vCPU migration, which calls 
vgein_release(...,old_pcpu,...).

I agree that it would probably be easier if the migration patches were 
included in this patch series as well. I can either post those patches 
to this thread now or include them in the v2 series when it is ready. 
What do you think?

> 
>> Regarding why _irqsave() it is mostly connected to ...
> 
> Why the mention of _irqsave? My use of "plain spinlock" was meant to contrast
> to the possible use of an r/w lock.

Oh, okay... I thought your question was why the _irqsave() variant is 
used specifically.

I think it is hard to predict whether read operations will be much more 
frequent than write operations in this case. It depends on how often 
vCPU migration occurs and how often hgei_interrupt() is called. At the 
moment, I believe hgei_interrupt() is the only reader.

~ Oleksii

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Jan Beulich]

On 30.07.2026 17:46, Oleksii Kurochko wrote:
> On 7/30/26 9:42 AM, Jan Beulich wrote:
>> On 29.07.2026 16:55, Oleksii Kurochko wrote:
>>> On 7/27/26 5:41 PM, Jan Beulich wrote:
>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>> It was decided to add support for IMSIC from the start instead of having APLIC
>>>>> operate in direct delivery mode, as it requires a trap-and-emulation approach,
>>>>> which is not optimal from a performance standpoint.
>>>>>
>>>>> AIA provides a hardware-accelerated mechanism for delivering external
>>>>> interrupts to domains via "guest interrupt files" located in IMSIC.
>>>>> A single physical hart can implement multiple such files (up to GEILEN),
>>>>> allowing several virtual harts to receive interrupts directly from hardware.
>>>>>
>>>>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>>>>> for systems implementing AIA specification. Each CPU maintains
>>>>> a bitmap describing which guest interrupt files are currently in use.
>>>>>
>>>>> Add helpers to initialize the bitmap based on the number of available
>>>>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>>>>> when no longer needed. When assigning a VGEIN, the corresponding value
>>>>> is written to the VGEIN field of the guest hstatus register so that
>>>>> VS-level external interrupts are delivered from the selected interrupt
>>>>> file.
>>>>
>>>> And when exactly is this "assignment" intended to occur? vgein_assign() and
>>>> vgein_release() have no callers here, so this remains entirely unclear.
>>>
>>> [A] Agreed, I should have added that information to the commit message:
>>>
>>> VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU
>>> execution context (in continue_new_vcpu()) and is re-assigned during
>>> vCPU migration from one pCPU to another.
>>>
>>> VGEIN is released (via vgein_release()) on the old pCPU during migration.
>>
>> That is, state of that vCPU is held in hardware for perhaps an extended
>> period of time after the vCPU was last de-scheduled. That's a fair
>> optimization (we do something similar on x86, albeit that has been
>> increasingly under question lately). However, doesn't this then require
>> sync_local_execstate() to become non-empty?
> 
> IIUC, sync_local_execstate() is needed for the lazy context switch case 
> when switching from vCPUA to the idle vCPU.

Or when full state is to be obtained for a vCPU, for example.

> The idea is that if, after 
> running the idle vCPU, the next scheduled vCPU is again vCPUA, then 
> nothing needs to be done because no real context switch has occurred yet.
> 
> IMO, this is not the case for VGEIN. It is perfectly fine for a vCPU to 
> keep its previously assigned VGEIN even if the next scheduled vCPU is 
> different. In fact, it is necessary to preserve VGEIN because it will be 
> needed later, for example, to wake up the vCPU if an interrupt for that 
> vCPU occurs.
> 
> The idea is that if a vCPU uses a hardware interrupt file, then when the 
> vCPU is descheduled, the corresponding CSR_HGEIE bit, where the bit 
> number corresponds to the VGEIN value, is set. If an IRQ_S_GEXT trap 
> then occurs, the hgei_interrupt() handler can determine which vCPU 
> should be woken up:
> 
> void hgei_interrupt(void)
> {
>      unsigned long hgei_mask, flags;
>      struct vgein_ctrl *vgein_ = &this_cpu(vgein);
> 
>      hgei_mask = csr_read(CSR_HGEIP) & csr_read(CSR_HGEIE);
>      csr_clear(CSR_HGEIE, hgei_mask);
> 
>      spin_lock_irqsave(&vgein_->lock, flags);
> 
>      for_each_set_bit ( vs_guest_file_id, hgei_mask )
>      {
>         ...
>         /* do some logic to call vcpu_kick */
>         ...
>      }
> 
>      spin_unlock_irqrestore(&vgein_->lock, flags);
> }

Ah, that's pretty helpful extra information.

>> Furthermore, rather than having vgein_assign() fail when
>> find_next_zero_bit() fails to find an available ID, shouldn't you release
>> some other vCPU's ID, making it available for re-use?
> 
> This is a good question, and it requires a separate investigation to 
> determine whether such an approach would actually be beneficial. It 
> would require not only changing the VGEIN field in vcpu->hstatus, but 
> also synchronizing at least the pending interrupts from one IMSIC 
> interrupt file to another, which would also consume time and further 
> complicate the logic.
> 
> If find_next_zero_bit() fails, the vCPU will simply receive VGEIN=0, 
> which means that the software interrupt file will be used. Therefore, 
> everything should continue to work correctly, although it will be slower 
> than using a hardware interrupt file.

Plus there may end up being subtly different behavior. Imo you want to
let the hardware do what it can do for you.

> Considering that the maximum value of GEILEN is 31 for RV32 and 63 for 
> RV64 (although there is no guarantee that an implementation will support 
> the maximum value), let's assume a GEILEN value of 31 for RV64 as well. 
> In that case, a system with 4 CPUs would cover the maximum number of 
> vCPUs supported by Xen (IIURC, it is 128).

That's a single domain. There can be many domains, totaling to far more
than 128 vCPU-s.

> Therefore, a software 
> interrupt file would not be needed at all, assuming the scheduler 
> distributes vCPUs reasonably well.
> 
> Even if GEILEN is smaller, I expect that the scheduler will migrate 
> vCPUs between pCPUs from time to time. This will free a hardware IMSIC 
> interrupt file slot on the previous pCPU, allowing another vCPU to 
> obtain a hardware IMSIC interrupt file slot.
> 
> For now, I would prefer to keep the current VGEIN allocation strategy as 
> it is definitely easier for implementation at least and consider your 
> suggestion of releasing another vCPU's VGEIN as a potential 
> optimization. I think this optimization should first be evaluated 
> through measurements and experiments.

Well, I'm not going to insist, but I expect this will need re-doing rather
sooner than later then.

>> Yet then I continue to question the presence of this array in the first place.
>> Something similar isn't needed elsewhere (afaik), and its intended use (as
>> said) doesn't become obvious here.
> 
> I can drop it for now and reintroduce it later when it is actually 
> needed. In short, it is intended to be used in hgei_interrupt(), as I 
> described above, in the following way (inside the for-loop):
> 
> ...
> for_each_set_bit(vs_guest_file_id, hgei_mask)
> {
> unsigned int owners_index = vs_guest_file_id /* - 1 */;
> 
> ```
> if ( vgein_->owners[owners_index] )
> {
>      dprintk("kick ->%pv, hgei_mask(%#lx)\n",
>              vgein_->owners[owners_index], hgei_mask);
> 
>      vcpu_kick(vgein_->owners[owners_index]);
> }
> ```
> 
> }
> ...
> 
> Alternatively, I could introduce this in this series, since it will be 
> necessary to set the HGEIE bit in imsic_state_save() anyway to allow the 
> vCPU to be woken up. It also seems like the best option, as it addresses 
> at least some of the comments you raised.

Right, and then preferably in an order where one won't need to peek ahead
in the series to actually understand what's going on.

>>>>> +unsigned int vgein_assign(struct vcpu *v)
>>>>> +{
>>>>> +    unsigned int vgein_id;
>>>>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>>>>> +    unsigned long *bmp = &vgein->bmp;
>>>>> +    unsigned long flags;
>>>>> +
>>>>> +    if ( !vgein->geilen )
>>>>> +        return 0;
>>>>> +
>>>>> +    spin_lock_irqsave(&vgein->lock, flags);
>>>>
>>>> Because it's unclear where this is to be called from, it's also unclear whether
>>>> a lock is needed here (and if so whether a plain spin lock is appropriate).
>>>
>>> Based on what I wrote in [A] above a lock is defintely needed as it
>>> could be that vgein_release() is called for old pCPU during migration
>>> and at the same time old pCPU could call vgein_assign() so we want to
>>> keep vgein bitmap consistent.
>>
>> Can this really happen? It almost sounds as if you were suspecting
>> context-switch-in could race with context-switch-out. Yet again - none of
>> this can sensibly be discussed without seeing how / where the functions are
>> to be used.
> 
> Maybe I didn't explain it clearly, but during migration (which, 
> according to my understanding of vcpu_move_irqs(), is executed on 
> pCPU1), when vCPU0 is migrated from pCPU0 to pCPU1, its old VGEIN on 
> pCPU0 needs to be released. I don't see any reason why, at the same 
> time, pCPU0 could not try to assign that VGEIN to another vCPU. Without 
> proper protection, this could lead to race conditions.

Doesn't migration of vCPU-s between pCPU-s happen under suitable scheduler
locks?

> Also, setting a bit in the VGEIN bitmap and updating the owner array 
> should be an atomic operation, at least to correctly handle the 
> hgei_interrupt() case mentioned above and vCPU migration, which calls 
> vgein_release(...,old_pcpu,...).
> 
> I agree that it would probably be easier if the migration patches were 
> included in this patch series as well. I can either post those patches 
> to this thread now or include them in the v2 series when it is ready. 
> What do you think?

Including in v2 may be helpful, again to eliminate gaps in the understanding
a reader like me needs to have.

Jan

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Oleksii Kurochko]

On 7/30/26 6:03 PM, Jan Beulich wrote:
> On 30.07.2026 17:46, Oleksii Kurochko wrote:
>> On 7/30/26 9:42 AM, Jan Beulich wrote:
>>> On 29.07.2026 16:55, Oleksii Kurochko wrote:
>>>> On 7/27/26 5:41 PM, Jan Beulich wrote:
>>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>>> It was decided to add support for IMSIC from the start instead of having APLIC
>>>>>> operate in direct delivery mode, as it requires a trap-and-emulation approach,
>>>>>> which is not optimal from a performance standpoint.
>>>>>>
>>>>>> AIA provides a hardware-accelerated mechanism for delivering external
>>>>>> interrupts to domains via "guest interrupt files" located in IMSIC.
>>>>>> A single physical hart can implement multiple such files (up to GEILEN),
>>>>>> allowing several virtual harts to receive interrupts directly from hardware.
>>>>>>
>>>>>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>>>>>> for systems implementing AIA specification. Each CPU maintains
>>>>>> a bitmap describing which guest interrupt files are currently in use.
>>>>>>
>>>>>> Add helpers to initialize the bitmap based on the number of available
>>>>>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>>>>>> when no longer needed. When assigning a VGEIN, the corresponding value
>>>>>> is written to the VGEIN field of the guest hstatus register so that
>>>>>> VS-level external interrupts are delivered from the selected interrupt
>>>>>> file.
>>>>>
>>>>> And when exactly is this "assignment" intended to occur? vgein_assign() and
>>>>> vgein_release() have no callers here, so this remains entirely unclear.
>>>>
>>>> [A] Agreed, I should have added that information to the commit message:
>>>>
>>>> VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU
>>>> execution context (in continue_new_vcpu()) and is re-assigned during
>>>> vCPU migration from one pCPU to another.
>>>>
>>>> VGEIN is released (via vgein_release()) on the old pCPU during migration.
>>>
>>> That is, state of that vCPU is held in hardware for perhaps an extended
>>> period of time after the vCPU was last de-scheduled. That's a fair
>>> optimization (we do something similar on x86, albeit that has been
>>> increasingly under question lately). However, doesn't this then require
>>> sync_local_execstate() to become non-empty?
>>
>> IIUC, sync_local_execstate() is needed for the lazy context switch case
>> when switching from vCPUA to the idle vCPU.
> 
> Or when full state is to be obtained for a vCPU, for example.

I assume you're referring to XEN_DOMCTL_getvcpucontext, right?

In general, it seems that sync_local_execstate() is primarily an 
optimization. If lazy switching isn't supported, then every time a vCPU 
is de-scheduled, its state must be fully saved to memory. My 
understanding is that everything will still work correctly, just less 
efficiently.

I'm curious how much this optimization actually helps. How often does it 
happen that a vCPU is de-scheduled from a pCPU and then immediately 
scheduled back onto the same pCPU without any other vCPU being scheduled 
in between?

I will add to my TODO list that it is nice to use sync_local_execstate() 
in future.

>>>>>> +unsigned int vgein_assign(struct vcpu *v)
>>>>>> +{
>>>>>> +    unsigned int vgein_id;
>>>>>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>>>>>> +    unsigned long *bmp = &vgein->bmp;
>>>>>> +    unsigned long flags;
>>>>>> +
>>>>>> +    if ( !vgein->geilen )
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    spin_lock_irqsave(&vgein->lock, flags);
>>>>>
>>>>> Because it's unclear where this is to be called from, it's also unclear whether
>>>>> a lock is needed here (and if so whether a plain spin lock is appropriate).
>>>>
>>>> Based on what I wrote in [A] above a lock is defintely needed as it
>>>> could be that vgein_release() is called for old pCPU during migration
>>>> and at the same time old pCPU could call vgein_assign() so we want to
>>>> keep vgein bitmap consistent.
>>>
>>> Can this really happen? It almost sounds as if you were suspecting
>>> context-switch-in could race with context-switch-out. Yet again - none of
>>> this can sensibly be discussed without seeing how / where the functions are
>>> to be used.
>>
>> Maybe I didn't explain it clearly, but during migration (which,
>> according to my understanding of vcpu_move_irqs(), is executed on
>> pCPU1), when vCPU0 is migrated from pCPU0 to pCPU1, its old VGEIN on
>> pCPU0 needs to be released. I don't see any reason why, at the same
>> time, pCPU0 could not try to assign that VGEIN to another vCPU. Without
>> proper protection, this could lead to race conditions.
> 
> Doesn't migration of vCPU-s between pCPU-s happen under suitable scheduler
> locks?
> 

If I am not mistaken every path that reaches arch_move_irqs() drops the 
scheduler lock first. The only thing still held at that point is 
sched_res_rculock , and that is an RCU read-side critical section, not 
mutual exclusion: it merely keeps struct sched_resource alive across 
get_sched_res() dereferences, since cpupool/hotplug frees those via 
call_rcu(&sr->rcu, sched_res_free). Any number of pCPUs can be inside it 
concurrently, and it does not disable interrupts, so it serialises 
neither the source pCPU against the destination one nor hgei_interrupt() 
mentioned above against either.

~ Oleksii

Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Jan Beulich]

On 31.07.2026 16:59, Oleksii Kurochko wrote:
> 
> 
> On 7/30/26 6:03 PM, Jan Beulich wrote:
>> On 30.07.2026 17:46, Oleksii Kurochko wrote:
>>> On 7/30/26 9:42 AM, Jan Beulich wrote:
>>>> On 29.07.2026 16:55, Oleksii Kurochko wrote:
>>>>> On 7/27/26 5:41 PM, Jan Beulich wrote:
>>>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>>>> It was decided to add support for IMSIC from the start instead of having APLIC
>>>>>>> operate in direct delivery mode, as it requires a trap-and-emulation approach,
>>>>>>> which is not optimal from a performance standpoint.
>>>>>>>
>>>>>>> AIA provides a hardware-accelerated mechanism for delivering external
>>>>>>> interrupts to domains via "guest interrupt files" located in IMSIC.
>>>>>>> A single physical hart can implement multiple such files (up to GEILEN),
>>>>>>> allowing several virtual harts to receive interrupts directly from hardware.
>>>>>>>
>>>>>>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>>>>>>> for systems implementing AIA specification. Each CPU maintains
>>>>>>> a bitmap describing which guest interrupt files are currently in use.
>>>>>>>
>>>>>>> Add helpers to initialize the bitmap based on the number of available
>>>>>>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>>>>>>> when no longer needed. When assigning a VGEIN, the corresponding value
>>>>>>> is written to the VGEIN field of the guest hstatus register so that
>>>>>>> VS-level external interrupts are delivered from the selected interrupt
>>>>>>> file.
>>>>>>
>>>>>> And when exactly is this "assignment" intended to occur? vgein_assign() and
>>>>>> vgein_release() have no callers here, so this remains entirely unclear.
>>>>>
>>>>> [A] Agreed, I should have added that information to the commit message:
>>>>>
>>>>> VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU
>>>>> execution context (in continue_new_vcpu()) and is re-assigned during
>>>>> vCPU migration from one pCPU to another.
>>>>>
>>>>> VGEIN is released (via vgein_release()) on the old pCPU during migration.
>>>>
>>>> That is, state of that vCPU is held in hardware for perhaps an extended
>>>> period of time after the vCPU was last de-scheduled. That's a fair
>>>> optimization (we do something similar on x86, albeit that has been
>>>> increasingly under question lately). However, doesn't this then require
>>>> sync_local_execstate() to become non-empty?
>>>
>>> IIUC, sync_local_execstate() is needed for the lazy context switch case
>>> when switching from vCPUA to the idle vCPU.
>>
>> Or when full state is to be obtained for a vCPU, for example.
> 
> I assume you're referring to XEN_DOMCTL_getvcpucontext, right?

Yes.

> In general, it seems that sync_local_execstate() is primarily an 
> optimization. If lazy switching isn't supported, then every time a vCPU 
> is de-scheduled, its state must be fully saved to memory. My 
> understanding is that everything will still work correctly, just less 
> efficiently.

The lazy switching is an optimization, yes. If any state is kept in
hardware, sync_local_execstate() has to be used when full state of a
vCPU is to be obtained. Supplying back stale state of "guest interrupt
files" can't be correct. (Of course you can also arrange to obtain
up-to-date state by custom means, but imo that's likely less desirable.)

> I'm curious how much this optimization actually helps. How often does it 
> happen that a vCPU is de-scheduled from a pCPU and then immediately 
> scheduled back onto the same pCPU without any other vCPU being scheduled 
> in between?

That heavily depends on overall load of the system. When pCPU-s aren't
over-subscribed, a HVM vCPU getting de-scheduled to wait for qemu to
handle a certain operation may very well be able to resume on the same
pCPU after completion of the ioreq. The less overhead there, the better.
(Just to give an example.)

Jan

[PATCH v1 03/17] xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h

[Oleksii Kurochko]

These definitions are required for correct decoding of APLIC MMIO
accesses and target configuration, and will be used by both the
physical and virtual APLIC implementations.

No functional change is intended by this patch; it only centralises
hardware definitions that were previously missing.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v3:
 - Drop APLIC_TARGET_IPRIO_MASK and APLIC_TARGET_GUEST_IDX_SHIFT (unused).
 - Add comments to each register field group (domaincfg, sourcecfg, target).
 - Group APLIC_TARGET_HART_IDX_SHIFT and APLIC_TARGET_EIID_MASK together
   under /* target register fields */ at the top of the field definitions.
---
Changes in v2:
 - new patch
---
---
 xen/arch/riscv/include/asm/aplic.h | 35 ++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
index 07318aaac25d..f22622b9a23f 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -15,6 +15,8 @@
 
 #include <asm/imsic.h>
 
+#define APLIC_REG_OFFSET_MASK   0x3fff
+
 /*
  * domaincfg read-only fields (AIA spec):
  *  - bits [31:24] -> read-only 0x80
@@ -25,6 +27,7 @@
 #define APLIC_DOMAINCFG_DM      BIT(2, U)
 #define APLIC_DOMAINCFG_BE      BIT(0, U)
 
+/* sourcecfg register fields */
 #define APLIC_SOURCECFG_SM_INACTIVE     0x0
 #define APLIC_SOURCECFG_SM_DETACH       0x1
 #define APLIC_SOURCECFG_SM_EDGE_RISE    0x4
@@ -32,7 +35,39 @@
 #define APLIC_SOURCECFG_SM_LEVEL_HIGH   0x6
 #define APLIC_SOURCECFG_SM_LEVEL_LOW    0x7
 
+/* target register fields */
 #define APLIC_TARGET_HART_IDX_SHIFT 18
+#define APLIC_TARGET_EIID_MASK      0x7ff
+
+#define APLIC_DOMAINCFG         0x0000
+#define APLIC_SOURCECFG_BASE    0x0004
+#define APLIC_SOURCECFG_LAST    0x0ffc
+
+#define APLIC_SMSICFGADDR       0x1bc8
+#define APLIC_SMSICFGADDRH      0x1bcc
+
+#define APLIC_SETIP_BASE        0x1c00
+#define APLIC_SETIP_LAST        0x1c7c
+#define APLIC_SETIPNUM          0x1cdc
+
+#define APLIC_CLRIP_BASE        0x1d00
+#define APLIC_CLRIP_LAST        0x1d7c
+#define APLIC_CLRIPNUM          0x1ddc
+
+#define APLIC_SETIE_BASE        0x1e00
+#define APLIC_SETIE_LAST        0x1e7c
+#define APLIC_SETIENUM          0x1edc
+
+#define APLIC_CLRIE_BASE        0x1f00
+#define APLIC_CLRIE_LAST        0x1f7c
+#define APLIC_CLRIENUM          0x1fdc
+
+#define APLIC_SETIPNUM_LE       0x2000
+
+#define APLIC_GENMSI            0x3000
+
+#define APLIC_TARGET_BASE       0x3004
+#define APLIC_TARGET_LAST       0x3ffc
 
 #define APLIC_IDC_SIZE          32
 
-- 
2.54.0

Re: [PATCH v1 03/17] xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h

[Jan Beulich]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> @@ -25,6 +27,7 @@
>  #define APLIC_DOMAINCFG_DM      BIT(2, U)
>  #define APLIC_DOMAINCFG_BE      BIT(0, U)
>  
> +/* sourcecfg register fields */
>  #define APLIC_SOURCECFG_SM_INACTIVE     0x0
>  #define APLIC_SOURCECFG_SM_DETACH       0x1
>  #define APLIC_SOURCECFG_SM_EDGE_RISE    0x4
> @@ -32,7 +35,39 @@
>  #define APLIC_SOURCECFG_SM_LEVEL_HIGH   0x6
>  #define APLIC_SOURCECFG_SM_LEVEL_LOW    0x7
>  
> +/* target register fields */
>  #define APLIC_TARGET_HART_IDX_SHIFT 18
> +#define APLIC_TARGET_EIID_MASK      0x7ff

So why would these live here, far ahead of ...

> +#define APLIC_DOMAINCFG         0x0000
> +#define APLIC_SOURCECFG_BASE    0x0004
> +#define APLIC_SOURCECFG_LAST    0x0ffc
> +
> +#define APLIC_SMSICFGADDR       0x1bc8
> +#define APLIC_SMSICFGADDRH      0x1bcc
> +
> +#define APLIC_SETIP_BASE        0x1c00
> +#define APLIC_SETIP_LAST        0x1c7c
> +#define APLIC_SETIPNUM          0x1cdc
> +
> +#define APLIC_CLRIP_BASE        0x1d00
> +#define APLIC_CLRIP_LAST        0x1d7c
> +#define APLIC_CLRIPNUM          0x1ddc
> +
> +#define APLIC_SETIE_BASE        0x1e00
> +#define APLIC_SETIE_LAST        0x1e7c
> +#define APLIC_SETIENUM          0x1edc
> +
> +#define APLIC_CLRIE_BASE        0x1f00
> +#define APLIC_CLRIE_LAST        0x1f7c
> +#define APLIC_CLRIENUM          0x1fdc
> +
> +#define APLIC_SETIPNUM_LE       0x2000
> +
> +#define APLIC_GENMSI            0x3000
> +
> +#define APLIC_TARGET_BASE       0x3004
> +#define APLIC_TARGET_LAST       0x3ffc

... the register definition itself. Would you mind taking a look at the top
third (or so) of arch/x86/include/asm/msr-index.h? There you'll find MSR
index values of MSRs we use, immediately followed by field definitions (of
course only where applicable). With such an arrangement, the extra comments
you add can easily be omitted.

Jan

Re: [PATCH v1 03/17] xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h

[Oleksii Kurochko]

On 7/28/26 2:02 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> @@ -25,6 +27,7 @@
>>   #define APLIC_DOMAINCFG_DM      BIT(2, U)
>>   #define APLIC_DOMAINCFG_BE      BIT(0, U)
>>   
>> +/* sourcecfg register fields */
>>   #define APLIC_SOURCECFG_SM_INACTIVE     0x0
>>   #define APLIC_SOURCECFG_SM_DETACH       0x1
>>   #define APLIC_SOURCECFG_SM_EDGE_RISE    0x4
>> @@ -32,7 +35,39 @@
>>   #define APLIC_SOURCECFG_SM_LEVEL_HIGH   0x6
>>   #define APLIC_SOURCECFG_SM_LEVEL_LOW    0x7
>>   
>> +/* target register fields */
>>   #define APLIC_TARGET_HART_IDX_SHIFT 18
>> +#define APLIC_TARGET_EIID_MASK      0x7ff
> 
> So why would these live here, far ahead of ...
> 
>> +#define APLIC_DOMAINCFG         0x0000
>> +#define APLIC_SOURCECFG_BASE    0x0004
>> +#define APLIC_SOURCECFG_LAST    0x0ffc
>> +
>> +#define APLIC_SMSICFGADDR       0x1bc8
>> +#define APLIC_SMSICFGADDRH      0x1bcc
>> +
>> +#define APLIC_SETIP_BASE        0x1c00
>> +#define APLIC_SETIP_LAST        0x1c7c
>> +#define APLIC_SETIPNUM          0x1cdc
>> +
>> +#define APLIC_CLRIP_BASE        0x1d00
>> +#define APLIC_CLRIP_LAST        0x1d7c
>> +#define APLIC_CLRIPNUM          0x1ddc
>> +
>> +#define APLIC_SETIE_BASE        0x1e00
>> +#define APLIC_SETIE_LAST        0x1e7c
>> +#define APLIC_SETIENUM          0x1edc
>> +
>> +#define APLIC_CLRIE_BASE        0x1f00
>> +#define APLIC_CLRIE_LAST        0x1f7c
>> +#define APLIC_CLRIENUM          0x1fdc
>> +
>> +#define APLIC_SETIPNUM_LE       0x2000
>> +
>> +#define APLIC_GENMSI            0x3000
>> +
>> +#define APLIC_TARGET_BASE       0x3004
>> +#define APLIC_TARGET_LAST       0x3ffc
> 
> ... the register definition itself. Would you mind taking a look at the top
> third (or so) of arch/x86/include/asm/msr-index.h? There you'll find MSR
> index values of MSRs we use, immediately followed by field definitions (of
> course only where applicable). With such an arrangement, the extra comments
> you add can easily be omitted.

I'll move the definitions mentioned above to the suggested location and 
follow this pattern in future changes. I just thought it would be better 
to keep the APLIC register definitions together in memory layout order.

Thanks.

~ Oleksii

Re: [PATCH v1 03/17] xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h

[Jan Beulich]

On 29.07.2026 17:26, Oleksii Kurochko wrote:
> 
> 
> On 7/28/26 2:02 PM, Jan Beulich wrote:
>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>> @@ -25,6 +27,7 @@
>>>   #define APLIC_DOMAINCFG_DM      BIT(2, U)
>>>   #define APLIC_DOMAINCFG_BE      BIT(0, U)
>>>   
>>> +/* sourcecfg register fields */
>>>   #define APLIC_SOURCECFG_SM_INACTIVE     0x0
>>>   #define APLIC_SOURCECFG_SM_DETACH       0x1
>>>   #define APLIC_SOURCECFG_SM_EDGE_RISE    0x4
>>> @@ -32,7 +35,39 @@
>>>   #define APLIC_SOURCECFG_SM_LEVEL_HIGH   0x6
>>>   #define APLIC_SOURCECFG_SM_LEVEL_LOW    0x7
>>>   
>>> +/* target register fields */
>>>   #define APLIC_TARGET_HART_IDX_SHIFT 18
>>> +#define APLIC_TARGET_EIID_MASK      0x7ff
>>
>> So why would these live here, far ahead of ...
>>
>>> +#define APLIC_DOMAINCFG         0x0000
>>> +#define APLIC_SOURCECFG_BASE    0x0004
>>> +#define APLIC_SOURCECFG_LAST    0x0ffc
>>> +
>>> +#define APLIC_SMSICFGADDR       0x1bc8
>>> +#define APLIC_SMSICFGADDRH      0x1bcc
>>> +
>>> +#define APLIC_SETIP_BASE        0x1c00
>>> +#define APLIC_SETIP_LAST        0x1c7c
>>> +#define APLIC_SETIPNUM          0x1cdc
>>> +
>>> +#define APLIC_CLRIP_BASE        0x1d00
>>> +#define APLIC_CLRIP_LAST        0x1d7c
>>> +#define APLIC_CLRIPNUM          0x1ddc
>>> +
>>> +#define APLIC_SETIE_BASE        0x1e00
>>> +#define APLIC_SETIE_LAST        0x1e7c
>>> +#define APLIC_SETIENUM          0x1edc
>>> +
>>> +#define APLIC_CLRIE_BASE        0x1f00
>>> +#define APLIC_CLRIE_LAST        0x1f7c
>>> +#define APLIC_CLRIENUM          0x1fdc
>>> +
>>> +#define APLIC_SETIPNUM_LE       0x2000
>>> +
>>> +#define APLIC_GENMSI            0x3000
>>> +
>>> +#define APLIC_TARGET_BASE       0x3004
>>> +#define APLIC_TARGET_LAST       0x3ffc
>>
>> ... the register definition itself. Would you mind taking a look at the top
>> third (or so) of arch/x86/include/asm/msr-index.h? There you'll find MSR
>> index values of MSRs we use, immediately followed by field definitions (of
>> course only where applicable). With such an arrangement, the extra comments
>> you add can easily be omitted.
> 
> I'll move the definitions mentioned above to the suggested location and 
> follow this pattern in future changes. I just thought it would be better 
> to keep the APLIC register definitions together in memory layout order.

Retaining that order is certainly wanted. Keeping them together is also wanted,
just with the bit/field definitions inserted accordingly.

Jan

[PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Oleksii Kurochko]

RISC-V guests can expose several virtual interrupt controllers at
distinct GPA ranges: vPLIC (hasn't been introduced yet) for legacy machines,
vAPLIC and vIMSIC for AIA-compliant ones (is being introduced in the follow
up patches). Routing MMIO faults via a per-device is_access() check in the
trap handler would couple it to every device it must serve, requiring a
new conditional branch in the fault path each time a new emulated device is
added.

Introduce a per-domain MMIO handler registration table, modeled
after the equivalent ARM framework, so that virtual devices
self-register their GPA ranges and read/write callbacks at domain
creation time. The MMIO fault path delegates to a single
try_handle_mmio() entry point and remains agnostic of which device
owns a particular address.

Subsequent patches wire this into arch_domain_create() and the MMIO fault
path in traps.c.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Note that find_mmio_handler() and try_handle_mmio() is handling found
handler differently for now in comparison to Arm. But this behaviour will
be aligned at the end. Look at discussion:
  https://lore.kernel.org/xen-devel/cd78972e-88d5-471d-a201-5f9cd1392c73@gmail.com/T/#t
---
---
 xen/arch/riscv/Makefile             |   1 +
 xen/arch/riscv/domain.c             |   4 +
 xen/arch/riscv/include/asm/domain.h |   3 +
 xen/arch/riscv/include/asm/mmio.h   |  63 ++++++++++++
 xen/arch/riscv/mmio.c               | 145 ++++++++++++++++++++++++++++
 5 files changed, 216 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/mmio.h
 create mode 100644 xen/arch/riscv/mmio.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 046f73f4d87c..c452ebc3cf61 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -14,6 +14,7 @@ obj-y += intc.o
 obj-y += irq.o
 obj-y += kernel.init.o
 obj-y += mm.o
+obj-y += mmio.o
 obj-y += p2m.o
 obj-y += paging.o
 obj-y += pt.o
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 4db9c28662c7..1e6f0ef66c2f 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -12,6 +12,7 @@
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
 #include <asm/intc.h>
+#include <asm/mmio.h>
 #include <asm/riscv_encoding.h>
 #include <asm/vtimer.h>
 
@@ -308,6 +309,9 @@ int arch_domain_create(struct domain *d,
     if ( (rc = p2m_init(d, config)) != 0)
         goto fail;
 
+    if ( (rc = domain_io_init(d, MAX_IO_HANDLER)) != 0 )
+        goto fail;
+
     if ( (rc = domain_vintc_init(d)) )
         goto fail;
 
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index e035b33ddfdc..15e8fa19685e 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -9,6 +9,7 @@
 
 #include <asm/cpufeature.h>
 #include <asm/guest-layout.h>
+#include <asm/mmio.h>
 #include <asm/p2m.h>
 #include <asm/vtimer.h>
 
@@ -101,6 +102,8 @@ struct arch_domain {
     const unsigned long *isa;
 
     struct vintc *vintc;
+
+    struct vmmio vmmio;
 };
 
 #include <xen/sched.h>
diff --git a/xen/arch/riscv/include/asm/mmio.h b/xen/arch/riscv/include/asm/mmio.h
new file mode 100644
index 000000000000..18df1133e621
--- /dev/null
+++ b/xen/arch/riscv/include/asm/mmio.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef RISCV_MMIO_H
+#define RISCV_MMIO_H
+
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+
+#define MAX_IO_HANDLER  16
+
+typedef struct {
+    paddr_t gpa;
+    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
+    bool is_write;
+    register_t data;   /* store: value to write; load: value read (set by handler) */
+} mmio_info_t;
+
+enum io_state
+{
+    IO_ABORT,       /* The IO was handled and led to an abort. */
+    IO_HANDLED,     /* The IO was successfully handled. */
+    IO_UNHANDLED,   /* No handler found for the IO. */
+};
+
+typedef enum io_state (*mmio_read_t)(struct vcpu *v, mmio_info_t *info,
+                                     register_t *r);
+typedef enum io_state (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
+                                      register_t r);
+
+struct mmio_handler_ops {
+    mmio_read_t read;
+    mmio_write_t write;
+};
+
+struct mmio_handler {
+    paddr_t addr;
+    paddr_t size;
+    const struct mmio_handler_ops *ops;
+};
+
+struct vmmio {
+    unsigned int num_entries;
+    unsigned int max_num_entries;
+    rwlock_t lock;
+    struct mmio_handler *handlers;
+};
+
+enum io_state try_handle_mmio(mmio_info_t *info);
+void register_mmio_handler(struct domain *d,
+                           const struct mmio_handler_ops *ops,
+                           paddr_t addr, paddr_t size);
+int domain_io_init(struct domain *d, unsigned int max_count);
+void domain_io_free(struct domain *d);
+
+#endif /* RISCV_MMIO_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/riscv/mmio.c b/xen/arch/riscv/mmio.c
new file mode 100644
index 000000000000..7d56bc8b27c5
--- /dev/null
+++ b/xen/arch/riscv/mmio.c
@@ -0,0 +1,145 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) Vates
+ */
+
+#include <xen/bsearch.h>
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+#include <xen/sched.h>
+#include <xen/sort.h>
+#include <xen/xvmalloc.h>
+
+#include <asm/current.h>
+#include <asm/mmio.h>
+
+static enum io_state handle_read(const struct mmio_handler *handler,
+                                 struct vcpu *v,
+                                 mmio_info_t *info)
+{
+    register_t r = 0;
+    enum io_state rc;
+
+    rc = handler->ops->read(v, info, &r);
+    if ( rc == IO_HANDLED )
+        info->data = r;
+
+    return rc;
+}
+
+static enum io_state handle_write(const struct mmio_handler *handler,
+                                  struct vcpu *v,
+                                  mmio_info_t *info)
+{
+    return handler->ops->write(v, info, info->data);
+}
+
+/* Assumes mmio regions are not overlapping. */
+static int cmp_mmio_handler(const void *key, const void *elem)
+{
+    const struct mmio_handler *handler0 = key;
+    const struct mmio_handler *handler1 = elem;
+
+    if ( handler0->addr < handler1->addr )
+        return -1;
+
+    if ( handler0->addr >= (handler1->addr + handler1->size) )
+        return 1;
+
+    return 0;
+}
+
+static void swap_mmio_handler(void *a, void *b)
+{
+    struct mmio_handler *t1 = a, *t2 = b;
+
+    SWAP(*t1, *t2);
+}
+
+/*
+ * Return a copy of the matching handler rather than a pointer into
+ * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
+ * array, so an escaped pointer could refer to a different (or torn)
+ * entry once the lock is dropped.  The copy stays valid as the ops
+ * structures are never freed.
+ */
+static bool find_mmio_handler(struct domain *d, paddr_t gpa,
+                              struct mmio_handler *out)
+{
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler key = { .addr = gpa };
+    const struct mmio_handler *handler;
+
+    read_lock(&vmmio->lock);
+    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
+                      sizeof(*handler), cmp_mmio_handler);
+    if ( handler )
+        *out = *handler;
+    read_unlock(&vmmio->lock);
+
+    return handler != NULL;
+}
+
+enum io_state try_handle_mmio(mmio_info_t *info)
+{
+    struct vcpu *v = current;
+    struct mmio_handler handler = {};
+
+    if ( !find_mmio_handler(v->domain, info->gpa, &handler) )
+        return IO_UNHANDLED;
+
+    if ( info->is_write )
+        return handle_write(&handler, v, info);
+    else
+        return handle_read(&handler, v, info);
+}
+
+void register_mmio_handler(struct domain *d,
+                           const struct mmio_handler_ops *ops,
+                           paddr_t addr, paddr_t size)
+{
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler *handler;
+
+    write_lock(&vmmio->lock);
+
+    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
+
+    handler = &vmmio->handlers[vmmio->num_entries];
+    handler->ops = ops;
+    handler->addr = addr;
+    handler->size = size;
+    vmmio->num_entries++;
+
+    /* Sort mmio handlers in ascending order based on base address */
+    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
+         cmp_mmio_handler, swap_mmio_handler);
+
+    write_unlock(&vmmio->lock);
+}
+
+int domain_io_init(struct domain *d, unsigned int max_count)
+{
+    rwlock_init(&d->arch.vmmio.lock);
+    d->arch.vmmio.num_entries = 0;
+    d->arch.vmmio.max_num_entries = max_count;
+    d->arch.vmmio.handlers = xvzalloc_array(struct mmio_handler, max_count);
+    if ( !d->arch.vmmio.handlers )
+        return -ENOMEM;
+
+    return 0;
+}
+
+void domain_io_free(struct domain *d)
+{
+    XVFREE(d->arch.vmmio.handlers);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.54.0

Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Jan Beulich]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> RISC-V guests can expose several virtual interrupt controllers at
> distinct GPA ranges: vPLIC (hasn't been introduced yet) for legacy machines,
> vAPLIC and vIMSIC for AIA-compliant ones (is being introduced in the follow
> up patches). Routing MMIO faults via a per-device is_access() check in the
> trap handler would couple it to every device it must serve, requiring a
> new conditional branch in the fault path each time a new emulated device is
> added.
> 
> Introduce a per-domain MMIO handler registration table, modeled
> after the equivalent ARM framework, so that virtual devices
> self-register their GPA ranges and read/write callbacks at domain
> creation time. The MMIO fault path delegates to a single
> try_handle_mmio() entry point and remains agnostic of which device
> owns a particular address.
> 
> Subsequent patches wire this into arch_domain_create() and the MMIO fault
> path in traps.c.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
> Note that find_mmio_handler() and try_handle_mmio() is handling found
> handler differently for now in comparison to Arm. But this behaviour will
> be aligned at the end. Look at discussion:
>   https://lore.kernel.org/xen-devel/cd78972e-88d5-471d-a201-5f9cd1392c73@gmail.com/T/#t
> ---
> ---
>  xen/arch/riscv/Makefile             |   1 +
>  xen/arch/riscv/domain.c             |   4 +
>  xen/arch/riscv/include/asm/domain.h |   3 +
>  xen/arch/riscv/include/asm/mmio.h   |  63 ++++++++++++
>  xen/arch/riscv/mmio.c               | 145 ++++++++++++++++++++++++++++
>  5 files changed, 216 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/mmio.h
>  create mode 100644 xen/arch/riscv/mmio.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 046f73f4d87c..c452ebc3cf61 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -14,6 +14,7 @@ obj-y += intc.o
>  obj-y += irq.o
>  obj-y += kernel.init.o
>  obj-y += mm.o
> +obj-y += mmio.o
>  obj-y += p2m.o
>  obj-y += paging.o
>  obj-y += pt.o
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index 4db9c28662c7..1e6f0ef66c2f 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -12,6 +12,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/csr.h>
>  #include <asm/intc.h>
> +#include <asm/mmio.h>
>  #include <asm/riscv_encoding.h>
>  #include <asm/vtimer.h>
>  
> @@ -308,6 +309,9 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = p2m_init(d, config)) != 0)
>          goto fail;
>  
> +    if ( (rc = domain_io_init(d, MAX_IO_HANDLER)) != 0 )
> +        goto fail;

Why does MAX_IO_HANDLER need passing into the function? Isn't that a global
boundary?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/mmio.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef RISCV_MMIO_H
> +#define RISCV_MMIO_H
> +
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +
> +#define MAX_IO_HANDLER  16
> +
> +typedef struct {
> +    paddr_t gpa;
> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
> +    bool is_write;
> +    register_t data;   /* store: value to write; load: value read (set by handler) */
> +} mmio_info_t;
> +
> +enum io_state
> +{
> +    IO_ABORT,       /* The IO was handled and led to an abort. */
> +    IO_HANDLED,     /* The IO was successfully handled. */
> +    IO_UNHANDLED,   /* No handler found for the IO. */
> +};
> +
> +typedef enum io_state (*mmio_read_t)(struct vcpu *v, mmio_info_t *info,
> +                                     register_t *r);
> +typedef enum io_state (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
> +                                      register_t r);

Can't info be pointer-to-const in the write case? In both cases, why is there
both "r" passed into the function as well as the info->data field, supposedly
(as per the comment) serving the same purpose?

Furthermore I think it helps if ...

> +struct mmio_handler_ops {
> +    mmio_read_t read;
> +    mmio_write_t write;

... pointer-ness is easily seen at use sites. I.e.

typedef enum io_state mmio_read_t(struct vcpu *v, mmio_info_t *info,
                                  register_t *r);
typedef enum io_state mmio_write_t(struct vcpu *v, const mmio_info_t *info,
                                   register_t r);

struct mmio_handler_ops {
    mmio_read_t *read;
    mmio_write_t *write;
};

> +};
> +
> +struct mmio_handler {
> +    paddr_t addr;
> +    paddr_t size;
> +    const struct mmio_handler_ops *ops;
> +};
> +
> +struct vmmio {
> +    unsigned int num_entries;
> +    unsigned int max_num_entries;
> +    rwlock_t lock;
> +    struct mmio_handler *handlers;

There shouldn't be any writes through this pointer, should there? In which
case it (once again) wants to be pointer-to-const.

> --- /dev/null
> +++ b/xen/arch/riscv/mmio.c
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) Vates
> + */
> +
> +#include <xen/bsearch.h>
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +#include <xen/sched.h>
> +#include <xen/sort.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <asm/current.h>
> +#include <asm/mmio.h>
> +
> +static enum io_state handle_read(const struct mmio_handler *handler,
> +                                 struct vcpu *v,
> +                                 mmio_info_t *info)
> +{
> +    register_t r = 0;
> +    enum io_state rc;
> +
> +    rc = handler->ops->read(v, info, &r);
> +    if ( rc == IO_HANDLED )
> +        info->data = r;

Extending my earlier comment: Why could ->read() not put the value directly
into info->data? And why ...

> +static enum io_state handle_write(const struct mmio_handler *handler,
> +                                  struct vcpu *v,
> +                                  mmio_info_t *info)
> +{
> +    return handler->ops->write(v, info, info->data);

... can't write take the value directly from info->data?

> +}
> +
> +/* Assumes mmio regions are not overlapping. */

Are you guaranteeing this anywhere?

> +static int cmp_mmio_handler(const void *key, const void *elem)
> +{
> +    const struct mmio_handler *handler0 = key;
> +    const struct mmio_handler *handler1 = elem;
> +
> +    if ( handler0->addr < handler1->addr )
> +        return -1;
> +
> +    if ( handler0->addr >= (handler1->addr + handler1->size) )
> +        return 1;
> +
> +    return 0;
> +}
> +
> +static void swap_mmio_handler(void *a, void *b)
> +{
> +    struct mmio_handler *t1 = a, *t2 = b;
> +
> +    SWAP(*t1, *t2);
> +}
> +
> +/*
> + * Return a copy of the matching handler rather than a pointer into
> + * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
> + * array, so an escaped pointer could refer to a different (or torn)
> + * entry once the lock is dropped.  The copy stays valid as the ops
> + * structures are never freed.
> + */
> +static bool find_mmio_handler(struct domain *d, paddr_t gpa,
> +                              struct mmio_handler *out)
> +{
> +    struct vmmio *vmmio = &d->arch.vmmio;
> +    struct mmio_handler key = { .addr = gpa };
> +    const struct mmio_handler *handler;
> +
> +    read_lock(&vmmio->lock);
> +    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
> +                      sizeof(*handler), cmp_mmio_handler);

So beyond the assumption stated further up you also assume the array to
be sorted. Which you ...

> +void register_mmio_handler(struct domain *d,
> +                           const struct mmio_handler_ops *ops,
> +                           paddr_t addr, paddr_t size)
> +{
> +    struct vmmio *vmmio = &d->arch.vmmio;
> +    struct mmio_handler *handler;
> +
> +    write_lock(&vmmio->lock);
> +
> +    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);

(Do we really need to crash in such a case? Can't we just fail domain
creation?)

> +    handler = &vmmio->handlers[vmmio->num_entries];
> +    handler->ops = ops;
> +    handler->addr = addr;
> +    handler->size = size;
> +    vmmio->num_entries++;
> +
> +    /* Sort mmio handlers in ascending order based on base address */
> +    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
> +         cmp_mmio_handler, swap_mmio_handler);

... arrange for here, yet in a pretty inefficient way: Inserting in an
already sorted list can be had without recurring calls to sort().

> +int domain_io_init(struct domain *d, unsigned int max_count)
> +{
> +    rwlock_init(&d->arch.vmmio.lock);
> +    d->arch.vmmio.num_entries = 0;
> +    d->arch.vmmio.max_num_entries = max_count;
> +    d->arch.vmmio.handlers = xvzalloc_array(struct mmio_handler, max_count);

If already an allocation is needed in all cases, why not allocate struct
vmmio, defined like this:

struct vmmio {
    unsigned int num_entries;
    unsigned int max_num_entries;
    rwlock_t lock;
    struct mmio_handler handlers[];
};

and then using xvzalloc_flex_struct(). Or yet simpler if (as mentioned
elsewhere) max_count doesn't need passing into here:

struct vmmio {
    unsigned int num_entries;
    unsigned int max_num_entries;
    rwlock_t lock;
    struct mmio_handler handlers[MAX_IO_HANDLER];
};

Jan

Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Oleksii Kurochko]

On 7/28/26 2:23 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> RISC-V guests can expose several virtual interrupt controllers at
>> distinct GPA ranges: vPLIC (hasn't been introduced yet) for legacy machines,
>> vAPLIC and vIMSIC for AIA-compliant ones (is being introduced in the follow
>> up patches). Routing MMIO faults via a per-device is_access() check in the
>> trap handler would couple it to every device it must serve, requiring a
>> new conditional branch in the fault path each time a new emulated device is
>> added.
>>
>> Introduce a per-domain MMIO handler registration table, modeled
>> after the equivalent ARM framework, so that virtual devices
>> self-register their GPA ranges and read/write callbacks at domain
>> creation time. The MMIO fault path delegates to a single
>> try_handle_mmio() entry point and remains agnostic of which device
>> owns a particular address.
>>
>> Subsequent patches wire this into arch_domain_create() and the MMIO fault
>> path in traps.c.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>> ---
>> Note that find_mmio_handler() and try_handle_mmio() is handling found
>> handler differently for now in comparison to Arm. But this behaviour will
>> be aligned at the end. Look at discussion:
>>    https://lore.kernel.org/xen-devel/cd78972e-88d5-471d-a201-5f9cd1392c73@gmail.com/T/#t
>> ---
>> ---
>>   xen/arch/riscv/Makefile             |   1 +
>>   xen/arch/riscv/domain.c             |   4 +
>>   xen/arch/riscv/include/asm/domain.h |   3 +
>>   xen/arch/riscv/include/asm/mmio.h   |  63 ++++++++++++
>>   xen/arch/riscv/mmio.c               | 145 ++++++++++++++++++++++++++++
>>   5 files changed, 216 insertions(+)
>>   create mode 100644 xen/arch/riscv/include/asm/mmio.h
>>   create mode 100644 xen/arch/riscv/mmio.c
>>
>> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
>> index 046f73f4d87c..c452ebc3cf61 100644
>> --- a/xen/arch/riscv/Makefile
>> +++ b/xen/arch/riscv/Makefile
>> @@ -14,6 +14,7 @@ obj-y += intc.o
>>   obj-y += irq.o
>>   obj-y += kernel.init.o
>>   obj-y += mm.o
>> +obj-y += mmio.o
>>   obj-y += p2m.o
>>   obj-y += paging.o
>>   obj-y += pt.o
>> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
>> index 4db9c28662c7..1e6f0ef66c2f 100644
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -12,6 +12,7 @@
>>   #include <asm/cpufeature.h>
>>   #include <asm/csr.h>
>>   #include <asm/intc.h>
>> +#include <asm/mmio.h>
>>   #include <asm/riscv_encoding.h>
>>   #include <asm/vtimer.h>
>>   
>> @@ -308,6 +309,9 @@ int arch_domain_create(struct domain *d,
>>       if ( (rc = p2m_init(d, config)) != 0)
>>           goto fail;
>>   
>> +    if ( (rc = domain_io_init(d, MAX_IO_HANDLER)) != 0 )
>> +        goto fail;
> 
> Why does MAX_IO_HANDLER need passing into the function? Isn't that a global
> boundary?

Good question. Considering that all domains are initialized with 
MAX_IO_HANDLER I think we could drop an argument for domain_io_init() 
and just use MAX_IO_HANDLER inside it for init. of handlers array.

> 
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/mmio.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef RISCV_MMIO_H
>> +#define RISCV_MMIO_H
>> +
>> +#include <xen/lib.h>
>> +#include <xen/rwlock.h>
>> +
>> +#define MAX_IO_HANDLER  16
>> +
>> +typedef struct {
>> +    paddr_t gpa;
>> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
>> +    bool is_write;
>> +    register_t data;   /* store: value to write; load: value read (set by handler) */
>> +} mmio_info_t;
>> +
>> +enum io_state
>> +{
>> +    IO_ABORT,       /* The IO was handled and led to an abort. */
>> +    IO_HANDLED,     /* The IO was successfully handled. */
>> +    IO_UNHANDLED,   /* No handler found for the IO. */
>> +};
>> +
>> +typedef enum io_state (*mmio_read_t)(struct vcpu *v, mmio_info_t *info,
>> +                                     register_t *r);
>> +typedef enum io_state (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
>> +                                      register_t r);
> 
> Can't info be pointer-to-const in the write case?

With the current implementaion it could be done for both mmio_read_t and 
mmio_write_t as value is return through r argument.

In both cases, why is there
> both "r" passed into the function as well as the info->data field, supposedly
> (as per the comment) serving the same purpose?

Agree, we don't need both "r" and info->data as they are serving the 
same purpose.

But I don't know which one option is actually better to drop "r" 
argument or drop ->data member in mmio_info_t.


> 
> Furthermore I think it helps if ...
> 
>> +struct mmio_handler_ops {
>> +    mmio_read_t read;
>> +    mmio_write_t write;
> 
> ... pointer-ness is easily seen at use sites. I.e.
> 
> typedef enum io_state mmio_read_t(struct vcpu *v, mmio_info_t *info,
>                                    register_t *r);
> typedef enum io_state mmio_write_t(struct vcpu *v, const mmio_info_t *info,
>                                     register_t r);
> 
> struct mmio_handler_ops {
>      mmio_read_t *read;
>      mmio_write_t *write;
> };
> 

I will apply that.

>> +};
>> +
>> +struct mmio_handler {
>> +    paddr_t addr;
>> +    paddr_t size;
>> +    const struct mmio_handler_ops *ops;
>> +};
>> +
>> +struct vmmio {
>> +    unsigned int num_entries;
>> +    unsigned int max_num_entries;
>> +    rwlock_t lock;
>> +    struct mmio_handler *handlers;
> 
> There shouldn't be any writes through this pointer, should there? In which
> case it (once again) wants to be pointer-to-const.

Agree, it should be const.

> 
>> --- /dev/null
>> +++ b/xen/arch/riscv/mmio.c
>> @@ -0,0 +1,145 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) Vates
>> + */
>> +
>> +#include <xen/bsearch.h>
>> +#include <xen/lib.h>
>> +#include <xen/rwlock.h>
>> +#include <xen/sched.h>
>> +#include <xen/sort.h>
>> +#include <xen/xvmalloc.h>
>> +
>> +#include <asm/current.h>
>> +#include <asm/mmio.h>
>> +
>> +static enum io_state handle_read(const struct mmio_handler *handler,
>> +                                 struct vcpu *v,
>> +                                 mmio_info_t *info)
>> +{
>> +    register_t r = 0;
>> +    enum io_state rc;
>> +
>> +    rc = handler->ops->read(v, info, &r);
>> +    if ( rc == IO_HANDLED )
>> +        info->data = r;
> 
> Extending my earlier comment: Why could ->read() not put the value directly
> into info->data? And why ...
> 
>> +static enum io_state handle_write(const struct mmio_handler *handler,
>> +                                  struct vcpu *v,
>> +                                  mmio_info_t *info)
>> +{
>> +    return handler->ops->write(v, info, info->data);
> 
> ... can't write take the value directly from info->data?

I totally agree, it can. Do you think it is better to keep ->data and 
drop an argument 'r' or vice versa?

> 
>> +}
>> +
>> +/* Assumes mmio regions are not overlapping. */
> 
> Are you guaranteeing this anywhere?

There is no such guarantee. register_mmio_handler() simply adds the 
handler to the handlers array without performing any checks. I can add 
such a check. The only question is whether it should be enabled only in 
debug builds or in all builds.

I assume this is a rare case, and overlapping regions would indicate 
that something is wrong with the guest's memory layout configuration so 
it seems like it would be enough to add only for debug builds.

> 
>> +static int cmp_mmio_handler(const void *key, const void *elem)
>> +{
>> +    const struct mmio_handler *handler0 = key;
>> +    const struct mmio_handler *handler1 = elem;
>> +
>> +    if ( handler0->addr < handler1->addr )
>> +        return -1;
>> +
>> +    if ( handler0->addr >= (handler1->addr + handler1->size) )
>> +        return 1;
>> +
>> +    return 0;
>> +}
>> +
>> +static void swap_mmio_handler(void *a, void *b)
>> +{
>> +    struct mmio_handler *t1 = a, *t2 = b;
>> +
>> +    SWAP(*t1, *t2);
>> +}
>> +
>> +/*
>> + * Return a copy of the matching handler rather than a pointer into
>> + * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
>> + * array, so an escaped pointer could refer to a different (or torn)
>> + * entry once the lock is dropped.  The copy stays valid as the ops
>> + * structures are never freed.
>> + */
>> +static bool find_mmio_handler(struct domain *d, paddr_t gpa,
>> +                              struct mmio_handler *out)
>> +{
>> +    struct vmmio *vmmio = &d->arch.vmmio;
>> +    struct mmio_handler key = { .addr = gpa };
>> +    const struct mmio_handler *handler;
>> +
>> +    read_lock(&vmmio->lock);
>> +    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
>> +                      sizeof(*handler), cmp_mmio_handler);
> 
> So beyond the assumption stated further up you also assume the array to
> be sorted. Which you ...
> 
>> +void register_mmio_handler(struct domain *d,
>> +                           const struct mmio_handler_ops *ops,
>> +                           paddr_t addr, paddr_t size)
>> +{
>> +    struct vmmio *vmmio = &d->arch.vmmio;
>> +    struct mmio_handler *handler;
>> +
>> +    write_lock(&vmmio->lock);
>> +
>> +    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
> 
> (Do we really need to crash in such a case? Can't we just fail domain
> creation?)

Generally, no. However, the approach used by Arm's dom0less solution is 
to crash as soon as any issue occurs instead of trying to continue 
running other domains, so I follow the same approach for RISC-V.

Even if I return an error here, the common dom0less code will panic anyway.

> 
>> +    handler = &vmmio->handlers[vmmio->num_entries];
>> +    handler->ops = ops;
>> +    handler->addr = addr;
>> +    handler->size = size;
>> +    vmmio->num_entries++;
>> +
>> +    /* Sort mmio handlers in ascending order based on base address */
>> +    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
>> +         cmp_mmio_handler, swap_mmio_handler);
> 
> ... arrange for here, yet in a pretty inefficient way: Inserting in an
> already sorted list can be had without recurring calls to sort().

Good point. I will rework that.
> 
>> +int domain_io_init(struct domain *d, unsigned int max_count)
>> +{
>> +    rwlock_init(&d->arch.vmmio.lock);
>> +    d->arch.vmmio.num_entries = 0;
>> +    d->arch.vmmio.max_num_entries = max_count;
>> +    d->arch.vmmio.handlers = xvzalloc_array(struct mmio_handler, max_count);
> 
> If already an allocation is needed in all cases, why not allocate struct
> vmmio, defined like this:
> 
> struct vmmio {
>      unsigned int num_entries;
>      unsigned int max_num_entries;
>      rwlock_t lock;
>      struct mmio_handler handlers[];
> };
> 
> and then using xvzalloc_flex_struct(). Or yet simpler if (as mentioned
> elsewhere) max_count doesn't need passing into here:
> 
> struct vmmio {
>      unsigned int num_entries;
>      unsigned int max_num_entries;
>      rwlock_t lock;
>      struct mmio_handler handlers[MAX_IO_HANDLER];
> };
> 
Agree, both option are good to me. Considering that we are going to use 
MAX_IO_HANDLER then second option is really better for now.

Thanks!

~ Oleksii

Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Jan Beulich]

On 30.07.2026 18:03, Oleksii Kurochko wrote:
> On 7/28/26 2:23 PM, Jan Beulich wrote:
>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/mmio.c
>>> @@ -0,0 +1,145 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (C) Vates
>>> + */
>>> +
>>> +#include <xen/bsearch.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/rwlock.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/sort.h>
>>> +#include <xen/xvmalloc.h>
>>> +
>>> +#include <asm/current.h>
>>> +#include <asm/mmio.h>
>>> +
>>> +static enum io_state handle_read(const struct mmio_handler *handler,
>>> +                                 struct vcpu *v,
>>> +                                 mmio_info_t *info)
>>> +{
>>> +    register_t r = 0;
>>> +    enum io_state rc;
>>> +
>>> +    rc = handler->ops->read(v, info, &r);
>>> +    if ( rc == IO_HANDLED )
>>> +        info->data = r;
>>
>> Extending my earlier comment: Why could ->read() not put the value directly
>> into info->data? And why ...
>>
>>> +static enum io_state handle_write(const struct mmio_handler *handler,
>>> +                                  struct vcpu *v,
>>> +                                  mmio_info_t *info)
>>> +{
>>> +    return handler->ops->write(v, info, info->data);
>>
>> ... can't write take the value directly from info->data?
> 
> I totally agree, it can. Do you think it is better to keep ->data and 
> drop an argument 'r' or vice versa?

How can I know? You know future plans you have.

>>> +}
>>> +
>>> +/* Assumes mmio regions are not overlapping. */
>>
>> Are you guaranteeing this anywhere?
> 
> There is no such guarantee. register_mmio_handler() simply adds the 
> handler to the handlers array without performing any checks. I can add 
> such a check. The only question is whether it should be enabled only in 
> debug builds or in all builds.

Depends on what other badness can happen when this is violated. My gut
feeling is that checking in debug builds may be enough.

>>> +/*
>>> + * Return a copy of the matching handler rather than a pointer into
>>> + * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
>>> + * array, so an escaped pointer could refer to a different (or torn)
>>> + * entry once the lock is dropped.  The copy stays valid as the ops
>>> + * structures are never freed.
>>> + */
>>> +static bool find_mmio_handler(struct domain *d, paddr_t gpa,
>>> +                              struct mmio_handler *out)
>>> +{
>>> +    struct vmmio *vmmio = &d->arch.vmmio;
>>> +    struct mmio_handler key = { .addr = gpa };
>>> +    const struct mmio_handler *handler;
>>> +
>>> +    read_lock(&vmmio->lock);
>>> +    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
>>> +                      sizeof(*handler), cmp_mmio_handler);
>>
>> So beyond the assumption stated further up you also assume the array to
>> be sorted. Which you ...
>>
>>> +void register_mmio_handler(struct domain *d,
>>> +                           const struct mmio_handler_ops *ops,
>>> +                           paddr_t addr, paddr_t size)
>>> +{
>>> +    struct vmmio *vmmio = &d->arch.vmmio;
>>> +    struct mmio_handler *handler;
>>> +
>>> +    write_lock(&vmmio->lock);
>>> +
>>> +    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
>>
>> (Do we really need to crash in such a case? Can't we just fail domain
>> creation?)
> 
> Generally, no. However, the approach used by Arm's dom0less solution is 
> to crash as soon as any issue occurs instead of trying to continue 
> running other domains, so I follow the same approach for RISC-V.
> 
> Even if I return an error here, the common dom0less code will panic anyway.

That's the policy there, but you're writing code here also for the case where
Dom0 creates domains.

Jan

Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Oleksii Kurochko]

On 7/30/26 6:09 PM, Jan Beulich wrote:
> On 30.07.2026 18:03, Oleksii Kurochko wrote:
>> On 7/28/26 2:23 PM, Jan Beulich wrote:
>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/mmio.c
>>>> @@ -0,0 +1,145 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (C) Vates
>>>> + */
>>>> +
>>>> +#include <xen/bsearch.h>
>>>> +#include <xen/lib.h>
>>>> +#include <xen/rwlock.h>
>>>> +#include <xen/sched.h>
>>>> +#include <xen/sort.h>
>>>> +#include <xen/xvmalloc.h>
>>>> +
>>>> +#include <asm/current.h>
>>>> +#include <asm/mmio.h>
>>>> +
>>>> +static enum io_state handle_read(const struct mmio_handler *handler,
>>>> +                                 struct vcpu *v,
>>>> +                                 mmio_info_t *info)
>>>> +{
>>>> +    register_t r = 0;
>>>> +    enum io_state rc;
>>>> +
>>>> +    rc = handler->ops->read(v, info, &r);
>>>> +    if ( rc == IO_HANDLED )
>>>> +        info->data = r;
>>>
>>> Extending my earlier comment: Why could ->read() not put the value directly
>>> into info->data? And why ...
>>>
>>>> +static enum io_state handle_write(const struct mmio_handler *handler,
>>>> +                                  struct vcpu *v,
>>>> +                                  mmio_info_t *info)
>>>> +{
>>>> +    return handler->ops->write(v, info, info->data);
>>>
>>> ... can't write take the value directly from info->data?
>>
>> I totally agree, it can. Do you think it is better to keep ->data and
>> drop an argument 'r' or vice versa?
> 
> How can I know? You know future plans you have.
> 
>>>> +}
>>>> +
>>>> +/* Assumes mmio regions are not overlapping. */
>>>
>>> Are you guaranteeing this anywhere?
>>
>> There is no such guarantee. register_mmio_handler() simply adds the
>> handler to the handlers array without performing any checks. I can add
>> such a check. The only question is whether it should be enabled only in
>> debug builds or in all builds.
> 
> Depends on what other badness can happen when this is violated. My gut
> feeling is that checking in debug builds may be enough.

Overlapping regions would be a Xen bug rather than something a guest can 
trigger — register_mmio_handler() is only called from Xen's own emulated 
device code, so the layout isn't under guest control.

The badness is worse than just mis-emulating one device though: 
cmp_mmio_handler() is used both by bsearch() and by sort(). With 
overlapping regions it's no longer a consistent ordering, so sort() may 
produce an arbitrary order and lookups can then fail (or match the wrong 
handler) even for regions which don't overlap themselves. That would 
show up as a spurious fault injected into the guest, which is quite hard 
to debug.

So I agree a check is worthwhile; I'll add one under CONFIG_DEBUG in 
register_mmio_handler().

> 
>>>> +/*
>>>> + * Return a copy of the matching handler rather than a pointer into
>>>> + * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
>>>> + * array, so an escaped pointer could refer to a different (or torn)
>>>> + * entry once the lock is dropped.  The copy stays valid as the ops
>>>> + * structures are never freed.
>>>> + */
>>>> +static bool find_mmio_handler(struct domain *d, paddr_t gpa,
>>>> +                              struct mmio_handler *out)
>>>> +{
>>>> +    struct vmmio *vmmio = &d->arch.vmmio;
>>>> +    struct mmio_handler key = { .addr = gpa };
>>>> +    const struct mmio_handler *handler;
>>>> +
>>>> +    read_lock(&vmmio->lock);
>>>> +    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
>>>> +                      sizeof(*handler), cmp_mmio_handler);
>>>
>>> So beyond the assumption stated further up you also assume the array to
>>> be sorted. Which you ...
>>>
>>>> +void register_mmio_handler(struct domain *d,
>>>> +                           const struct mmio_handler_ops *ops,
>>>> +                           paddr_t addr, paddr_t size)
>>>> +{
>>>> +    struct vmmio *vmmio = &d->arch.vmmio;
>>>> +    struct mmio_handler *handler;
>>>> +
>>>> +    write_lock(&vmmio->lock);
>>>> +
>>>> +    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
>>>
>>> (Do we really need to crash in such a case? Can't we just fail domain
>>> creation?)
>>
>> Generally, no. However, the approach used by Arm's dom0less solution is
>> to crash as soon as any issue occurs instead of trying to continue
>> running other domains, so I follow the same approach for RISC-V.
>>
>> Even if I return an error here, the common dom0less code will panic anyway.
> 
> That's the policy there, but you're writing code here also for the case where
> Dom0 creates domains.

Missed that. In this case I agree that it would be nice to return something.

Thanks.

~ Oleksii

Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Jan Beulich]

On 31.07.2026 17:24, Oleksii Kurochko wrote:
> On 7/30/26 6:09 PM, Jan Beulich wrote:
>> On 30.07.2026 18:03, Oleksii Kurochko wrote:
>>> On 7/28/26 2:23 PM, Jan Beulich wrote:
>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/riscv/mmio.c
>>>>> @@ -0,0 +1,145 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (C) Vates
>>>>> + */
>>>>> +
>>>>> +#include <xen/bsearch.h>
>>>>> +#include <xen/lib.h>
>>>>> +#include <xen/rwlock.h>
>>>>> +#include <xen/sched.h>
>>>>> +#include <xen/sort.h>
>>>>> +#include <xen/xvmalloc.h>
>>>>> +
>>>>> +#include <asm/current.h>
>>>>> +#include <asm/mmio.h>
>>>>> +
>>>>> +static enum io_state handle_read(const struct mmio_handler *handler,
>>>>> +                                 struct vcpu *v,
>>>>> +                                 mmio_info_t *info)
>>>>> +{
>>>>> +    register_t r = 0;
>>>>> +    enum io_state rc;
>>>>> +
>>>>> +    rc = handler->ops->read(v, info, &r);
>>>>> +    if ( rc == IO_HANDLED )
>>>>> +        info->data = r;
>>>>
>>>> Extending my earlier comment: Why could ->read() not put the value directly
>>>> into info->data? And why ...
>>>>
>>>>> +static enum io_state handle_write(const struct mmio_handler *handler,
>>>>> +                                  struct vcpu *v,
>>>>> +                                  mmio_info_t *info)
>>>>> +{
>>>>> +    return handler->ops->write(v, info, info->data);
>>>>
>>>> ... can't write take the value directly from info->data?
>>>
>>> I totally agree, it can. Do you think it is better to keep ->data and
>>> drop an argument 'r' or vice versa?
>>
>> How can I know? You know future plans you have.
>>
>>>>> +}
>>>>> +
>>>>> +/* Assumes mmio regions are not overlapping. */
>>>>
>>>> Are you guaranteeing this anywhere?
>>>
>>> There is no such guarantee. register_mmio_handler() simply adds the
>>> handler to the handlers array without performing any checks. I can add
>>> such a check. The only question is whether it should be enabled only in
>>> debug builds or in all builds.
>>
>> Depends on what other badness can happen when this is violated. My gut
>> feeling is that checking in debug builds may be enough.
> 
> Overlapping regions would be a Xen bug rather than something a guest can 
> trigger — register_mmio_handler() is only called from Xen's own emulated 
> device code, so the layout isn't under guest control.
> 
> The badness is worse than just mis-emulating one device though: 
> cmp_mmio_handler() is used both by bsearch() and by sort(). With 
> overlapping regions it's no longer a consistent ordering, so sort() may 
> produce an arbitrary order and lookups can then fail (or match the wrong 
> handler) even for regions which don't overlap themselves. That would 
> show up as a spurious fault injected into the guest, which is quite hard 
> to debug.

Didn't you say you'd get rid of the use of sort()?

> So I agree a check is worthwhile; I'll add one under CONFIG_DEBUG in 
> register_mmio_handler().

Some assertion then hopefully, rather than an open-coded use of CONFIG_DEBUG.

Jan

Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Oleksii Kurochko]

On 8/3/26 12:41 PM, Jan Beulich wrote:
> On 31.07.2026 17:24, Oleksii Kurochko wrote:
>> On 7/30/26 6:09 PM, Jan Beulich wrote:
>>> On 30.07.2026 18:03, Oleksii Kurochko wrote:
>>>> On 7/28/26 2:23 PM, Jan Beulich wrote:
>>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/arch/riscv/mmio.c
>>>>>> @@ -0,0 +1,145 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>> +/*
>>>>>> + * Copyright (C) Vates
>>>>>> + */
>>>>>> +
>>>>>> +#include <xen/bsearch.h>
>>>>>> +#include <xen/lib.h>
>>>>>> +#include <xen/rwlock.h>
>>>>>> +#include <xen/sched.h>
>>>>>> +#include <xen/sort.h>
>>>>>> +#include <xen/xvmalloc.h>
>>>>>> +
>>>>>> +#include <asm/current.h>
>>>>>> +#include <asm/mmio.h>
>>>>>> +
>>>>>> +static enum io_state handle_read(const struct mmio_handler *handler,
>>>>>> +                                 struct vcpu *v,
>>>>>> +                                 mmio_info_t *info)
>>>>>> +{
>>>>>> +    register_t r = 0;
>>>>>> +    enum io_state rc;
>>>>>> +
>>>>>> +    rc = handler->ops->read(v, info, &r);
>>>>>> +    if ( rc == IO_HANDLED )
>>>>>> +        info->data = r;
>>>>>
>>>>> Extending my earlier comment: Why could ->read() not put the value directly
>>>>> into info->data? And why ...
>>>>>
>>>>>> +static enum io_state handle_write(const struct mmio_handler *handler,
>>>>>> +                                  struct vcpu *v,
>>>>>> +                                  mmio_info_t *info)
>>>>>> +{
>>>>>> +    return handler->ops->write(v, info, info->data);
>>>>>
>>>>> ... can't write take the value directly from info->data?
>>>>
>>>> I totally agree, it can. Do you think it is better to keep ->data and
>>>> drop an argument 'r' or vice versa?
>>>
>>> How can I know? You know future plans you have.
>>>
>>>>>> +}
>>>>>> +
>>>>>> +/* Assumes mmio regions are not overlapping. */
>>>>>
>>>>> Are you guaranteeing this anywhere?
>>>>
>>>> There is no such guarantee. register_mmio_handler() simply adds the
>>>> handler to the handlers array without performing any checks. I can add
>>>> such a check. The only question is whether it should be enabled only in
>>>> debug builds or in all builds.
>>>
>>> Depends on what other badness can happen when this is violated. My gut
>>> feeling is that checking in debug builds may be enough.
>>
>> Overlapping regions would be a Xen bug rather than something a guest can
>> trigger — register_mmio_handler() is only called from Xen's own emulated
>> device code, so the layout isn't under guest control.
>>
>> The badness is worse than just mis-emulating one device though:
>> cmp_mmio_handler() is used both by bsearch() and by sort(). With
>> overlapping regions it's no longer a consistent ordering, so sort() may
>> produce an arbitrary order and lookups can then fail (or match the wrong
>> handler) even for regions which don't overlap themselves. That would
>> show up as a spurious fault injected into the guest, which is quite hard
>> to debug.
> 
> Didn't you say you'd get rid of the use of sort()?
> 
Yes, I will. I just wrote that for the case if sort() will still present.

~ Oleksii

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

[Oleksii Kurochko]

Guests running under Xen program interrupt routing by writing to APLIC
MMIO registers. Xen must intercept these accesses to enforce interrupt
isolation between domains and to translate guest routing intent into the
underlying physical MSI topology.

Writes are gated by the domain's authorised interrupt bitmap so that a
guest cannot affect interrupts it does not own. TARGET register writes
additionally require translation of the hart and IMSIC guest-file
indices from virtual to physical, as the APLIC uses these fields
directly to compute the MSI delivery address.

Delegation (APLIC_SOURCECFG_D) is not yet supported.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Reviewed-by: Baptiste Le Duc <baptiste.le-duc@vates.tech> # vaplic_mmio_{read,write}

The downstream changes related to `vaplic_mmio_{read,write}` were originally
in a separate patch (which was reviewed by Baptiste). However, before
upstreaming, it was decided to merge them into the current patch.
I added `Reviewed-by: Baptiste` in this form for now, but Baptiste will
probably review the remaining changes as well.
Once that happens, I'll simply move the `Reviewed-by` tag up and
remove the `#`.

If it will be easier I can move that changes to separate commit.
---
Changes in v3:
 - Drop ->is_access(), ->emulate_{store,load}() from struct vintc_ops and
   use MMIO framework instead.
 - Rename local variable base_ppn to tppn to not confuse it with base_ppn
   from AIA specification in the correspondent formula.
 - Move aplic_msi_target_gen() from vaplic.c to aplic.c.
 - Extract static aplic_hart_field() helper to compute the combined
   hart + group-index field for the TARGET register.
 - Use MASK_INSR() with APLIC_TARGET_GUEST_IDX_MASK and
   APLIC_TARGET_HART_IDX_MASK instead of open-coded shifts in
   aplic_msi_target_gen().
 - Add APLIC_xMSICFGADDR_PPN_SHIFT, _HHX_MASK, and _HHX_SHIFT macros
   to asm/aplic.h; use them in aplic_hart_field() to extract the group
   index from base_ppn.
 - Move APLIC_TARGET_{HART,GUEST}_IDX_MASK definitions to the preceding
   patch ("add missing APLIC register offsets, masks").
 - xen/arch/riscv/aplic.c:
   - Extend ASSERT() in aplic_hw_read_reg() and aplic_hw_write_reg() to
     also check 4-byte alignment via IS_ALIGNED(offset, sizeof(uint32_t)).
   - Use (volatile void __iomem *)aplic.regs + offset in readl()/writel()
     instead of the uintptr_t cast.
 - xen/arch/riscv/include/asm/aplic.h:
   - Drop unused APLIC_NUM_REGS macro.
   - Rewrite APLIC_SETCLR_OFFSET_MASK as
     (sizeof_field(struct aplic_regs, setip) - sizeof(uint32_t)) and add a
     comment explaining the choice of setip as a representative field.
 - xen/arch/riscv/include/asm/vaplic.h:
   - Change regs_size type from paddr_t to unsigned int.
 - xen/arch/riscv/vaplic.c:
   - s/regindx_to_irqn/regoffset_to_word_idx and add an explanatory comment.
   - s/irqsn/word_idx in generate_auth_mask().
   - Replace pointer-cast bitmap access in generate_auth_mask() with proper
     index arithmetic via first_bit to avoid strict-aliasing violation.
   - Add cf_check to vaplic_emulate_load().
   - s/vcpu/v in vaplic_emulate_load(), vaplic_emulate_store() and
     vaplic_is_access().
   - Fix comment typos: s/start for/start from/, s/substracting/subtracting/,
     drop stray 'of' after 'subtracting' in two comments.
   - AUTH_IRQ_BIT() intentionally uses '<' rather than '<=' when
     comparing irqn against nr_virqs: nr_virqs is a count of virtual IRQs
     and irqn is a 0-based index, so the valid range is
     [0, nr_virqs - 1] and irqn == nr_virqs is already out of bounds.
   - Update handling of 'case APLIC_DOMAINCFG'.
---
Changes in v2:
 - Merge the following patches into one:
    xen/riscv: add vaplic access check:
      - Add check that address is properly aligned.
      - Check vaplic range intead of APLIC one.
      - Return bool from vaplic_is_access instead of int.
    xen/riscv: emulate guest writes to virtual APLIC MMIO
      - Drop CALC_REG_VALUE.
      - Use unsigned int instead of uin32_t for offset.
      - s/.../subtracting in the comment.
      - start one line comments from the upper case.
      - Check the value before being written to sourcecfg register.
      - 'unsigned int' for loop index.
      - Omit unneessary braces.
      - s/vaplic_update_target/aplic_msi_target_gen.
      - Use IMSIC_MMIO_PAGE_SHIFT instead of 12 in aplic_msi_target_gen().
      - Drop explicit usage of APLIC register in store function.
      - Drop APLIC_REG_{GET,SET} macros and introudce APLIC specific funtcions.
      - Ignore write to SOURCECFG_BASE when value is out-of-range.
      - Drop ASSERT(!target_vcpu) inside handler of targer register setting,
        just avoid such writings + debug message.
      - domain_crash() instead of panic() in the case of default case.
      - Drop ASSERT() in APLIC_SOURCE_CFG_BASE case and use domain_crash()
        instead.
    xen/riscv: emulate guest reads from virtual APLIC MMIO:
      - s/regval_to_irqn/regindx_to_irqn.
      - pass to to_vaplic() a domain instead of vintc.
      - add check that load access is aligned.
      - instead of panic() just crash a domain().
      - use 'unsigned int' for local variable offset.
      - Return 0 in the case APLIC_CLRIE_BASE ...APLIC_CLRIE_LAST reading to
        follow AIA spec.
      - Drop explicit usage of physical APLIC registers.
---
---
 xen/arch/riscv/aplic-priv.h         |   2 +
 xen/arch/riscv/aplic.c              |  55 +++++
 xen/arch/riscv/include/asm/aplic.h  |  24 +++
 xen/arch/riscv/include/asm/imsic.h  |  10 +
 xen/arch/riscv/include/asm/vaplic.h |   3 +
 xen/arch/riscv/vaplic.c             | 301 ++++++++++++++++++++++++++++
 6 files changed, 395 insertions(+)

diff --git a/xen/arch/riscv/aplic-priv.h b/xen/arch/riscv/aplic-priv.h
index 1391837f89b9..96bc56dbe585 100644
--- a/xen/arch/riscv/aplic-priv.h
+++ b/xen/arch/riscv/aplic-priv.h
@@ -48,4 +48,6 @@ struct aplic_priv {
  */
 extern unsigned int guest_aplic_num_sources;
 
+uint32_t aplic_msi_target_gen(const struct vcpu *target_vcpu, uint32_t base_val);
+
 #endif /* ASM_RISCV_APLIC_PRIV_H */
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index 3681f0669efb..87f2134bc561 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -16,6 +16,7 @@
 #include <xen/irq.h>
 #include <xen/mm.h>
 #include <xen/sections.h>
+#include <xen/sched.h>
 #include <xen/spinlock.h>
 #include <xen/types.h>
 #include <xen/vmap.h>
@@ -38,6 +39,60 @@ static struct intc_info __ro_after_init aplic_info = {
     .hw_variant = INTC_APLIC,
 };
 
+static unsigned long aplic_hart_field(unsigned long hartid)
+{
+    const struct imsic_config *imsic = imsic_get_config();
+    unsigned int lhxw = imsic->hart_index_bits;
+    unsigned int hhxw = imsic->group_index_bits;
+    unsigned int hhxs =
+        imsic->group_index_shift - APLIC_xMSICFGADDR_PPN_SHIFT * 2;
+    unsigned long tppn =
+        imsic->msi[hartid].base_addr >> APLIC_xMSICFGADDR_PPN_SHIFT;
+    unsigned long group_index =
+        (tppn >> APLIC_xMSICFGADDR_PPN_HHX_SHIFT(hhxs)) &
+        APLIC_xMSICFGADDR_PPN_HHX_MASK(hhxw);
+
+    return (group_index << lhxw) | hartid;
+}
+
+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_id = cpuid_to_hartid(target_vcpu->processor);
+    unsigned long hart_field = aplic_hart_field(hart_id);
+
+    base_val &= APLIC_TARGET_EIID_MASK;
+    base_val |= MASK_INSR(guest_id, APLIC_TARGET_GUEST_IDX_MASK);
+    base_val |= MASK_INSR(hart_field, APLIC_TARGET_HART_IDX_MASK);
+
+    return base_val;
+}
+
+uint32_t aplic_hw_read_reg(unsigned int offset, uint32_t mask)
+{
+    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) & mask;
+    spin_unlock_irqrestore(&aplic.lock, flags);
+
+    return val;
+}
+
+void aplic_hw_write_reg(unsigned int offset, uint32_t value)
+{
+    unsigned long flags;
+
+    ASSERT((offset < aplic.size) && IS_ALIGNED(offset, sizeof(uint32_t)));
+
+    spin_lock_irqsave(&aplic.lock, flags);
+    writel(value, (volatile void __iomem *)aplic.regs + offset);
+    spin_unlock_irqrestore(&aplic.lock, flags);
+}
+
 static void __init aplic_init_hw_interrupts(void)
 {
     unsigned int i;
diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
index f22622b9a23f..4ae5fb8f26d1 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -28,6 +28,8 @@
 #define APLIC_DOMAINCFG_BE      BIT(0, U)
 
 /* sourcecfg register fields */
+#define APLIC_SOURCECFG_D       BIT(10, U)
+
 #define APLIC_SOURCECFG_SM_INACTIVE     0x0
 #define APLIC_SOURCECFG_SM_DETACH       0x1
 #define APLIC_SOURCECFG_SM_EDGE_RISE    0x4
@@ -38,6 +40,16 @@
 /* target register fields */
 #define APLIC_TARGET_HART_IDX_SHIFT 18
 #define APLIC_TARGET_EIID_MASK      0x7ff
+#define APLIC_TARGET_HART_IDX_MASK  0xfffc0000
+#define APLIC_TARGET_GUEST_IDX_MASK 0x3f000
+
+/* xmsicfgaddr/h register fields */
+#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_DOMAINCFG         0x0000
 #define APLIC_SOURCECFG_BASE    0x0004
@@ -77,6 +89,15 @@
 #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))
+
 struct aplic_regs {
     uint32_t domaincfg;         /* 0x0000 */
     uint32_t sourcecfg[1023];   /* 0x0004 */
@@ -120,4 +141,7 @@ struct aplic_regs {
     uint32_t target[1023];      /* 0x3008 */
 };
 
+uint32_t aplic_hw_read_reg(unsigned int offset, uint32_t mask);
+void aplic_hw_write_reg(unsigned int offset, uint32_t value);
+
 #endif /* ASM_RISCV_APLIC_H */
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index e1ec3d03c4e9..612f503b5799 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -40,6 +40,16 @@ struct imsic_config {
     /* Base address */
     paddr_t base_addr;
 
+    /*
+     * MSI Target Address Scheme
+     *
+     * XLEN-1                                                12     0
+     * |                                                     |     |
+     * -------------------------------------------------------------
+     * |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
+     * -------------------------------------------------------------
+     */
+
     /* Bits representing Guest index, HART index, and Group index */
     unsigned int guest_index_bits;
     unsigned int hart_index_bits;
diff --git a/xen/arch/riscv/include/asm/vaplic.h b/xen/arch/riscv/include/asm/vaplic.h
index 96080bfbc23b..7bf9247f4eae 100644
--- a/xen/arch/riscv/include/asm/vaplic.h
+++ b/xen/arch/riscv/include/asm/vaplic.h
@@ -26,6 +26,9 @@ struct vaplic_regs {
 struct vaplic {
     struct vintc vintc;
     struct vaplic_regs regs;
+
+    paddr_t regs_start;
+    unsigned int regs_size;
 };
 
 int domain_vaplic_init(struct domain *d);
diff --git a/xen/arch/riscv/vaplic.c b/xen/arch/riscv/vaplic.c
index 449240c5cd23..03240730e344 100644
--- 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,256 @@ 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 allocated_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 inline uint32_t generate_auth_mask(const struct domain *d,
+                                          unsigned int word_idx)
+{
+    unsigned int first_bit = word_idx * sizeof(uint32_t) * BITS_PER_BYTE;
+
+    if ( word_idx >= DIV_ROUND_UP(d->arch.vintc->nr_virqs,
+                                  sizeof(uint32_t) * BITS_PER_BYTE) )
+    {
+        dprintk(XENLOG_DEBUG, "incorrect word_idx(%u) is passed\n", word_idx);
+
+        return 0U;
+    }
+
+    return (uint32_t)(d->arch.vintc->used_irqs[first_bit / BITS_PER_LONG] >>
+                      (first_bit % BITS_PER_LONG));
+}
+
+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);
+
+    return 0;
+}
+
+static int cf_check vaplic_emulate_store(const struct vcpu *v,
+                                         unsigned long addr, uint32_t value)
+{
+    int rc = -EINVAL;
+    const struct domain *d = v->domain;
+    unsigned int offset = addr & APLIC_REG_OFFSET_MASK;
+
+    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(d, word_idx);
+
+        break;
+    }
+
+    case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
+        if ( value & APLIC_SOURCECFG_D )
+        {
+            rc = -EOPNOTSUPP;
+
+            dprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
+
+            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(d, regoffset_to_word_idx(offset - APLIC_DOMAINCFG)) )
+            /* Interrupt not enabled, ignore it */
+            return 0;
+
+        if ( value > APLIC_SOURCECFG_SM_LEVEL_LOW )
+        {
+            gdprintk(XENLOG_ERR,
+                     "value(%u) is incorrect for sourcecfg register\n", value);
+
+            return 0;
+        }
+
+        break;
+
+    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
+    {
+        struct vcpu *target_vcpu = NULL;
+        unsigned int hart_idx = value >> APLIC_TARGET_HART_IDX_SHIFT;
+
+        /*
+         * Look at vaplic_emulate_load() for explanation why
+         * APLIC_GENMSI is subtracted.
+         */
+        if ( !AUTH_IRQ_BIT(d, regoffset_to_word_idx(offset - APLIC_GENMSI)) )
+            /* Interrupt not enabled, ignore it */
+            return 0;
+
+        if ( hart_idx < v->domain->max_vcpus )
+            target_vcpu = v->domain->vcpu[hart_idx];
+
+        if ( !target_vcpu )
+        {
+            dprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
+
+            /* Ignore such writings */
+            return 0;
+        }
+
+        value = aplic_msi_target_gen(target_vcpu, value);
+
+        break;
+    }
+
+    case APLIC_SETIPNUM:
+    case APLIC_SETIPNUM_LE:
+    case APLIC_CLRIPNUM:
+    case APLIC_SETIENUM:
+    case APLIC_CLRIENUM:
+        if ( !value || !AUTH_IRQ_BIT(d, value) )
+            return 0;
+
+        break;
+
+    case APLIC_DOMAINCFG:
+    {
+        struct vaplic *vaplic = to_vaplic(v->domain);
+
+        /*
+         * The domaincfg register has this format:
+         * bits 31:24 read-only 0x80
+         * bit 8      IE
+         * bit 7      read-only 0
+         * bit 2      DM (WARL)
+         * bit 0      BE (WARL)
+         *
+         * The most interesting bit for us is IE(Interrupt Enable) bit.
+         * At the moment, at least, Linux doesn't use domaincfg.IE bit to
+         * disable interrupts globally, but if one day someone will use it
+         * then extra actions should be done.
+         *
+         * Only DM (bit 2) and IE (bit 8) are writable here. They are assigned
+         * (not OR-ed) so that a write of 0 can also clear them (WARL), and the
+         * read-only high byte (0x80) is always kept set on read-back.
+         */
+        if ( value & ~(APLIC_DOMAINCFG_RO | APLIC_DOMAINCFG_DM |
+                       APLIC_DOMAINCFG_IE) )
+            printk_once("%s: Ignore writes to non-writable domaincfg bits as "
+                        "they are set by aplic during initialization in Xen\n",
+                        __func__);
+
+        vaplic->regs.domaincfg = APLIC_DOMAINCFG_RO |
+                                 (value & (APLIC_DOMAINCFG_DM |
+                                           APLIC_DOMAINCFG_IE));
+
+        return 0;
+    }
+
+    default:
+        goto fail;
+    }
+
+    aplic_hw_write_reg(offset, value);
+
+    return 0;
+
+ fail:
+    gdprintk(XENLOG_WARNING,
+             "Unhandled APLIC write at offset %#x (value %#x)\n", offset,
+             value);
+
+    return rc;
+}
+
 static int cf_check vaplic_init(struct vcpu *v)
 {
     return vcpu_imsic_init(v);
@@ -105,6 +356,50 @@ 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,
+                                               register_t *r)
+{
+    uint32_t data = 0;
+
+    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, &data) < 0 )
+        return IO_ABORT;
+
+    *r = data;
+    return IO_HANDLED;
+}
+
+static enum io_state cf_check vaplic_mmio_write(struct vcpu *v, mmio_info_t *info,
+                                                register_t r)
+{
+    if ( info->len != sizeof(uint32_t) ||
+         !IS_ALIGNED(info->gpa, sizeof(uint32_t)) )
+    {
+        gdprintk(XENLOG_DEBUG,
+                 "VAPLIC: unaligned/wrong-width write gpa=%"PRIpaddr" len=%u\n",
+                 info->gpa, info->len);
+        return IO_ABORT;
+    }
+
+    if ( vaplic_emulate_store(v, info->gpa, r) < 0 )
+        return IO_ABORT;
+
+    return IO_HANDLED;
+}
+
+static const struct mmio_handler_ops vaplic_mmio_ops = {
+    .read  = vaplic_mmio_read,
+    .write = vaplic_mmio_write,
+};
+
 static const struct vintc_ops vintc_ops = {
     .vcpu_init = vaplic_init,
     .vcpu_deinit = vaplic_deinit,
@@ -132,6 +427,12 @@ int domain_vaplic_init(struct domain *d)
      */
     d->arch.vintc->nr_virqs = guest_aplic_num_sources + 1;
 
+    vaplic->regs_start = GUEST_APLIC_S_BASE;
+    vaplic->regs_size = APLIC_SIZE(d->max_vcpus);
+
+    register_mmio_handler(d, &vaplic_mmio_ops,
+                          vaplic->regs_start, vaplic->regs_size);
+
     return 0;
 }
 
-- 
2.54.0

[PATCH v1 06/17] xen/riscv: map IMSIC interrupt file for vCPUs

[Oleksii Kurochko]

A guest running in VS-mode expects its own IMSIC S-file at offset 0 of its
guest-physical IMSIC block. Physically, the guest-file (G-file) assigned to
this vCPU lives at a hart-relative offset given by guest_file_id (assigned
via the vGEIN allocator). Therefore, imsic_map_guest_file() uses stage-2
translation to redirect the guest's fixed per-vCPU GPA page (offset 0) to
the specific physical guest-file page.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
The corresponding unmap of the IMSIC interrupt file will be introduced
separately when the need arises.
---
---
 xen/arch/riscv/imsic.c             | 63 ++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/imsic.h |  2 +
 2 files changed, 65 insertions(+)

diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index ffce77209c26..c5ae74e456e8 100644
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -25,7 +25,9 @@
 #include <xen/spinlock.h>
 #include <xen/xvmalloc.h>
 
+#include <asm/aia.h>
 #include <asm/imsic.h>
+#include <asm/p2m.h>
 
 #define IMSIC_HART_SIZE(guest_bits) (BIT(guest_bits, U) * IMSIC_MMIO_PAGE_SZ)
 
@@ -342,6 +344,67 @@ static int __init imsic_parse_node(const struct dt_device_node *node,
     return 0;
 }
 
+/*
+ * Map the physical IMSIC guest interrupt file (G-file) assigned to vCPU v
+ * into the domain's stage-2 guest-physical address space.
+ *
+ * In the machine's physical address space (SPA), each hart's IMSIC
+ * supervisor-level file (S-file) is located at offset 0 of its address block,
+ * followed contiguously by GEILEN guest files at offsets of 1, 2, ..., N pages.
+ *
+ * Because a guest OS running in VS-mode expects its own supervisor-level
+ * interrupt file to be at offset 0 of its guest-physical IMSIC block, the
+ * hypervisor must use stage-2 address translation to map the vCPU's
+ * guest-physical "supervisor" page (GPA offset 0) to the specific
+ * physical guest file page (SPA offset guest_file_id) on the physical hart.
+ *
+ * Xen pins each vCPU to a pCPU (v->processor) and assigns it a physical
+ * guest file index (guest_file_id) from the vGEIN allocator. A guest_file_id
+ * of 0 indicates that no hardware guest file is selected (matching the
+ * architectural behavior where vGEIN = 0 in the hstatus CSR selects no
+ * guest external interrupt source), requiring the VS-file to be emulated
+ * in software.
+ *
+ * The base guest-physical address advertised to the guest in the device
+ * tree matches offset 0 of the vCPU's virtual IMSIC block. Stage-2
+ * translation ensures that guest supervisor accesses to this page are
+ * transparently routed to the real hardware VS-file granted to it on
+ * the current pCPU.
+ */
+int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
+{
+    int res = 0;
+    struct domain *d = v->domain;
+    unsigned int cpu = v->processor;
+    vaddr_t gaddr = imsic_cfg.base_addr + (IMSIC_MMIO_PAGE_SZ * v->vcpu_id);
+    paddr_t paddr;
+    unsigned long guest_stride;
+
+    /* Nothing to map in the case of sw interrupt file. */
+    if ( !vsfile_id )
+        return res;
+
+    guest_stride = vsfile_id * IMSIC_MMIO_PAGE_SZ;
+
+    paddr = imsic_cfg.msi[cpu].base_addr + imsic_cfg.msi[cpu].offset +
+            guest_stride;
+
+#ifdef IMSIC_DEBUG
+    printk("%s: %pv: ga(%#lx) -> pa(%#lx), cpu(%#x), guest_file_id(%d) "
+           "base_addr(%#lx) offset(%#lx)\n", __func__, v, gaddr, paddr, cpu,
+           vsfile_id, imsic_cfg.msi[cpu].base_addr, imsic_cfg.msi[cpu].offset);
+#endif
+
+    res = map_regions_p2mt(d, gaddr_to_gfn(gaddr),
+                           PFN_DOWN(IMSIC_MMIO_PAGE_SZ), maddr_to_mfn(paddr),
+                           arch_dt_passthrough_p2m_type());
+    if ( res )
+        printk("%s: Failed to map %#lx to the guest at %#lx\n",
+               __func__, paddr, gaddr);
+
+    return res;
+}
+
 int vcpu_imsic_init(struct vcpu *v)
 {
     struct vimsic_state *imsic_state;
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index 612f503b5799..f2c649517fd1 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -106,4 +106,6 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v);
 
 int vimsic_make_domu_dt_node(struct kernel_info *kinfo, unsigned int *phandle);
 
+int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id);
+
 #endif /* ASM_RISCV_IMSIC_H */
-- 
2.54.0

[PATCH v1 07/17] xen/riscv: introduce vCPU AIA initialization

[Oleksii Kurochko]

Introduce vcpu_aia_init() to initialize the AIA-related state needed
for a vCPU to have a working guest interrupt file.

A guest (VS) interrupt file must be mapped to one of a pCPU's
hardware interrupt files (if they exist), so the pCPU a vCPU will actually
run on needs to be known first. arch_vcpu_create() is therefore not a
suitable place to call vcpu_aia_init(), since the pCPU assigned to a
vCPU can still change before it is first scheduled. To avoid
reassigning the VS interrupt file id and remapping it to a different
pCPU's hardware interrupt file, vcpu_aia_init() will instead be
called from a later point in the scheduling path (e.g.
continue_to_new_vcpu()), to be introduced in a follow-up patch. Since
it will end up being called from a non-__init context, it is not
itself marked __init.

Introduce imsic_update_state() to update a vCPU's guest IMSIC state
(the guest interrupt file id and the pCPU whose hardware interrupt
file it is mapped to) as a single consistent unit. This state can be
read concurrently, e.g. by a future helper that checks whether a
vCPU has a pending IMSIC interrupt, though no such consumer exists
yet at this stage - so it is protected by a lock.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/aia.c               | 30 ++++++++++++++++++++++++++++++
 xen/arch/riscv/imsic.c             | 13 +++++++++++++
 xen/arch/riscv/include/asm/aia.h   |  2 ++
 xen/arch/riscv/include/asm/imsic.h |  2 ++
 4 files changed, 47 insertions(+)

diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
index 4f7f46f58f0b..ed19600d4644 100644
--- a/xen/arch/riscv/aia.c
+++ b/xen/arch/riscv/aia.c
@@ -14,6 +14,7 @@
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
 #include <asm/current.h>
+#include <asm/imsic.h>
 
 struct vgein_ctrl {
     unsigned long bmp;
@@ -36,6 +37,35 @@ bool aia_usable(void)
     return _aia_usable;
 }
 
+void vcpu_aia_init(struct vcpu *v)
+{
+    unsigned int new_vsfile_id;
+    int rc;
+
+    if ( !aia_usable() )
+        return;
+
+    new_vsfile_id = vgein_assign(v);
+
+    /*
+     * vgein_assign() returns 0 when no free h/w guest interrupt file is
+     * available (including GEILEN == 0); imsic_map_guest_file() maps nothing
+     * in that case.
+     */
+    rc = imsic_map_guest_file(v, new_vsfile_id);
+    if ( rc )
+    {
+        /* Can't continue w/o correctly mapped IMSIC interrupt file */
+        domain_crash(v->domain);
+        return;
+    }
+
+    vcpu_guest_cpu_user_regs(v)->hstatus |=
+        MASK_INSR(new_vsfile_id, HSTATUS_VGEIN);
+
+    imsic_update_state(v, new_vsfile_id);
+}
+
 static int vgein_init(unsigned int cpu)
 {
     struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index c5ae74e456e8..2a792e756c4e 100644
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -83,6 +83,19 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v)
     return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
 }
 
+void imsic_update_state(struct vcpu *v, unsigned int guest_file_id)
+{
+    unsigned long flags;
+    struct vimsic_state *vimsic_state = v->arch.vimsic_state;
+    unsigned long pcpu = ( !guest_file_id ) ?
+                         NR_CPUS : cpuid_to_hartid(v->processor);
+
+    write_lock_irqsave(&vimsic_state->vsfile_lock, flags);
+    vimsic_state->guest_file_id = guest_file_id;
+    vimsic_state->vsfile_pcpu = pcpu;
+    write_unlock_irqrestore(&vimsic_state->vsfile_lock, flags);
+}
+
 void __init imsic_ids_local_delivery(bool enable)
 {
     if ( enable )
diff --git a/xen/arch/riscv/include/asm/aia.h b/xen/arch/riscv/include/asm/aia.h
index c67be0069a1d..2aef24cad4c0 100644
--- a/xen/arch/riscv/include/asm/aia.h
+++ b/xen/arch/riscv/include/asm/aia.h
@@ -15,4 +15,6 @@ void aia_init(void);
 unsigned int vgein_assign(struct vcpu *v);
 void vgein_release(struct vcpu *v, unsigned int vgein_id);
 
+void vcpu_aia_init(struct vcpu *v);
+
 #endif /* RISCV_AIA_H */
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index f2c649517fd1..8e3797690f99 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -104,6 +104,8 @@ int vcpu_imsic_init(struct vcpu *v);
 void vcpu_imsic_deinit(struct vcpu *v);
 unsigned int vcpu_guest_file_id(const struct vcpu *v);
 
+void imsic_update_state(struct vcpu *v, unsigned int guest_file_id);
+
 int vimsic_make_domu_dt_node(struct kernel_info *kinfo, unsigned int *phandle);
 
 int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id);
-- 
2.54.0

[PATCH v1 08/17] xen/riscv: add IMSIC state save/restore

[Oleksii Kurochko]

IMSIC state is currently needed only to track which physical CPU owns a
vCPU's IMSIC interrupt file. This is required because the physical CPU
ID is part of the physical address used to map the IMSIC file.

Add imsic_state_save() to record the current pCPU for a vCPU. When the
vCPU is migrated to a different pCPU, the mapping will need to be updated.

When imsic_state_restore() is called, VGEIN is already assigned to the
vCPU and the guest interrupt file is already mapped, and, as only h/w
interrupt files are used for now, nothing specific needs to be done.
Action is only required when the vCPU is moved to a different pCPU, which
requires recalculating VGEIN and the mapping for the new guest interrupt
file. That will be handled separately by vcpu_move_irqs(), which is
introduced in a follow-up patch; until then this case is guarded by a
BUG_ON(), which is fine.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/imsic.c             | 23 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/imsic.h |  3 +++
 2 files changed, 26 insertions(+)

diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index 2a792e756c4e..406bc68cbcd1 100644
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -20,6 +20,7 @@
 #include <xen/init.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/macros.h>
+#include <xen/rwlock.h>
 #include <xen/sched.h>
 #include <xen/smp.h>
 #include <xen/spinlock.h>
@@ -418,6 +419,28 @@ int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
     return res;
 }
 
+void imsic_state_save(struct vcpu *v)
+{
+    struct vimsic_state *imsic_state = v->arch.vimsic_state;
+    unsigned long flags;
+
+    /*
+     * SW interrupt file always has ->vsfile_pcpu = NR_CPUS so nothing specific
+     * should be done in this case.
+     */
+    if ( !vcpu_guest_file_id(v) )
+        return;
+
+    write_lock_irqsave(&imsic_state->vsfile_lock, flags);
+    imsic_state->vsfile_pcpu = cpuid_to_hartid(v->processor);
+    write_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
+}
+
+void imsic_state_restore(struct vcpu *v)
+{
+    /* Nothing to do */
+}
+
 int vcpu_imsic_init(struct vcpu *v)
 {
     struct vimsic_state *imsic_state;
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index 8e3797690f99..cfb968204a08 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -110,4 +110,7 @@ int vimsic_make_domu_dt_node(struct kernel_info *kinfo, unsigned int *phandle);
 
 int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id);
 
+void imsic_state_save(struct vcpu *v);
+void imsic_state_restore(struct vcpu *v);
+
 #endif /* ASM_RISCV_IMSIC_H */
-- 
2.54.0

[PATCH v1 09/17] xen/riscv: add helper to check APLIC MSI mode

[Oleksii Kurochko]

This helper can be used outside aplic.c to determine whether MSI mode
is enabled. A follow-up patch uses it to decide whether the guest
IMSIC state should be saved/restored.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/aplic.c             | 9 +++++++--
 xen/arch/riscv/include/asm/aplic.h | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index 87f2134bc561..1ce844cd2162 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -93,6 +93,11 @@ void aplic_hw_write_reg(unsigned int offset, uint32_t value)
     spin_unlock_irqrestore(&aplic.lock, flags);
 }
 
+bool has_msi_support(void)
+{
+    return readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM;
+}
+
 static void __init aplic_init_hw_interrupts(void)
 {
     unsigned int i;
@@ -185,7 +190,7 @@ static void cf_check aplic_irq_enable(struct irq_desc *desc)
      *       If APLIC without MSI interrupts is required in the future,
      *       this function will need to be updated accordingly.
      */
-    ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
+    ASSERT(has_msi_support());
 
     ASSERT(spin_is_locked(&desc->lock));
 
@@ -216,7 +221,7 @@ static void cf_check aplic_irq_disable(struct irq_desc *desc)
      *       If APLIC without MSI interrupts is required in the future,
      *       this function will need to be updated accordingly.
      */
-    ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
+    ASSERT(has_msi_support());
 
     ASSERT(spin_is_locked(&desc->lock));
 
diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
index 4ae5fb8f26d1..9a0b23351154 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -144,4 +144,6 @@ struct aplic_regs {
 uint32_t aplic_hw_read_reg(unsigned int offset, uint32_t mask);
 void aplic_hw_write_reg(unsigned int offset, uint32_t value);
 
+bool has_msi_support(void);
+
 #endif /* ASM_RISCV_APLIC_H */
-- 
2.54.0

[PATCH v1 10/17] xen/riscv: introduce vintc_state_{save,restore}()

[Oleksii Kurochko]

Virtual interrupt controller state must be preserved across vCPU context
switches: for AIA, a vCPU's guest interrupt file lives in the IMSIC of
the pCPU it runs on, so the related state has to be saved when the vCPU
is descheduled and re-established when it is scheduled again.

Introduce vintc_state_save()/vintc_state_restore() wrappers around new
store_state()/restore_state() hooks in struct vintc_ops, so that the
context switch path can save/restore this state without knowing which
vINTC variant a domain uses.

No callers are wired up yet: the vAPLIC implementation of the hooks is
added by the follow-up patch, and the calls from the context switch path
will be introduced together with vCPU context switch support.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/intc.h |  9 +++++++++
 xen/arch/riscv/intc.c             | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 2ee5d1533c8e..f4f0ce365c00 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -64,6 +64,12 @@ struct vintc_ops {
 
     /* Deinitialize some vINTC-related stuff for a vCPU */
     void (*vcpu_deinit)(struct vcpu *v);
+
+    /* Store virtual interrupt controller state */
+    void (*store_state)(struct vcpu *v);
+
+    /* Restore virtual interrupt controller state */
+    void (*restore_state)(struct vcpu *v);
 };
 
 struct vintc {
@@ -91,4 +97,7 @@ void domain_vintc_deinit(struct domain *d);
 
 bool vintc_reserve_virq(const struct domain *d, unsigned int virq);
 
+void vintc_state_save(struct vcpu *vcpu);
+void vintc_state_restore(struct vcpu *vcpu);
+
 #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 372c8d3a20f9..879d51337491 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -163,3 +163,17 @@ bool vintc_reserve_virq(const struct domain *d, unsigned int virq)
 
     return !test_and_set_bit(virq, d->arch.vintc->used_irqs);
 }
+
+void vintc_state_save(struct vcpu *vcpu)
+{
+    const struct vintc_ops *ops = vcpu->domain->arch.vintc->ops;
+
+    ops->store_state(vcpu);
+}
+
+void vintc_state_restore(struct vcpu *vcpu)
+{
+    const struct vintc_ops *ops = vcpu->domain->arch.vintc->ops;
+
+    ops->restore_state(vcpu);
+}
-- 
2.54.0

[PATCH v1 11/17] xen/riscv: add vAPLIC state save/restore hooks

[Oleksii Kurochko]

vAPLIC state needs to be saved and restored as part of vCPU context
management.

Introduce vaplic_state_save() and vaplic_state_restore() and wire them
to the IMSIC state save/restore helpers when MSI is available. The
functions are currently no-ops on platforms without MSI support and
will lead to BUG_ON() to not miss add support of no-MSI case.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/vaplic.h |  3 +++
 xen/arch/riscv/vaplic.c             | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/xen/arch/riscv/include/asm/vaplic.h b/xen/arch/riscv/include/asm/vaplic.h
index 7bf9247f4eae..fbd224b9a34a 100644
--- a/xen/arch/riscv/include/asm/vaplic.h
+++ b/xen/arch/riscv/include/asm/vaplic.h
@@ -34,4 +34,7 @@ struct vaplic {
 int domain_vaplic_init(struct domain *d);
 void domain_vaplic_deinit(struct domain *d);
 
+void vaplic_state_save(struct vcpu *v);
+void vaplic_state_restore(struct vcpu *v);
+
 #endif /* ASM__RISCV__VAPLIC_H */
diff --git a/xen/arch/riscv/vaplic.c b/xen/arch/riscv/vaplic.c
index 03240730e344..0723aad9558e 100644
--- a/xen/arch/riscv/vaplic.c
+++ b/xen/arch/riscv/vaplic.c
@@ -400,9 +400,27 @@ static const struct mmio_handler_ops vaplic_mmio_ops = {
     .write = vaplic_mmio_write,
 };
 
+void vaplic_state_save(struct vcpu *v)
+{
+    if ( has_msi_support() )
+        imsic_state_save(v);
+    else
+        BUG_ON("unimplemented");
+}
+
+void vaplic_state_restore(struct vcpu *v)
+{
+    if ( has_msi_support() )
+        imsic_state_restore(v);
+    else
+        BUG_ON("unimplemented");
+}
+
 static const struct vintc_ops vintc_ops = {
     .vcpu_init = vaplic_init,
     .vcpu_deinit = vaplic_deinit,
+    .store_state = vaplic_state_save,
+    .restore_state = vaplic_state_restore,
 };
 
 int domain_vaplic_init(struct domain *d)
-- 
2.54.0

[PATCH v1 12/17] xen/riscv: extend exception tables with type and data fields

[Oleksii Kurochko]

Extend the RISC-V exception table format to include a type and
auxiliary data field.

The existing format only supports simple fixups. Some use cases require
additional context from the fault (e.g. capturing trap information),
which cannot be expressed with the current EX_TYPE_FIXUP entries.

Introduce a generic ASM_EXTABLE_RAW() helper to describe entries with a
handler type and associated data. Reimplement ASM_EXTABLE() in terms of
it using EX_TYPE_FIXUP for compatibility.

Add EX_TYPE_TRAP_INFO to allow handlers to retrieve trap state
(sepc/scause/stval) and pass it to the fixup path. The data field is
used to encode which GPR contains a pointer to a struct trap_info.

Provide ASM_EXTABLE_TRAP_INFO() as a convenience wrapper for this case.

Also add gpr-num.h, providing symbolic GPR numbers for use in assembly
and inline asm. This is derived from Linux 6.16 with minor adjustments such
as using .irp instead of open-coding the same using a set of .equ.

Update the exception handling code to dispatch based on the entry type.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/extable.c               | 60 ++++++++++++++++++++++++-
 xen/arch/riscv/include/asm/extable.h   | 62 ++++++++++++++++++--------
 xen/arch/riscv/include/asm/gpr-num.h   | 33 ++++++++++++++
 xen/arch/riscv/include/asm/processor.h |  2 +
 xen/arch/riscv/include/asm/traps.h     |  6 +++
 5 files changed, 143 insertions(+), 20 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/gpr-num.h

diff --git a/xen/arch/riscv/extable.c b/xen/arch/riscv/extable.c
index 77e5e9e89439..e0ced0537182 100644
--- a/xen/arch/riscv/extable.c
+++ b/xen/arch/riscv/extable.c
@@ -7,8 +7,10 @@
 #include <xen/sort.h>
 #include <xen/virtual_region.h>
 
+#include <asm/csr.h>
 #include <asm/extable.h>
 #include <asm/processor.h>
+#include <asm/traps.h>
 
 #define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field)
 
@@ -33,6 +35,12 @@ static void __init cf_check swap_ex(void *a, void *b)
 
     x->fixup = y->fixup + delta;
     y->fixup = tmp.fixup - delta;
+
+    x->type = y->type;
+    y->type = tmp.type;
+
+    x->data = y->data;
+    y->data = tmp.data;
 }
 
 static int cf_check cmp_ex(const void *a, const void *b)
@@ -60,6 +68,40 @@ static void ex_handler_fixup(const struct exception_table_entry *ex,
     regs->sepc = ex_fixup(ex);
 }
 
+static inline unsigned long regs_get_gpr(struct cpu_user_regs *regs,
+                                         unsigned int offset)
+{
+    /*
+     * The GPR number -> offset arithmetic below relies on x0..x31 being
+     * laid out at the start of struct cpu_user_regs in architectural
+     * order.
+     */
+    BUILD_BUG_ON(offsetof(struct cpu_user_regs, ra) !=
+                 sizeof(unsigned long));
+    BUILD_BUG_ON(offsetof(struct cpu_user_regs, t6) !=
+                 31 * sizeof(unsigned long));
+
+    if ( unlikely(!offset || (offset > MAX_REG_OFFSET)) )
+        return 0;
+
+    return *(unsigned long *)((unsigned long)regs + offset);
+}
+
+static void ex_handler_trap_info(const struct exception_table_entry *ex,
+                                 struct cpu_user_regs *regs)
+{
+    struct trap_info *trap_info =
+        (struct trap_info *)regs_get_gpr(regs, ex->data * sizeof(unsigned long));
+
+    BUG_ON(!trap_info);
+
+    trap_info->sepc = csr_read(CSR_SEPC);
+    trap_info->scause = csr_read(CSR_SCAUSE);
+    trap_info->stval = csr_read(CSR_STVAL);
+
+    regs->sepc = ex_fixup(ex);
+}
+
 bool fixup_exception(struct cpu_user_regs *regs)
 {
     unsigned long pc = regs->sepc;
@@ -78,7 +120,23 @@ bool fixup_exception(struct cpu_user_regs *regs)
     if ( !ex )
         return false;
 
-    ex_handler_fixup(ex, regs);
+    switch ( ex->type )
+    {
+    case EX_TYPE_FIXUP:
+        ex_handler_fixup(ex, regs);
+        break;
+
+    case EX_TYPE_TRAP_INFO:
+        ex_handler_trap_info(ex, regs);
+        break;
+
+    default:
+        printk(XENLOG_ERR
+               "Unsupported exception table entry type %u for pc %#lx\n",
+               ex->type, pc);
+
+        return false;
+    }
 
     return true;
 }
diff --git a/xen/arch/riscv/include/asm/extable.h b/xen/arch/riscv/include/asm/extable.h
index c0128a91818f..287ce962b5cf 100644
--- a/xen/arch/riscv/include/asm/extable.h
+++ b/xen/arch/riscv/include/asm/extable.h
@@ -3,17 +3,24 @@
 #ifndef ASM__RISCV__ASM_EXTABLE_H
 #define ASM__RISCV__ASM_EXTABLE_H
 
+#include <asm/gpr-num.h>
+
+#define EX_TYPE_FIXUP 0
+#define EX_TYPE_TRAP_INFO 1
+
 #ifdef __ASSEMBLER__
 
-#define ASM_EXTABLE(insn, fixup) \
-    .pushsection .ex_table, "a"; \
-    .balign     4;               \
-    .word       (insn) - .;      \
-    .word       (fixup) - .;     \
-    .popsection
+#define ASM_EXTABLE_RAW(insn, fixup, type, data)    \
+    .pushsection .ex_table, "a";                    \
+    .balign     4;                                  \
+    .long       ((insn) - .);                       \
+    .long       ((fixup) - .);                      \
+    .short      (type);                             \
+    .short      (data);                             \
+    .popsection;
 
-.macro asm_extable, insn, fixup
-    ASM_EXTABLE(\insn, \fixup)
+.macro _asm_extable, insn, fixup
+    ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0)
 .endm
 
 #else /* __ASSEMBLER__ */
@@ -23,20 +30,36 @@
 
 struct cpu_user_regs;
 
-#define ASM_EXTABLE(insn, fixup)      \
-    ".pushsection .ex_table, \"a\"\n" \
-    ".balign    4\n"                  \
-    ".word      (" #insn " - .)\n"    \
-    ".word      (" #fixup " - .)\n"   \
+#define ASM_EXTABLE_RAW(insn, fixup, type, data)    \
+    ".pushsection .ex_table, \"a\"\n"               \
+    ".balign    4\n"                                \
+    ".long      ((" insn ") - .)\n"                 \
+    ".long      ((" fixup ") - .)\n"                \
+    ".short      (" type ")\n"                      \
+    ".short      (" data ")\n"                      \
     ".popsection\n"
 
+#define ASM_EXTABLE(insn, fixup)    \
+    ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_FIXUP), "0")
+
+#define EX_TRAP_INFO_REG(gpr)   \
+    "(.L_gpr_num_" #gpr ")"
+
+#define ASM_EXTABLE_TRAP_INFO(insn, fixup, data)                    \
+    DEFINE_ASM_GPR_NUMS                                             \
+    ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_TRAP_INFO),  \
+                    EX_TRAP_INFO_REG(data))
+
 /*
- * The exception table consists of pairs of relative offsets: the first
- * is the relative offset to an instruction that is allowed to fault,
- * and the second is the relative offset at which the program should
- * continue. No general-purpose registers are modified by the exception
- * handling mechanism itself, so it is up to the fixup code to handle
- * any necessary state cleanup.
+ * Each exception table entry consists of two relative offsets and a
+ * handler description: `insn` is the relative offset to an instruction
+ * that is allowed to fault, `fixup` is the relative offset at which the
+ * program should continue, `type` selects how the exception is handled
+ * (EX_TYPE_*), and `data` holds auxiliary information for the handler
+ * (e.g. for EX_TYPE_TRAP_INFO, the number of the GPR that contains a
+ * pointer to a struct trap_info). No general-purpose registers are
+ * modified by the exception handling mechanism itself, so it is up to
+ * the fixup code to handle any necessary state cleanup.
  *
  * The exception table and fixup code live out of line with the main
  * instruction path. This means when everything is well, we don't even
@@ -45,6 +68,7 @@ struct cpu_user_regs;
  */
 struct exception_table_entry {
     int32_t insn, fixup;
+    uint16_t type, data;
 };
 
 extern struct exception_table_entry __start___ex_table[];
diff --git a/xen/arch/riscv/include/asm/gpr-num.h b/xen/arch/riscv/include/asm/gpr-num.h
new file mode 100644
index 000000000000..1578f55cbd97
--- /dev/null
+++ b/xen/arch/riscv/include/asm/gpr-num.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef RISCV_GPR_NUM_H
+#define RISCV_GPR_NUM_H
+
+/* GPR ABI names, in register-number order (x0 .. x31). */
+#define GPR_ABI_NAMES                   \
+    zero, ra, sp, gp, tp, t0, t1, t2,   \
+    s0, s1, a0, a1, a2, a3, a4, a5,     \
+    a6, a7, s2, s3, s4, s5, s6, s7,     \
+    s8, s9, s10, s11, t3, t4, t5, t6
+
+#ifdef __ASSEMBLER__
+
+    .equ    .L_gpr_num, 0
+    .irp    name, GPR_ABI_NAMES
+    .equ    .L_gpr_num_\name, .L_gpr_num
+    .equ    .L_gpr_num, .L_gpr_num + 1
+    .endr
+
+#else /* __ASSEMBLER__ */
+
+#include <xen/stringify.h>
+
+#define DEFINE_ASM_GPR_NUMS                         \
+"   .equ    .L_gpr_num, 0\n"                        \
+"   .irp    name, " __stringify(GPR_ABI_NAMES) "\n" \
+"   .equ    .L_gpr_num_\\name, .L_gpr_num\n"        \
+"   .equ    .L_gpr_num, .L_gpr_num + 1\n"           \
+"   .endr\n"
+
+#endif /* __ASSEMBLER__ */
+
+#endif /* RISCV_GPR_NUM_H */
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index 6b89df4a2d4f..a5ccfa61bb9f 100644
--- a/xen/arch/riscv/include/asm/processor.h
+++ b/xen/arch/riscv/include/asm/processor.h
@@ -54,6 +54,8 @@ struct cpu_user_regs
     unsigned long pregs;
 };
 
+#define MAX_REG_OFFSET offsetof(struct cpu_user_regs, t6)
+
 /* TODO: need to implement */
 #define cpu_to_core(cpu)   0
 #define cpu_to_socket(cpu) 0
diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index 21fa3c3259b3..8d4ab664bca9 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -7,6 +7,12 @@
 
 #ifndef __ASSEMBLER__
 
+struct trap_info {
+    register_t sepc;
+    register_t scause;
+    register_t stval;
+};
+
 void do_trap(struct cpu_user_regs *cpu_regs);
 void handle_trap(void);
 void trap_init(void);
-- 
2.54.0

[PATCH v1 13/17] xen/riscv: add unprivileged guest memory read helper

[Oleksii Kurochko]

Introduce riscv_vcpu_unpriv_read() to allow Xen to safely read guest memory
using HLV/HLVX instructions while reliably capturing trap context.

This is required for instruction fetch emulation and MMIO decoding, where
Xen must inspect guest memory that may not be directly accessible and may
fault.

The implementation is based on kvm_riscv_vcpu_unpriv_read() from Linux,
with one deviation: the hlv/hlvx instructions translate the guest address
through the live vsatp/hgatp CSRs, i.e. through the address space of the
currently running vCPU, so the function can only be called safely for
current. Instead of taking a struct vcpu argument, it always operates on
current directly.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/guestcopy.c                | 91 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/guest_access.h |  6 ++
 2 files changed, 97 insertions(+)

diff --git a/xen/arch/riscv/guestcopy.c b/xen/arch/riscv/guestcopy.c
index 8a89212e0bea..57844bc442f6 100644
--- a/xen/arch/riscv/guestcopy.c
+++ b/xen/arch/riscv/guestcopy.c
@@ -6,6 +6,7 @@
 #include <xen/string.h>
 
 #include <asm/guest_access.h>
+#include <asm/traps.h>
 
 #define COPY_from_guest     0U
 #define COPY_to_guest       BIT(0, U)
@@ -114,3 +115,93 @@ unsigned long copy_to_guest_phys(struct domain *d, paddr_t gpa, void *buf,
     return copy_guest(buf, gpa, len, GPA_INFO(d),
                       COPY_to_guest | COPY_gpa);
 }
+
+/*
+ * Read machine word from Guest memory
+ *
+ * @read_insn: Flag representing whether we are reading instruction
+ * @guest_addr: Guest address to read
+ * @trap: Output pointer to trap details
+ *
+ * The hlv/hlvx instructions translate guest_addr through the live
+ * vsatp/hgatp CSRs, so the read is only meaningful for the address
+ * space of the currently running vCPU.
+ */
+unsigned long riscv_vcpu_unpriv_read(bool read_insn,
+                                     unsigned long guest_addr,
+                                     struct trap_info *trap)
+{
+    unsigned long val, tmp;
+    unsigned long flags, old_hstatus;
+
+    /*
+     * As hstatus is going to be changed we don't want an interrupt to occur
+     * with guest's hstatus register.
+     */
+    local_irq_save(flags);
+
+    /*
+     * The hypervisor virtual-machine load and store instructions are valid
+     * only in M-mode or HS-mode, or in U-mode when hstatus.HU=1. Each
+     * instruction performs an explicit memory access as though V=1; i.e.,
+     * with the address translation and protection, and the endianness,
+     * that apply to memory accesses in either VS-mode or VU-mode.
+     * Field SPVP of hstatus controls the privilege level of the access.
+     * The explicit memory access is done as though in VU-mode when SPVP=0,
+     * and as though in VS-mode when SPVP=1.
+     *
+     * So it is necessary to restore vCPU's hstatus before execution of
+     * hlv* instruction.
+     */
+    old_hstatus = csr_swap(CSR_HSTATUS,
+                           vcpu_guest_cpu_user_regs(current)->hstatus);
+
+    if ( read_insn )
+    {
+        asm volatile ( "\n"
+            "1:\n"
+            "   hlvx.hu %[val], (%[addr])\n"
+            ASM_EXTABLE_TRAP_INFO(1b, 3f, %[ti])
+            "   andi %[tmp], %[val], 3\n"
+            "   addi %[tmp], %[tmp], -3\n"
+            "   bne %[tmp], zero, 3f\n"
+            "   addi %[addr], %[addr], 2\n"
+            "\n"
+            "2:\n"
+            "   hlvx.hu %[tmp], (%[addr])\n"
+            ASM_EXTABLE_TRAP_INFO(2b, 3f, %[ti])
+            "   sll %[tmp], %[tmp], 16\n"
+            "   add %[val], %[val], %[tmp]\n"
+            "3:\n"
+        : [val] "=&r" (val), [tmp] "=&r" (tmp), [addr] "+&r" (guest_addr)
+        : [ti] "r" (trap) : "memory" );
+
+        /*
+         * Although HLVX instructions' explicit memory accesses require execute
+         * permissions, they still raise the same exceptions as other load
+         * instructions, rather than raising fetch exceptions instead.
+         */
+        if ( trap->scause == CAUSE_LOAD_PAGE_FAULT )
+            trap->scause = CAUSE_FETCH_PAGE_FAULT;
+    }
+    else
+    {
+        asm volatile ( "\n"
+            "1:\n"
+#ifdef CONFIG_RISCV_64
+            "hlv.d %[val], (%[addr])\n"
+#else
+            "hlv.w %[val], (%[addr])\n"
+#endif
+            "2:\n"
+            ASM_EXTABLE_TRAP_INFO(1b, 2b, %[ti])
+        : [val] "=&r" (val)
+        : [addr] "r" (guest_addr), [ti] "r" (trap) : "memory" );
+    }
+
+    csr_write(CSR_HSTATUS, old_hstatus);
+
+    local_irq_restore(flags);
+
+    return val;
+}
diff --git a/xen/arch/riscv/include/asm/guest_access.h b/xen/arch/riscv/include/asm/guest_access.h
index 8d679319ded0..153ec6ce26d1 100644
--- a/xen/arch/riscv/include/asm/guest_access.h
+++ b/xen/arch/riscv/include/asm/guest_access.h
@@ -5,6 +5,8 @@
 #include <xen/types.h>
 
 struct domain;
+struct trap_info;
+struct vcpu;
 
 unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len);
 unsigned long raw_copy_from_guest(void *to, const void *from, unsigned len);
@@ -25,6 +27,10 @@ unsigned long raw_clear_guest(void *to, unsigned int len);
 unsigned long copy_to_guest_phys(struct domain *d, paddr_t gpa, void *buf,
                                  unsigned long len);
 
+unsigned long riscv_vcpu_unpriv_read(bool read_insn,
+                                     unsigned long guest_addr,
+                                     struct trap_info *trap);
+
 #endif /* ASM__RISCV__GUEST_ACCESS_H */
 /*
  * Local variables:
-- 
2.54.0

[PATCH v1 14/17] xen/riscv: add guest page fault handling stub

[Oleksii Kurochko]

Add a stub handler for guest page faults and hook it into the trap path,
providing the basic infrastructure for future MMIO trap handling.

This will be used, for example, to trap accesses to APLIC registers in
order to initialize and emulate the interrupt controller for guests.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/traps.c | 66 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index d35c013e1399..1c97bd101948 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -191,6 +191,67 @@ static void timer_interrupt(void)
     raise_softirq(TIMER_SOFTIRQ);
 }
 
+static always_inline unsigned long get_faulting_gpa(void)
+{
+    /*
+     * According to RISC-V spec:
+     *  18.2.8. Hypervisor Trap Value Register (htval)
+     *   ...
+     *   A guest physical address written to htval is shifted right by 2 bits
+     *   to accommodate addresses wider than the current XLEN.
+     *   ...
+     *   If the least-significant two bits of a faulting guest physical address
+     *   are needed, these bits are ordinarily the same as the
+     *   least-significant two bits of the faulting virtual address in stval.
+     *   For faults due to implicit memory accesses for VS-stage address
+     *   translation, the least-significant two bits are instead zeros. These
+     *   cases can be distinguished using the value provided in register htinst.
+     */
+    return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
+}
+
+static int emulate_load(unsigned long fault_addr, unsigned long htinst)
+{
+    return -EOPNOTSUPP;
+}
+
+static int emulate_store(unsigned long fault_addr, unsigned long htinst)
+{
+    return -EOPNOTSUPP;
+}
+
+static void handle_guest_page_fault(unsigned long cause,
+                                    struct cpu_user_regs *regs)
+{
+    unsigned long addr;
+    int rc;
+
+    addr = get_faulting_gpa();
+
+    switch ( cause )
+    {
+    case CAUSE_LOAD_GUEST_PAGE_FAULT:
+        rc = emulate_load(addr, csr_read(CSR_HTINST));
+        break;
+
+    case CAUSE_STORE_GUEST_PAGE_FAULT:
+        rc = emulate_store(addr, csr_read(CSR_HTINST));
+        break;
+
+    default:
+        rc = -EOPNOTSUPP;
+        ASSERT_UNREACHABLE();
+        break;
+    }
+
+    if ( rc )
+        domain_crash(current->domain,
+                     "%s: unable to handle faulted guest %s addr %#lx\n",
+                     __func__,
+                     (cause == CAUSE_LOAD_GUEST_PAGE_FAULT) ? "load" : "store",
+                     addr);
+}
+
 void do_trap(struct cpu_user_regs *cpu_regs)
 {
     register_t pc = cpu_regs->sepc;
@@ -205,6 +266,11 @@ void do_trap(struct cpu_user_regs *cpu_regs)
         vsbi_handle_ecall(cpu_regs);
         break;
 
+    case CAUSE_LOAD_GUEST_PAGE_FAULT:
+    case CAUSE_STORE_GUEST_PAGE_FAULT:
+        handle_guest_page_fault(cause, cpu_regs);
+        break;
+
     case CAUSE_ILLEGAL_INSTRUCTION:
         if ( do_bug_frame(cpu_regs, pc) >= 0 )
         {
-- 
2.54.0

[PATCH v1 15/17] xen/riscv: implement trap redirection to a guest

[Oleksii Kurochko]

Some traps taken by Xen on behalf of a guest can't or shouldn't be
handled by the hypervisor and must be forwarded to the guest's own
S-mode exception handler instead: e.g. when riscv_vcpu_unpriv_read()
faults while accessing guest memory, or when emulation hits a condition
only the guest kernel can resolve.

Introduce riscv_vcpu_trap_redirect() for that purpose. It makes the
trap appear to the guest as if it had been taken directly in VS-mode:
the trap information is transferred to the guest's virtual supervisor
CSRs and the vCPU is resumed at its exception vector in supervisor
mode, following the trap entry rules of the RISC-V privileged
specification.

The implementation is based on kvm_riscv_vcpu_trap_redirect() from
Linux, with a few deviations:
 - The function reads and writes physical VS-mode CSRs, so it is only
   meaningful for the currently running vCPU. Instead of taking a
   struct vcpu argument, it always operates on current.
 - The MODE field of vstvec is masked off explicitly when computing the
   exception target PC (exceptions always vector to BASE), rather than
   relying on the hardwired zero bit of sepc to drop it on VM entry.
 - Assertions document the preconditions: the trap must have been taken
   from virtualized mode (hstatus.SPV set), and only synchronous
   exceptions may be redirected - interrupts must be injected via hvip
   instead, so that the hardware performs VS-mode trap entry itself,
   respecting vsstatus.SIE and vectored vstvec dispatch.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/guestcopy.c                | 54 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/guest_access.h |  2 +
 2 files changed, 56 insertions(+)

diff --git a/xen/arch/riscv/guestcopy.c b/xen/arch/riscv/guestcopy.c
index 57844bc442f6..6dc7a2e3a7ae 100644
--- a/xen/arch/riscv/guestcopy.c
+++ b/xen/arch/riscv/guestcopy.c
@@ -205,3 +205,57 @@ unsigned long riscv_vcpu_unpriv_read(bool read_insn,
 
     return val;
 }
+
+/* Redirect trap to Guest. */
+void riscv_vcpu_trap_redirect(const struct trap_info *trap)
+{
+    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
+    unsigned long vsstatus = csr_read(CSR_VSSTATUS);
+
+    /*
+     * Redirecting a trap makes sense only if the trap was taken from
+     * virtualized mode, i.e. sret is going to return to VS-mode.
+     */
+    ASSERT(regs->hstatus & HSTATUS_SPV);
+
+    /*
+     * Only synchronous exceptions can be redirected. Interrupts must be
+     * injected via hvip instead, so that the hardware itself performs
+     * VS-mode trap entry, respecting vsstatus.SIE and the vectored
+     * dispatch (BASE + 4 * cause) if vstvec is configured so.
+     */
+    ASSERT(!(trap->scause & CAUSE_IRQ_FLAG));
+
+    /* Change Guest SSTATUS.SPP bit */
+    vsstatus &= ~SSTATUS_SPP;
+    if ( regs->sstatus & SSTATUS_SPP )
+        vsstatus |= SSTATUS_SPP;
+
+    /* Change Guest SSTATUS.SPIE bit */
+    vsstatus &= ~SSTATUS_SPIE;
+    if ( vsstatus & SSTATUS_SIE )
+        vsstatus |= SSTATUS_SPIE;
+
+    /* Clear Guest SSTATUS.SIE bit */
+    vsstatus &= ~SSTATUS_SIE;
+
+    /* Update Guest SSTATUS */
+    csr_write(CSR_VSSTATUS, vsstatus);
+
+    /* Update Guest SCAUSE, STVAL, and SEPC */
+    csr_write(CSR_VSCAUSE, trap->scause);
+    csr_write(CSR_VSTVAL, trap->stval);
+    csr_write(CSR_VSEPC, trap->sepc);
+
+    /*
+     * Set Guest PC to Guest exception vector.
+     *
+     * vstvec[1:0] is the vector MODE, not part of the address. Exceptions
+     * always target BASE regardless of MODE, so mask it off explicitly
+     * instead of relying on the hardwired zero bit of sepc to drop it.
+     */
+    regs->sepc = csr_read(CSR_VSTVEC) & ~0x3UL;
+
+    /* Set Guest privilege mode to supervisor */
+    regs->sstatus |= SSTATUS_SPP;
+}
diff --git a/xen/arch/riscv/include/asm/guest_access.h b/xen/arch/riscv/include/asm/guest_access.h
index 153ec6ce26d1..d96cbc833d27 100644
--- a/xen/arch/riscv/include/asm/guest_access.h
+++ b/xen/arch/riscv/include/asm/guest_access.h
@@ -31,6 +31,8 @@ unsigned long riscv_vcpu_unpriv_read(bool read_insn,
                                      unsigned long guest_addr,
                                      struct trap_info *trap);
 
+void riscv_vcpu_trap_redirect(const struct trap_info *trap);
+
 #endif /* ASM__RISCV__GUEST_ACCESS_H */
 /*
  * Local variables:
-- 
2.54.0

Re: [PATCH v1 00/17] [RISC-V] virtual interrupt controller (vAPLIC/vIMSIC) support

[Jan Beulich]

On 20.07.2026 18:01, Oleksii Kurochko wrote:
> Hi all,
> 
> This series adds the initial virtual interrupt controller (vINTC) support
> for RISC-V guests in Xen, based on the Advanced Interrupt Architecture
> (AIA): a virtual APLIC (vAPLIC) in MSI mode backed by a virtual IMSIC
> (vIMSIC) using hardware guest interrupt files.
> 
> Rather than emulating APLIC in direct-delivery mode (which requires
> trap-and-emulate for every interrupt and is costly), the series targets
> IMSIC from the start. AIA lets a hart implement several "guest interrupt
> files" (up to GEILEN), so external interrupts can be delivered to a vCPU
> directly by hardware via the VGEIN field of hstatus, without a hypervisor
> round-trip. Xen only has to emulate the APLIC MMIO programming interface
> and route the guest's intent onto the physical MSI topology; interrupt
> delivery itself stays in hardware.
> 
> The work breaks down into a few logical blocks:
> 
> Physical APLIC/AIA groundwork (patches 1-3)
>   - Correctly track IRQ_DISABLED across runtime enable/disable and order
>     the ->status update against the IMSIC CSR write.
>   - Per-pCPU VGEIN (guest interrupt file) allocator: assign/release a
>     hardware guest file to a vCPU and program hstatus.VGEIN.
>   - Add the missing APLIC register offsets/masks needed by both the
>     physical and virtual APLIC code (no functional change).
> 
> Device-agnostic MMIO dispatch + vAPLIC emulation (patches 4-5)
>   - A per-domain MMIO handler table modelled on Arm's framework, so
>     emulated devices self-register their GPA ranges and the fault path
>     stays agnostic via a single try_handle_mmio() entry point.
>   - vAPLIC MMIO read/write emulation. Writes are gated by the domain's
>     authorised-IRQ bitmap so a guest cannot touch interrupts it does not
>     own, and TARGET writes are translated from virtual to physical
>     hart/guest-file indices. Delegation (SOURCECFG.D) is not yet
>     supported.
> 
> vIMSIC guest interrupt files and state (patches 6-9)
>   - Stage-2 map a vCPU's physical guest interrupt file to the fixed
>     per-vCPU GPA page the guest expects at offset 0.
>   - vcpu_aia_init(): assign a VGEIN, map its guest file and record the
>     IMSIC state as a consistent unit.
>   - IMSIC state save/restore (currently tracking which pCPU owns the
>     guest file, needed because the pCPU id is part of the MSI address).
>   - has_msi_support() helper to decide whether IMSIC state has to be
>     saved/restored.
> 
> vINTC state save/restore plumbing (patches 10-11)
>   - vintc_state_{save,restore}() wrappers over new store/restore hooks in
>     struct vintc_ops, and the vAPLIC implementation of those hooks. No
>     callers are wired up yet; the context-switch calls arrive with vCPU
>     context switch support.
> 
> Trap and instruction emulation infrastructure (patches 12-17)
>   - Extend the exception-table format with type/data fields and add
>     EX_TYPE_TRAP_INFO so fixups can capture sepc/scause/stval.
>   - riscv_vcpu_unpriv_read() (HLV/HLVX) to read guest memory/instructions
>     safely, and riscv_vcpu_trap_redirect() to forward a synchronous trap
>     back into the guest's VS-mode handler.
>   - A guest page-fault handler that decodes the trapped load/store
>     instruction (HTINST, or an unprivileged fetch as a fallback) and
>     dispatches the MMIO access through try_handle_mmio().
> 
> Note on versioning: the series is posted as v1 as a freshly split-out
> series, but several patches carry v2/v3 changelogs because they were
> previously circulated as part of a larger another patches [1] from which the
> current depends.
> 
> CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2690874319
> 
> [1] https://lore.kernel.org/xen-devel/cover.1784559209.git.oleksii.kurochko@gmail.com/T/#t
> 
> Oleksii Kurochko (17):
>   xen/riscv: manage IRQ_DISABLED flag in APLIC irq enable/disable
>     callbacks
>   xen/riscv: add basic VGEIN management for AIA guests
>   xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h
>   xen/riscv: introduce device-agnostic MMIO emulation dispatch
>   xen/riscv: implement virtual APLIC MMIO emulation
>   xen/riscv: map IMSIC interrupt file for vCPUs
>   xen/riscv: introduce vCPU AIA initialization
>   xen/riscv: add IMSIC state save/restore
>   xen/riscv: add helper to check APLIC MSI mode
>   xen/riscv: introduce vintc_state_{save,restore}()
>   xen/riscv: add vAPLIC state save/restore hooks
>   xen/riscv: extend exception tables with type and data fields
>   xen/riscv: add unprivileged guest memory read helper
>   xen/riscv: add guest page fault handling stub
>   xen/riscv: implement trap redirection to a guest
>   xen/riscv: add guest load emulation for trapped MMIO accesses
>   xen/riscv: add guest store emulation for trapped MMIO accesses

Where did the last two patches go? My inbox agrees with [1] that only 15 of the
17 patches arrived.

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2026-07/threads.html

Re: [PATCH v1 00/17] [RISC-V] virtual interrupt controller (vAPLIC/vIMSIC) support

[Oleksii Kurochko]

On 7/27/26 5:21 PM, Jan Beulich wrote:
> On 20.07.2026 18:01, Oleksii Kurochko wrote:
>> Hi all,
>>
>> This series adds the initial virtual interrupt controller (vINTC) support
>> for RISC-V guests in Xen, based on the Advanced Interrupt Architecture
>> (AIA): a virtual APLIC (vAPLIC) in MSI mode backed by a virtual IMSIC
>> (vIMSIC) using hardware guest interrupt files.
>>
>> Rather than emulating APLIC in direct-delivery mode (which requires
>> trap-and-emulate for every interrupt and is costly), the series targets
>> IMSIC from the start. AIA lets a hart implement several "guest interrupt
>> files" (up to GEILEN), so external interrupts can be delivered to a vCPU
>> directly by hardware via the VGEIN field of hstatus, without a hypervisor
>> round-trip. Xen only has to emulate the APLIC MMIO programming interface
>> and route the guest's intent onto the physical MSI topology; interrupt
>> delivery itself stays in hardware.
>>
>> The work breaks down into a few logical blocks:
>>
>> Physical APLIC/AIA groundwork (patches 1-3)
>>    - Correctly track IRQ_DISABLED across runtime enable/disable and order
>>      the ->status update against the IMSIC CSR write.
>>    - Per-pCPU VGEIN (guest interrupt file) allocator: assign/release a
>>      hardware guest file to a vCPU and program hstatus.VGEIN.
>>    - Add the missing APLIC register offsets/masks needed by both the
>>      physical and virtual APLIC code (no functional change).
>>
>> Device-agnostic MMIO dispatch + vAPLIC emulation (patches 4-5)
>>    - A per-domain MMIO handler table modelled on Arm's framework, so
>>      emulated devices self-register their GPA ranges and the fault path
>>      stays agnostic via a single try_handle_mmio() entry point.
>>    - vAPLIC MMIO read/write emulation. Writes are gated by the domain's
>>      authorised-IRQ bitmap so a guest cannot touch interrupts it does not
>>      own, and TARGET writes are translated from virtual to physical
>>      hart/guest-file indices. Delegation (SOURCECFG.D) is not yet
>>      supported.
>>
>> vIMSIC guest interrupt files and state (patches 6-9)
>>    - Stage-2 map a vCPU's physical guest interrupt file to the fixed
>>      per-vCPU GPA page the guest expects at offset 0.
>>    - vcpu_aia_init(): assign a VGEIN, map its guest file and record the
>>      IMSIC state as a consistent unit.
>>    - IMSIC state save/restore (currently tracking which pCPU owns the
>>      guest file, needed because the pCPU id is part of the MSI address).
>>    - has_msi_support() helper to decide whether IMSIC state has to be
>>      saved/restored.
>>
>> vINTC state save/restore plumbing (patches 10-11)
>>    - vintc_state_{save,restore}() wrappers over new store/restore hooks in
>>      struct vintc_ops, and the vAPLIC implementation of those hooks. No
>>      callers are wired up yet; the context-switch calls arrive with vCPU
>>      context switch support.
>>
>> Trap and instruction emulation infrastructure (patches 12-17)
>>    - Extend the exception-table format with type/data fields and add
>>      EX_TYPE_TRAP_INFO so fixups can capture sepc/scause/stval.
>>    - riscv_vcpu_unpriv_read() (HLV/HLVX) to read guest memory/instructions
>>      safely, and riscv_vcpu_trap_redirect() to forward a synchronous trap
>>      back into the guest's VS-mode handler.
>>    - A guest page-fault handler that decodes the trapped load/store
>>      instruction (HTINST, or an unprivileged fetch as a fallback) and
>>      dispatches the MMIO access through try_handle_mmio().
>>
>> Note on versioning: the series is posted as v1 as a freshly split-out
>> series, but several patches carry v2/v3 changelogs because they were
>> previously circulated as part of a larger another patches [1] from which the
>> current depends.
>>
>> CI tests: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2690874319
>>
>> [1] https://lore.kernel.org/xen-devel/cover.1784559209.git.oleksii.kurochko@gmail.com/T/#t
>>
>> Oleksii Kurochko (17):
>>    xen/riscv: manage IRQ_DISABLED flag in APLIC irq enable/disable
>>      callbacks
>>    xen/riscv: add basic VGEIN management for AIA guests
>>    xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h
>>    xen/riscv: introduce device-agnostic MMIO emulation dispatch
>>    xen/riscv: implement virtual APLIC MMIO emulation
>>    xen/riscv: map IMSIC interrupt file for vCPUs
>>    xen/riscv: introduce vCPU AIA initialization
>>    xen/riscv: add IMSIC state save/restore
>>    xen/riscv: add helper to check APLIC MSI mode
>>    xen/riscv: introduce vintc_state_{save,restore}()
>>    xen/riscv: add vAPLIC state save/restore hooks
>>    xen/riscv: extend exception tables with type and data fields
>>    xen/riscv: add unprivileged guest memory read helper
>>    xen/riscv: add guest page fault handling stub
>>    xen/riscv: implement trap redirection to a guest
>>    xen/riscv: add guest load emulation for trapped MMIO accesses
>>    xen/riscv: add guest store emulation for trapped MMIO accesses
> 
> Where did the last two patches go? My inbox agrees with [1] that only 15 of the
> 17 patches arrived.
> 
They lost for some reason, I sent them separately again. Now it should 
be okay.
> [1] https://lists.xen.org/archives/html/xen-devel/2026-07/threads.html

[PATCH v1 16/17] xen/riscv: add guest load emulation for trapped MMIO accesses

[Oleksii Kurochko]

Introduce emulate_load() to decode and emulate guest load instructions
that fault due to MMIO accesses. This provides the basic infrastructure
required for MMIO emulation on RISC-V.

The instruction decode (decode_trapped_insn() and the mask/match chain
for standard and compressed load encodings) is adapted from Linux's KVM
RISC-V implementation. The completion path differs from KVM's,
since Xen dispatches MMIO synchronously to an in-hypervisor handler via
try_handle_mmio() and has no userspace exit/return step equivalent to
KVM's kvm_io_bus_read() / KVM_EXIT_MMIO / kvm_riscv_vcpu_mmio_return()
split.

A fault taken while re-reading the trapped instruction is handled
depending on the faulting translation stage:
 - A VS-stage fault is the guest's own fault (e.g. it modified its page
   tables from another vCPU) and, as in KVM, is redirected to the
   guest's trap vector, with the cause remapped to
   CAUSE_FETCH_PAGE_FAULT since HLVX reports execute-permission failures
   as load faults.
 - A G-stage fault would mean the P2M mapping of the instruction page
   disappeared after the instruction was fetched. KVM must handle this
   by resuming the guest and retrying, as Linux MM can invalidate
   G-stage mappings at any time. Xen does not remove P2M mappings of a
   running domain at the moment, so this case is asserted unreachable with
   BUG_ON(); it will need to be revisited once such removal is implemented.

When a guest load triggers a page fault, the trapped instruction is
decoded using HTINST or, if unavailable, fetched via unprivileged access.
At the moment only virtual interrupt controller (vINTC) traps are
expected to occur, since it is currently the only backend registered
with the MMIO handler dispatch, so in practice the load is emulated via
the vINTC backend and the guest register state is updated accordingly.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/traps.h |   6 ++
 xen/arch/riscv/traps.c             | 162 ++++++++++++++++++++++++++++-
 2 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index 8d4ab664bca9..937295c5c76c 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -4,6 +4,7 @@
 #define ASM__RISCV__TRAPS_H
 
 #include <asm/processor.h>
+#include <asm/riscv_encoding.h>
 
 #ifndef __ASSEMBLER__
 
@@ -13,6 +14,11 @@ struct trap_info {
     register_t stval;
 };
 
+static inline bool is_load_guest_page_fault(unsigned long scause)
+{
+    return (scause == CAUSE_LOAD_GUEST_PAGE_FAULT);
+}
+
 void do_trap(struct cpu_user_regs *cpu_regs);
 void handle_trap(void);
 void trap_init(void);
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 1c97bd101948..12690ae37aba 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -14,7 +14,9 @@
 
 #include <asm/extable.h>
 #include <asm/cpufeature.h>
+#include <asm/guest_access.h>
 #include <asm/intc.h>
+#include <asm/mmio.h>
 #include <asm/processor.h>
 #include <asm/riscv_encoding.h>
 #include <asm/traps.h>
@@ -191,6 +193,11 @@ static void timer_interrupt(void)
     raise_softirq(TIMER_SOFTIRQ);
 }
 
+static always_inline void advance_pc(struct cpu_user_regs *regs, int step)
+{
+    regs->sepc += step;
+}
+
 static always_inline unsigned long get_faulting_gpa(void)
 {
     /*
@@ -210,9 +217,162 @@ static always_inline unsigned long get_faulting_gpa(void)
     return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
 }
 
+/*
+ * Determine the trapped instruction which caused a guest MMIO trap.
+ *
+ * Returns true if the trap was redirected to the guest, in which case
+ * the caller must stop emulation and return success. Otherwise *insn
+ * and *insn_len are filled in and the caller should continue decoding.
+ */
+static bool decode_trapped_insn(unsigned long htinst, unsigned long *insn,
+                                unsigned int *insn_len)
+{
+    if ( htinst & 0x1 )
+    {
+        /*
+         * Bit[0] == 1 implies trapped instruction value is
+         * transformed instruction or custom instruction.
+         */
+        *insn = htinst | INSN_16BIT_MASK;
+        *insn_len = (htinst & BIT(1, UL)) ? INSN_LEN(*insn) : 2;
+    }
+    else
+    {
+        struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
+        struct trap_info utrap = { 0 };
+
+        /*
+         * Bit[0] == 0 implies trapped instruction value is
+         * zero or special value.
+         */
+        *insn = riscv_vcpu_unpriv_read(true, regs->sepc, &utrap);
+        if ( utrap.scause )
+        {
+            /*
+             * A G-stage fault here would mean the P2M mapping of the page
+             * containing the trapped instruction disappeared after it was
+             * fetched. Nothing removes P2M mappings of a running domain yet,
+             * so this cannot happen.
+             *
+             * TODO: Revisit once P2M mappings can be removed at runtime.
+             */
+            BUG_ON(is_load_guest_page_fault(utrap.scause));
+
+            utrap.sepc = regs->sepc;
+            utrap.stval = utrap.sepc;
+
+            riscv_vcpu_trap_redirect(&utrap);
+
+            return true;
+        }
+
+        *insn_len = INSN_LEN(*insn);
+    }
+
+    return false;
+}
+
+/*
+ * Check alignment and dispatch a decoded MMIO access to a registered
+ * handler. On success (0), info->data holds the read value for loads.
+ */
+static int do_mmio(mmio_info_t *info, unsigned long fault_addr,
+                   unsigned int len)
+{
+    /* Fault address should be aligned to length of MMIO */
+    if ( fault_addr & (len - 1) )
+        return -EIO;
+
+    info->gpa = fault_addr;
+    info->len = len;
+
+    switch ( try_handle_mmio(info) )
+    {
+    case IO_HANDLED:
+        return 0;
+    case IO_ABORT:
+        return -EIO;
+    default:
+        return -EOPNOTSUPP;
+    }
+}
+
 static int emulate_load(unsigned long fault_addr, unsigned long htinst)
 {
-    return -EOPNOTSUPP;
+    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
+    mmio_info_t info = { .is_write = false };
+    unsigned long insn;
+    unsigned int shift = 0, len, insn_len;
+    bool is_unsigned = false;
+    int rc;
+
+    if ( decode_trapped_insn(htinst, &insn, &insn_len) )
+        return 0;
+
+    /* Decode length of MMIO and whether it is a sign- or zero-extending load */
+    if ( (insn & INSN_MASK_LB) == INSN_MATCH_LB )
+        len = 1;
+    else if ( (insn & INSN_MASK_LBU) == INSN_MATCH_LBU )
+    {
+        len = 1;
+        is_unsigned = true;
+    }
+    else if ( (insn & INSN_MASK_LH) == INSN_MATCH_LH )
+        len = 2;
+    else if ( (insn & INSN_MASK_LHU) == INSN_MATCH_LHU )
+    {
+        len = 2;
+        is_unsigned = true;
+    }
+    else if ( (insn & INSN_MASK_LW) == INSN_MATCH_LW )
+        len = 4;
+#ifndef CONFIG_RISCV_32
+    else if ( (insn & INSN_MASK_LWU) == INSN_MATCH_LWU )
+    {
+        len = 4;
+        is_unsigned = true;
+    }
+#endif
+    else if ( (insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW )
+    {
+        len = 4;
+        insn = RVC_RS2S(insn) << SH_RD;
+    }
+    else if ( (insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP &&
+              RV_X(insn, SH_RD, 5) )
+        len = 4;
+#ifndef CONFIG_RISCV_32
+    else if ( (insn & INSN_MASK_LD) == INSN_MATCH_LD )
+        len = 8;
+    else if ( (insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD )
+    {
+        len = 8;
+        insn = RVC_RS2S(insn) << SH_RD;
+    }
+    else if ( (insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
+              RV_X(insn, SH_RD, 5) )
+        len = 8;
+#endif
+    else
+        return -EOPNOTSUPP;
+
+    if ( !is_unsigned )
+        shift = BITS_PER_BYTE * (sizeof(unsigned long) - len);
+
+#ifdef EMULATE_LOAD_DEBUG
+    gdprintk(XENLOG_DEBUG, "pc=%#02lx, addr=%#02lx, len=%d, shift=%d\n",
+             regs->sepc, fault_addr, len, shift);
+#endif
+
+    rc = do_mmio(&info, fault_addr, len);
+    if ( rc )
+        return rc;
+
+    SET_RD(insn, regs, (long)((unsigned long)info.data << shift) >> shift);
+
+    advance_pc(regs, insn_len);
+
+    return 0;
 }
 
 static int emulate_store(unsigned long fault_addr, unsigned long htinst)
-- 
2.54.0

[PATCH v1 17/17] xen/riscv: add guest store emulation for trapped MMIO accesses

[Oleksii Kurochko]

Extend the guest page fault handler with store emulation to support MMIO
write accesses.

The instruction decode mirrors emulate_load() and, like it, is adapted
from Linux's KVM RISC-V implementation. As with the load path, the
completion is synchronous through try_handle_mmio() rather than KVM's
userspace exit/return split, since Xen's MMIO handlers run in the
hypervisor. Faults taken while re-reading the trapped instruction are
handled by decode_trapped_insn(), shared with the load path.

When a guest store instruction faults, the trapped instruction is decoded
using HTINST or, if unavailable, fetched via unprivileged access. At the
moment only virtual interrupt controller (vINTC) traps are expected to
occur, since it is currently the only backend registered with the MMIO
handler dispatch, so in practice the store is emulated via the vINTC
backend.

Together with load emulation, this completes the basic MMIO handling path
needed for virtual interrupt controller support on RISC-V.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/traps.c | 55 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 12690ae37aba..18156627d7cd 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -377,7 +377,60 @@ static int emulate_load(unsigned long fault_addr, unsigned long htinst)
 
 static int emulate_store(unsigned long fault_addr, unsigned long htinst)
 {
-    return -EOPNOTSUPP;
+    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
+    mmio_info_t info = { .is_write = true };
+    register_t data;
+    unsigned long insn;
+    unsigned int len, insn_len;
+    int rc;
+
+    if ( decode_trapped_insn(htinst, &insn, &insn_len) )
+        return 0;
+
+    data = GET_RS2(insn, regs);
+
+    if ( (insn & INSN_MASK_SB) == INSN_MATCH_SB )
+        len = 1;
+    else if ( (insn & INSN_MASK_SH) == INSN_MATCH_SH )
+        len = 2;
+    else if ( (insn & INSN_MASK_SW) == INSN_MATCH_SW )
+        len = 4;
+    else if ( (insn & INSN_MASK_C_SW) == INSN_MATCH_C_SW )
+    {
+        len = 4;
+        data = GET_RS2S(insn, regs);
+    }
+    else if ( (insn & INSN_MASK_C_SWSP) == INSN_MATCH_C_SWSP )
+    {
+        len = 4;
+        data = GET_RS2C(insn, regs);
+    }
+#ifndef CONFIG_RISCV_32
+    else if ( (insn & INSN_MASK_SD) == INSN_MATCH_SD )
+        len = 8;
+    else if ( (insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD )
+    {
+        len = 8;
+        data = GET_RS2S(insn, regs);
+    }
+    else if ( (insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP )
+    {
+        len = 8;
+        data = GET_RS2C(insn, regs);
+    }
+#endif
+    else
+        return -EOPNOTSUPP;
+
+    info.data = data;
+
+    rc = do_mmio(&info, fault_addr, len);
+    if ( rc )
+        return rc;
+
+    advance_pc(regs, insn_len);
+
+    return 0;
 }
 
 static void handle_guest_page_fault(unsigned long cause,
-- 
2.54.0