[PATCH v1 15/27] xen/riscv: add very early virtual APLIC (vAPLIC) initialization support

Oleksii Kurochko posted 27 patches 4 weeks ago
[PATCH v1 15/27] xen/riscv: add very early virtual APLIC (vAPLIC) initialization support
Posted by Oleksii Kurochko 4 weeks ago
At the current development stage, only domain vINTC init and deinit
operations are required, so implement those first.

Initialize vAPLIC's domaincfg to with the interrupt-enable bit set and
MSI delivery mode selected as the current solution is exepcted to have
always IMSIC, and initialize vintc->ops.

Other operations such as emulate_load(), emulate_store(), and is_access()
will be needed once guests are running and MMIO accesses to APLIC MMIO
range must be handled. These will be introduced separately later.

Introduce a structure to describe a virtual interrupt controller (vINTC)
and a vintc_ops structure, which provides operations to emulate load and
store accesses to interrupt controller MMIOs and to check whether a given
address falls within the MMIO range of a specific virtual interrupt
controller.

The vAPLIC implementation of these operations will be provided later
once guests can be run and these operations are actually needed.

Introduce these structures here as they are required for the implementation
of domain_vaplic_init() and domain_vaplic_alloc(). Also, introduce
vcpu_vaplic_init() and init vintc_ops->vcpu_init() with it.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile             |  1 +
 xen/arch/riscv/domain.c             | 11 ++---
 xen/arch/riscv/include/asm/domain.h |  2 +
 xen/arch/riscv/include/asm/intc.h   | 14 ++++++
 xen/arch/riscv/include/asm/vaplic.h | 36 ++++++++++++++
 xen/arch/riscv/intc.c               |  1 +
 xen/arch/riscv/vaplic.c             | 74 +++++++++++++++++++++++++++++
 7 files changed, 131 insertions(+), 8 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/vaplic.h
 create mode 100644 xen/arch/riscv/vaplic.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index d772b42386c0..b9941a230e03 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -24,6 +24,7 @@ obj-y += smpboot.o
 obj-y += stubs.o
 obj-y += time.o
 obj-y += traps.o
+obj-y += vaplic.o
 obj-y += vmid.o
 obj-y += vm_event.o
 obj-y += vsbi/
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 515735b32e30..560b21b16ffb 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -11,6 +11,7 @@
 #include <asm/bitops.h>
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
+#include <asm/intc.h>
 #include <asm/riscv_encoding.h>
 #include <asm/vtimer.h>
 
@@ -154,14 +155,8 @@ int arch_vcpu_create(struct vcpu *v)
     if ( (rc = vcpu_vtimer_init(v)) )
         goto fail;
 
-    /*
-     * As interrupt controller (IC) is not yet implemented,
-     * return an error.
-     *
-     * TODO: Drop this once IC is implemented.
-     */
-    rc = -EOPNOTSUPP;
-    goto fail;
+    if ( (rc = v->domain->arch.vintc->ops->vcpu_init(v)) )
+        goto fail;
 
     return rc;
 
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index bdb1ffd748c9..21a3e6876f36 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -98,6 +98,8 @@ struct arch_domain {
     struct paging_domain paging;
 #endif
 
+    struct vintc *vintc;
+
     /* Next unused device tree phandle number */
     uint32_t next_phandle;
 };
diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 8300d71d472f..c5a869db8bc5 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -16,6 +16,7 @@ struct cpu_user_regs;
 struct dt_device_node;
 struct irq_desc;
 struct kernel_info;
+struct vcpu;
 
 struct intc_info {
     enum intc_version hw_version;
@@ -47,6 +48,19 @@ struct intc_hw_operations {
                             const struct dt_device_node *intc);
 };
 
+struct vintc_ops {
+    /* Initialize some vINTC-related stuff for a vCPU */
+    int (*vcpu_init)(struct vcpu *vcpu);
+
+    /* Check if a register is virtual interrupt controller MMIO */
+    int (*is_access)(const struct vcpu *vcpu, const unsigned long addr);
+};
+
+struct vintc {
+    const struct intc_info *info;
+    const struct vintc_ops *ops;
+};
+
 void intc_preinit(void);
 
 void register_intc_ops(const struct intc_hw_operations *ops);
