Re: [PATCH v2 14/26] xen/riscv: add very early virtual APLIC (vAPLIC) initialization support

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/4/26 1:29 PM, Oleksii Kurochko wrote:
> > > +
> > > +static const struct vintc_ops vintc_ops = {
> > > +    .vcpu_init = vcpu_vaplic_init,
> > > +};
> > > +
> > > +int __init domain_vaplic_init(struct domain *d)
> > Why __init, and why is there no caller? 
> The caller is in follow-up patch. I will add that to commit message.
>
> Considering that domain_vintc_init() isn't __init from where 
> domain_vaplic_init() is called then __init should be dropped here. I 
> will do that.
> Plus why is the vCPU-init a hook,
> > but the domain init is not? Either you mean to allow for other ICs, or
> > you you don't.
> IIUC your question domain_vaplic_init() ins't a hook because vaplic 
> structure is allocated dynamically so vintc, vintc->ops and/or vintc- 
>  >init_ops aren't initialized at the moment when vintc->{ops or 
> init_ops}->domain_vaplic_init() is used in domain_vintc_init() (which is 
> introduced in the follow up patch).
As an alternative it could be that vintc->init_ops are moved as a 
variable to int.c and then domain_vaplic_(de)init will be just a hook. 
Something like:
$ git diff
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index 86d0a247ef3d..4c4f777f7e1b 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -28,6 +28,7 @@
 #include <asm/imsic.h>
 #include <asm/intc.h>
 #include <asm/io.h>
+#include <asm/vaplic.h>
 #include <asm/riscv_encoding.h>

 #define APLIC_DEFAULT_PRIORITY  1
@@ -394,6 +395,7 @@ static int __init aplic_preinit(struct 
dt_device_node *node, const void *dat)
     dt_irq_xlate = aplic_irq_xlate;

     register_intc_ops(&aplic_init_ops);
+    register_vintc_init_ops(&vaplic_vintc_init_ops);

     /* Enable supervisor external interrupt */
     csr_set(CSR_SIE, BIT(IRQ_S_EXT, UL));
diff --git a/xen/arch/riscv/include/asm/intc.h 
b/xen/arch/riscv/include/asm/intc.h
index 4842ca08aa54..84b97d6c56f9 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -56,6 +56,8 @@ struct intc_hw_init_ops {
 };

 struct vintc_init_ops {
+    int (*init)(struct domain *d);
+    void (*deinit)(struct domain *d);
     /* Create interrupt controller node for domain */
     int (*make_domu_dt_node)(struct kernel_info *kinfo);
 };
