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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/29/26 5:19 PM, Jan Beulich wrote:
> On 26.06.2026 17:46, 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,16 @@ 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 __read_mostly guest_num_msis;
> > +
> > +#define GUEST_IMSIC_COMPATIBLE "riscv,imsics"
> > +#define GUEST_IMSIC_NUM_MSIS 255
> Considering its use this isn't named correctly - it's not the number of MSIs
> guests get to use.
I will rename to GUEST_IMSIC_NUM_IDS then it will be fully aligned with 
dts property name.
Then it makes sense to rename guest_num_msis to guest_num_ids.

> > @@ -285,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_MAX_ID;
> Why is guest_num_msis __read_mostly, not __ro_after_init? Merely because the
> function here (wrongly) isn't __init?
Yes, because of the function isn't __init what I can agree is wrong. I 
will change that and s/__read_mostly/__ro_after_init for guest_num_msis.
> > @@ -522,3 +541,121 @@ 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, sizeof(buf), "/cpus/cpu@%u/interrupt-controller", cpu);
> May I, btw, suggest you get into the habit of using ARRAY_SIZE() in favor of
> sizeof() with snprintf()? That's because sizeof() isn't correct to use if
> wide strings (and hence swnprintf()) come into play.
Oh, right with wchar_t buf[64] sizeof(buf) won't return 64...

I will apply your suggestion and use ARRAY_SIZE() here instead.

> > +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;
> > +    unsigned int num_msis = min(GUEST_IMSIC_NUM_MSIS + 0U, guest_num_msis);
> As guest_num_msis (supposedly) doesn't change anymore after it was set, why
> would this need calculating again for each call here? Can't you apply the
> upper bound right in imsic_parse_node()?
Agree, I will add the following to imsic_parse_node() after 
guest_num_msis is init-ed:
guest_num_msis = min(GUEST_IMSIC_NUM_MSIS + 0U, guest_num_msis);

> > --- a/xen/arch/riscv/include/asm/guest-layout.h
> > +++ b/xen/arch/riscv/include/asm/guest-layout.h
> > @@ -3,6 +3,12 @@
> >   
> >   #include <public/xen.h>
> >   
> > +/*
> > + * Base address of the guest's supervisor-mode IMSIC. The value is the address
> > + * typically used for IMSIC by QEMU.
> > + */
> > +#define GUEST_IMSIC_S_BASE _UL(0x28000000)
> As you mention it explicitly: Is there also a user-mode IMSIC?
I am not aware of such.

I mention it explicitly as machine mode IMSIC exists and it is a 
separate DT node for that.
Out of scope: there is a reserved bit in MISA register for user-level 
interrupts but I don't see that AIA uses that bit.
Thanks.

~ Oleksii