diff --git a/xen/arch/riscv/include/asm/vaplic.h b/xen/arch/riscv/include/asm/vaplic.h
new file mode 100644
index 000000000000..7684f3490829
--- /dev/null
+++ b/xen/arch/riscv/include/asm/vaplic.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * xen/arch/riscv/vaplic.c
+ *
+ * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
+ *
+ * Copyright (c) Microchip.
+ */
+
+#ifndef ASM__RISCV__VAPLIC_H
+#define ASM__RISCV__VAPLIC_H
+
+#include <xen/kernel.h>
+#include <xen/types.h>
+
+#include <asm/intc.h>
+
+struct domain;
+
+#define to_vaplic(v) container_of(v, struct vaplic, base)
+
+struct vaplic_regs {
+    uint32_t domaincfg;
+    uint32_t smsiaddrcfg;
+    uint32_t smsiaddrcfgh;
+};
+
+struct vaplic {
+    struct vintc base;
+    struct vaplic_regs regs;
+};
+
+int domain_vaplic_init(struct domain *d);
+void domain_vaplic_deinit(struct domain *d);
+
+#endif /* ASM__RISCV__VAPLIC_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index c9f12651fda1..ff7a76accaca 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -6,6 +6,7 @@
 #include <xen/init.h>
 #include <xen/irq.h>
 #include <xen/lib.h>
+#include <xen/sched.h>
 #include <xen/spinlock.h>
 
 #include <asm/aia.h>