@@ -79,13 +81,13 @@ struct vintc_ops {
 struct vintc {
     unsigned int irq_nums;
     unsigned long *allocated_irqs;
-    const struct vintc_init_ops *init_ops;
     const struct vintc_ops *ops;
 };

 void intc_preinit(void);

 void register_intc_ops(const struct intc_hw_init_ops *init_ops);
+void register_vintc_init_ops(const struct vintc_init_ops *ops);

 void intc_init(void);

diff --git a/xen/arch/riscv/include/asm/vaplic.h 
b/xen/arch/riscv/include/asm/vaplic.h
index 0fa690fcb2d7..7720e6556fcb 100644
--- a/xen/arch/riscv/include/asm/vaplic.h
+++ b/xen/arch/riscv/include/asm/vaplic.h
@@ -15,8 +15,6 @@

 #include <asm/intc.h>

-struct domain;
-
 #define to_vaplic(d) container_of(d->arch.vintc, struct vaplic, vintc)

 struct vaplic_regs {
@@ -31,7 +29,6 @@ struct vaplic {
     paddr_t regs_size;
 };

-int domain_vaplic_init(struct domain *d);
-void domain_vaplic_deinit(struct domain *d);
+extern const struct vintc_init_ops vaplic_vintc_init_ops;

 #endif /* ASM__RISCV__VAPLIC_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 0934bce3f2f8..304d4def7a4b 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -13,18 +13,24 @@

 #include <asm/aia.h>
 #include <asm/intc.h>
-#include <asm/vaplic.h>

 static const struct intc_hw_operations *__ro_after_init intc_hw_ops;

 static const struct intc_hw_init_ops *__initdata intc_hw_init_ops;

+static const struct vintc_init_ops *__ro_after_init vintc_init_ops;
+
 void __init register_intc_ops(const struct intc_hw_init_ops *init_ops)
 {
     intc_hw_ops = init_ops->ops;
     intc_hw_init_ops = init_ops;
 }

+void __init register_vintc_init_ops(const struct vintc_init_ops *ops)
+{
+    vintc_init_ops = ops;
+}
+
 void __init intc_preinit(void)
 {
     if ( acpi_disabled )
@@ -105,29 +111,18 @@ int intc_route_irq_to_guest(struct irq_desc *desc,

 int __init make_intc_domU_node(struct kernel_info *kinfo)
 {
-    struct vintc *vintc = kinfo->bd.d->arch.vintc;
-
-    ASSERT(vintc->init_ops && vintc->init_ops->make_domu_dt_node);

-    return vintc->init_ops->make_domu_dt_node(kinfo);
+    return vintc_init_ops->make_domu_dt_node(kinfo);
 }

 int domain_vintc_init(struct domain *d)
 {
-    int ret = -EOPNOTSUPP;
-    const enum intc_version ver = intc_hw_ops->info->hw_version;
-
-    switch ( ver )
-    {
-    case INTC_APLIC:
-        ret = domain_vaplic_init(d);
-        break;
+    int ret;

-    default:
-        printk("vintc (ver:%d) isn't implemented\n", ver);
-        break;
-    }

+    ret = vintc_init_ops->init(d);
     if ( !ret )
     {
         d->arch.vintc->allocated_irqs =
@@ -141,19 +136,9 @@ int domain_vintc_init(struct domain *d)

 void domain_vintc_deinit(struct domain *d)
 {
-    const enum intc_version ver = intc_hw_ops->info->hw_version;
-
-    switch ( ver )
-    {
-    case INTC_APLIC:
-        domain_vaplic_deinit(d);
-        break;
-
-    default:
-        printk("vintc (ver:%d) isn't implemented\n", ver);
-        break;
-    }
+    ASSERT(vintc_init_ops && vintc_init_ops->deinit);

+    vintc_init_ops->deinit(d);
     xvfree(d->arch.vintc->allocated_irqs);
 }

diff --git a/xen/arch/riscv/vaplic.c b/xen/arch/riscv/vaplic.c
index 57c3433ba03b..301078b8639e 100644
--- a/xen/arch/riscv/vaplic.c
+++ b/xen/arch/riscv/vaplic.c
@@ -397,7 +397,12 @@ static int __init cf_check 
vaplic_make_domu_dt_node(struct kernel_info *kinfo)
     return fdt_end_node(fdt);
 }

-static const struct vintc_init_ops __initdata init_ops = {
+static int domain_vaplic_init(struct domain *d);
+static void domain_vaplic_deinit(struct domain *d);
+
+const struct vintc_init_ops vaplic_vintc_init_ops = {
+    .init              = domain_vaplic_init,
+    .deinit            = domain_vaplic_deinit,
     .make_domu_dt_node = vaplic_make_domu_dt_node,
 };

@@ -408,7 +413,7 @@ static const struct vintc_ops vintc_ops = {
     .emulate_load = vaplic_emulate_load,
 };

-int __init domain_vaplic_init(struct domain *d)
+static int domain_vaplic_init(struct domain *d)
 {
     struct vaplic *vaplic = xvzalloc(struct vaplic);

@@ -417,7 +422,6 @@ int __init domain_vaplic_init(struct domain *d)

     d->arch.vintc = &vaplic->vintc;
     d->arch.vintc->ops = &vintc_ops;
-    d->arch.vintc->init_ops = &init_ops;

     vaplic->regs.domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM |
                              APLIC_DOMAINCFG_RO80;
@@ -428,7 +432,7 @@ int __init domain_vaplic_init(struct domain *d)
     return 0;
 }

-void __init domain_vaplic_deinit(struct domain *d)
+static void domain_vaplic_deinit(struct domain *d)
 {
     struct vaplic *vaplic = to_vaplic(d);


The downside is that we still need register_vintc_init_ops() which will 
called from real interrupt controller code.
Would it be better solution?

~ Oleksii