Re: [PATCH v3 16/23] xen/riscv: generate IMSIC DT node for guest domains

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/22/26 5:12 PM, Jan Beulich wrote:
> On 17.06.2026 13:17, 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.
> >
> > The value choosen for GUEST_IMSIC_S_BASE is an address which is typically
> > used for IMSIC and QEMU.
> >
> > DT-building functions are marked __init because domain creation happens at
> > boot time, before the init sections are freed. In a typical deployment
> > libxl creates the interrupt controller node in userspace and hands the
> > complete FDT to Xen, so these functions are only called during early
> > domain construction.
> >
> > Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v3:
> >   - s/__ro_after_init/__read_mostly for guest_num_msis.
> >   - Use IMSIC_MAX_ID as default for guest_num_msis instead of imsic_cfg.nr_ids.
> >   - Drop base_addr local variable in guest_imsic_make_reg_property(); use
> >     GUEST_IMSIC_S_BASE directly and introduce size to avoid spelling
> >     IMSIC_MMIO_PAGE_SZ * d->max_vcpus twice.
> >   - Change irq_ext type from uint32_t * to __be32 * in
> >     guest_imsic_set_interrupt_extended_prop().
> >   - Move phandle declaration into the loop body.
> >   - Extend commit message to explain why __init is used for DT-building
> >     functions: libxl creates the interrupt controller node before handing
> >     the FDT to Xen, so these functions are only invoked during boot-time
> >     domain construction.
> >   - Re-order patch before APLIC DT node creation patch.
> >   - Update commit message.
> > ---
> > 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.
> > ---
> > ---
> >   ...asic-VGEIN-management-for-AIA-guests.patch | 273 ++++++++++++++++++
> What is this doing here?
Accidentally added to the patch, I will drop it.

> > --- 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>
> > @@ -36,6 +40,11 @@ static struct imsic_config imsic_cfg = {
> >       .lock = SPIN_LOCK_UNLOCKED,
> >   };
> >   
> > +static unsigned int __read_mostly guest_num_msis;
> This being host dependent and hence the same for all guests likely also
> warrants a comment.
Agree, I will add the comment:

/*
 * 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;

> > @@ -521,3 +535,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);
> > +        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[128];
> Isn't this excessive? You need space for ...
>
> > +    unsigned int vimsic_phandle;
> > +    unsigned int num_msis = min(GUEST_IMSIC_NUM_MSIS + 0U, guest_num_msis);
> > +
> > +    res = snprintf(vimsic_name, sizeof(vimsic_name), "/soc/imsic@%lx",
> ... up to 11 + 16 + 1 characters. So 32 will do, to make it a "nice" number.
Agree, 128 is too much and 32 will be more then enough.

> > --- a/xen/arch/riscv/include/asm/guest-layout.h
> > +++ b/xen/arch/riscv/include/asm/guest-layout.h
> > @@ -3,6 +3,8 @@
> >   
> >   #include <public/xen.h>
> >   
> > +#define GUEST_IMSIC_S_BASE __ULL(0x28000000)
> May I remind you of my request to not leave entirely arbitrary (and seemingly
> random) numbers uncommented?
Then I misunderstood you I added the a sentence to commit message:
  The value choosen for GUEST_IMSIC_S_BASE is an address which is 
typically used for IMSIC and QEMU.
I will put the simialar comment before defintion of GUEST_IMSIC_S_BASE.

Thanks.

~ Oleksii