Re: [PATCH v2 17/26] xen/riscv: generate IMSIC DT node for guest domains

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/3/26 5:21 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
> > Guests using the IMSIC interrupt controller require a corresponding
> > Device Tree description.
> >
> > Add support for generating an IMSIC node when building the guest DT.
> > This allows guests to discover and use the IMSIC interrupt controller.
> >
> > Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v2:
> >   - s/imsic_make_reg_property/guest_imsic_make_reg_property.
> >   - s/imsic_set_interrupt_extended_prop/guest_imsic_set_interrupt_extended_prop.
> >   - Use initalizer for regs[] array in imsic_make_reg_property().
> >   - Move buf[] insde the for() loop.
> >   - Correct check of returned phandle.
> >   - Drop local variable len.
> >   - /s/XVFREE/xvfree in imsic_set_interrupt_extended_prop().
> >   - Drop initializer for local variable data.
> >   - s/uint32_t/unsinged int for pos and cpu in 
> > imsic_set_interrupt_extended_prop().
> >   - Drop next_phandle as it is now in common code.
> >   - Introduce vcpu_imsic_deinit.
> >   - Refactor vimsic_make_domu_dt_node() to avoid usage of host IMSIC dt node.
> > ---
> >   xen/arch/riscv/imsic.c                    | 127 +++++++++++++++++++++-
> >   xen/arch/riscv/include/asm/guest-layout.h |   2 +
> >   2 files changed, 128 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> > index ceea6778d9dc..19cbacdf96e1 100644
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -13,9 +13,12 @@
> >   #include <xen/const.h>
> >   #include <xen/cpumask.h>
> >   #include <xen/device_tree.h>
> > +#include <xen/domain.h>
> >   #include <xen/errno.h>
> > +#include <xen/fdt-domain-build.h>
> >   #include <xen/fdt-kernel.h>
> >   #include <xen/init.h>
> > +#include <xen/libfdt/libfdt.h>
> >   #include <xen/macros.h>
> >   #include <xen/sched.h>
> >   #include <xen/smp.h>
> > @@ -35,6 +38,11 @@ static struct imsic_config imsic_cfg = {
> >       .lock = SPIN_LOCK_UNLOCKED,
> >   };
> >   
> > +static unsigned int __ro_after_init guest_num_msis;
> How come this is __ro_after_init, when it's ...
>
> > @@ -291,6 +299,11 @@ static int imsic_parse_node(const struct dt_device_node 
> > *node,
> >           return -ENOENT;
> >       }
> >   
> > +    if ( dt_property_read_u32(node, "riscv,num-guest-ids", &tmp) )
> > +        guest_num_msis = tmp;
> > +    else
> > +        guest_num_msis = imsic_cfg.nr_ids;
> ... written by a non-__init function?
__ro_after_init should be dropped. I will use __read_mostly instead.

 Plus are you again inheriting a host
> property into guests without saying why?
Based on the discussion for similar place around vAPLIC code in the 
previous patch I will add here or the comment why or introduce 
VIMISC_NR_IDS and use it instead.
> > @@ -524,8 +537,120 @@ int __init imsic_init(const struct dt_device_node *node)
> >       return rc;
> >   }
> >   
> > +static int __init guest_imsic_make_reg_property(struct domain *d, void *fdt)
> Same question again as to __init throughout here.
From Arm code I understood that libxl creates the interrupt controller 
node in userspace before handing the FDT to Xen so this and related 
functions won't be called by userspace.
And for dom0's it is a separate *_make_hdwom_dt_node() (took it from Arm 
code base) which can expose the real hardware MMIO regions to dom0 
(since dom0 may need direct hardware access - if it isn't strict 
requirement then the current one way to create node could be re-used).
So if for dom0/hwdom it should be created separate node then we will 
have similar separate function for IMSIC node here and then it looks 
fine to keep __init.
> > +{
> > +    paddr_t base_addr = GUEST_IMSIC_S_BASE;
> So you make a local variable for a constant, ...
>
> > +    __be32 regs[4] = {
> > +        cpu_to_be32(base_addr >> 32),
> > +        cpu_to_be32(base_addr),
> > +        cpu_to_be32((IMSIC_MMIO_PAGE_SZ * d->max_vcpus) >> 32),
> > +        cpu_to_be32(IMSIC_MMIO_PAGE_SZ * d->max_vcpus),
> ... but this non-constant expression is spelled out twice.
I will use instead of base_addr GUEST_IMSIC_S_BASE directly and 
introduce local variable for:
  paddr_t size = IMSIC_MMIO_PAGE_SZ * d->max_vcpus;

> > +static int __init guest_imsic_set_interrupt_extended_prop(struct domain *d,
> > +                                                          void *fdt)
> > +{
> > +    unsigned int cpu, pos = 0;
> > +    uint32_t phandle;
> > +    uint32_t *irq_ext;
> Doesn't this want to be __be32, seeing ...
>
> > +    int res;
> > +
> > +    irq_ext = xvzalloc_array(uint32_t, d->max_vcpus * 2);
> > +    if ( !irq_ext )
> > +        return -ENOMEM;
> > +
> > +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> > +    {
> > +        char buf[64];
> > +
> > +        snprintf(buf, sizeof(buf), "/cpus/cpu@%u/interrupt-controller", cpu);
> > +        phandle = fdt_get_phandle(fdt, fdt_path_offset(fdt, buf));
> > +
> > +        if ( !phandle )
> > +        {
> > +            res = -ENODEV;
> > +            goto out;
> > +        }
> > +
> > +        irq_ext[pos++] = cpu_to_be32(phandle);
> > +        irq_ext[pos++] = cpu_to_be32(IRQ_S_EXT);
> ... this?
Agree, __be32 should be added for definition of irq_ext.

> Also, just like "buf", "phandle" can be local to this loop's body.
>
> > --- a/xen/arch/riscv/include/asm/guest-layout.h
> > +++ b/xen/arch/riscv/include/asm/guest-layout.h
> > @@ -5,6 +5,8 @@
> >   
> >   #define GUEST_APLIC_S_BASE 0xd000000
> >   
> > +#define GUEST_IMSIC_S_BASE 0x28000000
> > +
> >   #define GUEST_RAM_BANKS   2
> Is this going to become an unannotated collection of (seemingly) random
> numbers?
Could you please clarify what kind of annotation you would expect to see 
here? It's just an address, which is typically used for IMSIC by QEMU, 
so I chose the same notation for the guest address as well.
Thanks.

~ Oleksii