Re: [PATCH v2 16/26] xen/riscv: create APLIC DT node for guest domains

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/3/26 5:10 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -14,6 +14,7 @@
> >   #include <xen/cpumask.h>
> >   #include <xen/device_tree.h>
> >   #include <xen/errno.h>
> > +#include <xen/fdt-kernel.h>
> >   #include <xen/init.h>
> >   #include <xen/macros.h>
> >   #include <xen/sched.h>
> > @@ -522,3 +523,9 @@ int __init imsic_init(const struct dt_device_node *node)
> >   
> >       return rc;
> >   }
> > +
> > +int __init vimsic_make_domu_dt_node(struct kernel_info *kinfo,
> > +                                    unsigned int *phandle)
> > +{
> > +    return -EOPNOTSUPP;
> > +}
> This, I assume, is going to be filled properly by the next patch.
Yes as it was mentioned in the commit message. Actually here considering 
that this function isn't static I can just re-order patches and drop 
part of commit message in the current patch.
> > --- a/xen/arch/riscv/vaplic.c
> > +++ b/xen/arch/riscv/vaplic.c
> > @@ -9,6 +9,8 @@
> >    */
> >   
> >   #include <xen/errno.h>
> > +#include <xen/fdt-kernel.h>
> > +#include <xen/libfdt/libfdt.h>
> >   #include <xen/sched.h>
> >   #include <xen/xvmalloc.h>
> >   
> > @@ -19,8 +21,11 @@
> >   
> >   #include "aplic-priv.h"
> >   
> > +#define VAPLIC_COMPATIBLE "riscv,aplic"
> >   #define VAPLIC_NUM_SOURCES 96
> >   
> > +#define FDT_VAPLIC_INT_CELLS 2
> > +
> >   static int cf_check vcpu_vaplic_init(struct vcpu *v)
> >   {
> >       int rc = 0;
> > @@ -47,6 +52,73 @@ static int cf_check vcpu_vaplic_init(struct vcpu *v)
> >       return rc;
> >   }
> >   
> > +static int __init cf_check vaplic_make_domu_dt_node(struct kernel_info *kinfo)
> Again - why __init here and ...
>
> > +{
> > +    int res = 0;
> > +    void *fdt = kinfo->fdt;
> > +    unsigned int msi_parent_phandle;
> > +    char vaplic_name[128];
> > +    paddr_t aplic_addr = GUEST_APLIC_S_BASE;
> > +    paddr_t aplic_size = APLIC_SIZE(kinfo->bd.d->max_vcpus);
> > +    const __be32 reg[] = {
> > +        cpu_to_be32(aplic_addr >> 32),
> > +        cpu_to_be32(aplic_addr),
> > +        cpu_to_be32(aplic_size >> 32),
> > +        cpu_to_be32(aplic_size),
> > +    };
> > +    struct vintc *vintc = kinfo->bd.d->arch.vintc;
> > +
> > +    res = snprintf(vaplic_name, sizeof(vaplic_name), "/soc/aplic@%x",
> > +                   GUEST_APLIC_S_BASE);
> > +    if ( res >= sizeof(vaplic_name) )
> > +    {
> > +        dprintk(XENLOG_DEBUG, "vaplic name is truncated\n");
> > +        return -ENOBUFS;
> > +    }
> > +
> > +    res = vimsic_make_domu_dt_node(kinfo, &msi_parent_phandle);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_begin_node(fdt, vaplic_name);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#interrupt-cells", FDT_VAPLIC_INT_CELLS);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "riscv,num-sources", vintc->irq_nums);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_string(fdt, "compatible", VAPLIC_COMPATIBLE);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "msi-parent", msi_parent_phandle);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "phandle", kinfo->phandle_intc);
> > +    if ( res )
> > +        return res;
> > +
> > +    return fdt_end_node(fdt);
> > +}
> > +
> > +static const struct vintc_init_ops __initdata init_ops = {
> ... __initdata here? If you really want to have the option of moving
> domain creation stuff to .init.* when only dom0less is configured,
> then a proper abstraction is needed, along the lines of
> init_or_livepatch.
It was marked as __init and __initdata, respectively, because I had the 
dom0less use case in mind, where all domains are created during boot. In 
that scenario, vaplic_make_domu_dt_node() would not be reused, so the 
associated memory could be freed afterwards.
In the case of Dom0 with the xl toolstack, I assume the toolstack will 
create the node for the guest domain. For Dom0, however, I am not sure 
whether we can reuse the current implementation as-is. As it is written 
now, I assume that properties such as aplic_addr, reg, and compatible 
should be reused from the host, right?
If that is not a requirement for Dom0, then __init and __initdata could 
be removed (and possibly init_ops as well).
> > @@ -60,13 +132,14 @@ 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;
> > +    vaplic->regs.domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM |
> > +                             APLIC_DOMAINCFG_RO80;
> This looks unrelated. I don't mind it being done here, but then it
> wants mentioning in the description. Or maybe I simply don't understand
> what this is about.
Right, it shouldn't be here. It would be better to move to [PATCH v2 
14/26] xen/riscv: add very early virtual APLIC (vAPLIC) initialization 
support.
Thanks.

~ Oleksii