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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/9/26 5:49 PM, Jan Beulich wrote:
> On 06.07.2026 17:58, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -13,8 +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>
> > @@ -34,6 +38,21 @@ static struct imsic_config imsic_cfg = {
> >       .lock = SPIN_LOCK_UNLOCKED,
> >   };
> >   
> > +/*
> > + * Number of MSIs available to a guest. Determined by the host interrupt
> > + * controller, so it is identical for every domain -- hence a single global
> > + * rather than a per-domain value.
> > + */
> > +static unsigned int __ro_after_init guest_num_msis;
> > +
> > +#define GUEST_IMSIC_COMPATIBLE "riscv,imsics"
> > +
> > +/*
> > + * Value is inspired by what QEMU is using for riscv,num-ids property for IMSIC
> > + * node.
> > + */
> > +#define GUEST_IMSIC_MAX_MSIS 255
> #define GUEST_IMSIC_MAX_MSIS 255U
>
> to avoid ...
>
> > @@ -285,6 +304,13 @@ 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_MAX_ID;
> > +
> > +    guest_num_msis = min(GUEST_IMSIC_MAX_MSIS + 0U, guest_num_msis);
> ... the "+ 0U" here?
Good point. I'll apply.

> As to the use of IMSIC_MAX_ID - why not use GUEST_IMSIC_MAX_MSIS right away?
> Overall perhaps
>
>      if ( dt_property_read_u32(node, "riscv,num-guest-ids", &tmp) )
>          guest_num_msis = min(GUEST_IMSIC_MAX_MSIS, tmp);
>      else
>          guest_num_msis = GUEST_IMSIC_MAX_MSIS;
>
> ?
Agree, it would be better.

> > @@ -522,3 +548,119 @@ 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)
> > +{
> > +    paddr_t size = IMSIC_MMIO_PAGE_SZ * d->max_vcpus;
> > +    __be32 regs[4] = {
> > +        cpu_to_be32(GUEST_IMSIC_S_BASE >> 32),
> > +        cpu_to_be32(GUEST_IMSIC_S_BASE),
> > +        cpu_to_be32(size >> 32),
> > +        cpu_to_be32(size),
> > +    };
> > +
> > +    return fdt_property(fdt, "reg", regs, sizeof(regs));
> > +}
> > +
> > +static int __init guest_imsic_set_interrupt_extended_prop(struct domain *d,
> > +                                                          void *fdt)
> > +{
> > +    unsigned int cpu, pos = 0;
> > +    __be32 *irq_ext;
> > +    int res;
> > +
> > +    irq_ext = xvzalloc_array(__be32, d->max_vcpus * 2);
> > +    if ( !irq_ext )
> > +        return -ENOMEM;
> > +
> > +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> > +    {
> > +        char buf[64];
> > +        uint32_t phandle;
> > +
> > +        snprintf(buf, ARRAY_SIZE(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);
> > +    }
> > +
> > +    res = fdt_property(fdt, "interrupts-extended", irq_ext,
> > +                       d->max_vcpus * 2 * sizeof(*irq_ext));
> > +
> > + out:
> > +    xvfree(irq_ext);
> > +
> > +    return res;
> > +}
> > +
> > +int __init vimsic_make_domu_dt_node(struct kernel_info *kinfo,
> > +                                    unsigned int *phandle)
> > +{
> > +    int res;
> > +    void *fdt = kinfo->fdt;
> > +    char vimsic_name[32];
> > +    unsigned int vimsic_phandle;
> > +    res = snprintf(vimsic_name, sizeof(vimsic_name), "/soc/imsic@%lx",
> > +                   GUEST_IMSIC_S_BASE);
> Blank line please between declaration(s) and statement(s). Also the revlog
> claims you switched to ARRAY_SIZE() everywhere, when really you didn't.
I will do that next patch series version.

Thanks.

~ Oleksii