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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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;

?

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

Jan