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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- 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.

> @@ -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.

> --- 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?

Jan