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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.05.2026 16:43, 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.
> 
> Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> 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.
> ---
>  xen/arch/riscv/imsic.c                    | 127 +++++++++++++++++++++-
>  xen/arch/riscv/include/asm/guest-layout.h |   2 +
>  2 files changed, 128 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index ceea6778d9dc..19cbacdf96e1 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -13,9 +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>
> @@ -35,6 +38,11 @@ static struct imsic_config imsic_cfg = {
>      .lock = SPIN_LOCK_UNLOCKED,
>  };
>  
> +static unsigned int __ro_after_init guest_num_msis;

How come this is __ro_after_init, when it's ...

> @@ -291,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_cfg.nr_ids;

... written by a non-__init function? Plus are you again inheriting a host
property into guests without saying why?

> @@ -524,8 +537,120 @@ 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)

Same question again as to __init throughout here.

> +{
> +    paddr_t base_addr = GUEST_IMSIC_S_BASE;

So you make a local variable for a constant, ...

> +    __be32 regs[4] = {
> +        cpu_to_be32(base_addr >> 32),
> +        cpu_to_be32(base_addr),
> +        cpu_to_be32((IMSIC_MMIO_PAGE_SZ * d->max_vcpus) >> 32),
> +        cpu_to_be32(IMSIC_MMIO_PAGE_SZ * d->max_vcpus),

... but this non-constant expression is spelled out twice.

> +static int __init guest_imsic_set_interrupt_extended_prop(struct domain *d,
> +                                                          void *fdt)
> +{
> +    unsigned int cpu, pos = 0;
> +    uint32_t phandle;
> +    uint32_t *irq_ext;

Doesn't this want to be __be32, seeing ...

> +    int res;
> +
> +    irq_ext = xvzalloc_array(uint32_t, d->max_vcpus * 2);
> +    if ( !irq_ext )
> +        return -ENOMEM;
> +
> +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> +    {
> +        char buf[64];
> +
> +        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);

... this?

Also, just like "buf", "phandle" can be local to this loop's body.

> --- a/xen/arch/riscv/include/asm/guest-layout.h
> +++ b/xen/arch/riscv/include/asm/guest-layout.h
> @@ -5,6 +5,8 @@
>  
>  #define GUEST_APLIC_S_BASE 0xd000000
>  
> +#define GUEST_IMSIC_S_BASE 0x28000000
> +
>  #define GUEST_RAM_BANKS   2

Is this going to become an unannotated collection of (seemingly) random
numbers?

Jan