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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

> +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()?

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

Jan