diff --git a/xen/arch/riscv/vaplic.c b/xen/arch/riscv/vaplic.c
new file mode 100644
index 000000000000..9b105de7ed7d
--- /dev/null
+++ b/xen/arch/riscv/vaplic.c
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * xen/arch/riscv/vaplic.c
+ *
+ * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
+ *
+ * Copyright (c) Microchip.
+ * Copyright (c) Vates
+ */
+
+#include <xen/errno.h>
+#include <xen/sched.h>
+#include <xen/xvmalloc.h>
+
+#include <asm/aia.h>
+#include <asm/imsic.h>
+#include <asm/intc.h>
+#include <asm/vaplic.h>
+
+#include "aplic-priv.h"
+
+static int __init cf_check vcpu_vaplic_init(struct vcpu *v)
+{
+    int rc = 0;
+
+    rc = vcpu_imsic_init(v);
+    if ( rc )
+        return rc;
+
+    imsic_set_guest_file_id(v, vgein_assign(v));
+
+    return rc;
+}
+
+static const struct vintc_ops vaplic_ops = {
+    .vcpu_init = vcpu_vaplic_init,
+};
+
+static struct vintc * __init vaplic_alloc(void)
+{
+    struct vaplic *v = NULL;
+
+    v = xvzalloc(struct vaplic);
+    if ( !v )
+        return NULL;
+
+    return &v->base;
+}
+
+int __init domain_vaplic_init(struct domain *d)
+{
+    int ret = 0;
+
+    d->arch.vintc = vaplic_alloc();
+    if ( !d->arch.vintc )
+    {
+        ret = -ENOMEM;
+        goto fail;
+    }
+
+    d->arch.vintc->ops = &vaplic_ops;
+    to_vaplic(d->arch.vintc)->regs.domaincfg =
+        APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM;
+
+ fail:
+    return ret;
+}
+
+void __init domain_vaplic_deinit(struct domain *d)
+{
+    struct vaplic *vaplic = to_vaplic(d->arch.vintc);
+
+    XVFREE(vaplic);
+}
-- 
2.53.0
Re: [PATCH v1 15/27] xen/riscv: add very early virtual APLIC (vAPLIC) initialization support
Posted by Jan Beulich 5 days, 14 hours ago
On 10.03.2026 18:08, Oleksii Kurochko wrote:
> @@ -47,6 +48,19 @@ struct intc_hw_operations {
>                              const struct dt_device_node *intc);
>  };
>  
> +struct vintc_ops {
> +    /* Initialize some vINTC-related stuff for a vCPU */
> +    int (*vcpu_init)(struct vcpu *vcpu);

v as the parameter name, to fit our convention? (Same below for the other
hook.)

> +    /* Check if a register is virtual interrupt controller MMIO */
> +    int (*is_access)(const struct vcpu *vcpu, const unsigned long addr);

What does "register" in the comment refer to. All I see is an address.
(The const will also want dropping from the parameter in this declaration.)

> +};
> +
> +struct vintc {
> +    const struct intc_info *info;

Isn't this referencing a physical INTC's structure? Why would the virtual
one's properties have to match that of the physical one?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/vaplic.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * xen/arch/riscv/vaplic.c
> + *
> + * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + */
> +
> +#ifndef ASM__RISCV__VAPLIC_H
> +#define ASM__RISCV__VAPLIC_H
> +
> +#include <xen/kernel.h>
> +#include <xen/types.h>
> +
> +#include <asm/intc.h>
> +
> +struct domain;
> +
> +#define to_vaplic(v) container_of(v, struct vaplic, base)

I'm confused here, maybe first of all because of the use of v. v is our
common identified for struct vcpu * instances. Using it in a macro like
this one suggests a struct vcpu * needs passing into the macro. Yet from
the two uses of the macro that doesn't look to be the case.

Perhaps best to have a struct domain * passed into here?

> +struct vaplic_regs {
> +    uint32_t domaincfg;
> +    uint32_t smsiaddrcfg;
> +    uint32_t smsiaddrcfgh;

The latter two aren't used, and generally I'd expect a h-suffixed field to
exist only for RV32. (The un-suffixed field then would need to be unsigned
long, of course.)

> +};
> +
> +struct vaplic {
> +    struct vintc base;

How does "base" fit with the type of the field?

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -6,6 +6,7 @@
>  #include <xen/init.h>
>  #include <xen/irq.h>
>  #include <xen/lib.h>
> +#include <xen/sched.h>
>  #include <xen/spinlock.h>

Why is this change needed all of the sudden?

> --- /dev/null
> +++ b/xen/arch/riscv/vaplic.c
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * xen/arch/riscv/vaplic.c
> + *
> + * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <asm/aia.h>
> +#include <asm/imsic.h>
> +#include <asm/intc.h>
> +#include <asm/vaplic.h>
> +
> +#include "aplic-priv.h"
> +
> +static int __init cf_check vcpu_vaplic_init(struct vcpu *v)
> +{
> +    int rc = 0;
> +
> +    rc = vcpu_imsic_init(v);
> +    if ( rc )
> +        return rc;
> +
> +    imsic_set_guest_file_id(v, vgein_assign(v));

And vgein_assign() can't fail? (Rhetorical question - of course it can. That
function shouldn't assert that it can fine a valid ID.)

But then - aren't you limiting the number of vCPU-s a host can handle by the
number vgein IDs?

> +    return rc;
> +}
> +
> +static const struct vintc_ops vaplic_ops = {
> +    .vcpu_init = vcpu_vaplic_init,
> +};
> +
> +static struct vintc * __init vaplic_alloc(void)
> +{
> +    struct vaplic *v = NULL;

Onve again - why the initializer? In fact, ...

> +    v = xvzalloc(struct vaplic);

... this could be the initializer.

> +    if ( !v )
> +        return NULL;
> +
> +    return &v->base;
> +}

If you returned and ...

> +int __init domain_vaplic_init(struct domain *d)
> +{
> +    int ret = 0;
> +
> +    d->arch.vintc = vaplic_alloc();

... stored struct vaplic *, the slightly odd to_vaplic() macro wouldn't
be needed.

> +    if ( !d->arch.vintc )
> +    {
> +        ret = -ENOMEM;
> +        goto fail;

Nit: goto when simply return could be used.

> +    }
> +
> +    d->arch.vintc->ops = &vaplic_ops;

Are other kinds of ops structures going to appear? If not, why the extra
indirection?

> +    to_vaplic(d->arch.vintc)->regs.domaincfg =
> +        APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM;
> +
> + fail:
> +    return ret;
> +}
> +
> +void __init domain_vaplic_deinit(struct domain *d)
> +{
> +    struct vaplic *vaplic = to_vaplic(d->arch.vintc);
> +
> +    XVFREE(vaplic);

If this cleared the struct domain field, then yes. But the way it is, just
xvfree() will suffice. (Re-work following other remarks may want it to
become XVFREE() again, though.)

Jan