Re: [PATCH v1 10/27] xen/riscv: generate IMSIC DT node for guest domains

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 4/1/26 5:05 PM, Jan Beulich wrote:
> On 10.03.2026 18:08, 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.
>>
>> Keep a reference to the host IMSIC DT node and reuse its compatible
>> property while constructing the guest-visible node.
> 
> Again raises a migration concern. Presumably a guest would then be able
> to migrate only to other hosts with the same compatible property.

Right, but I don't think we can do too much and it is the simplest 
approach just to migrate to hosts with the same compatible property.

If you concern is about DTS property then for IMSIC it will be always 
riscv,imsics according to the RISC-V DT spec. (IIRC, the only other 
option could be qemu,riscv). Actually, I can just hard code 
"riscv,imsic" explicitly instead of re-using of host name.

If your concern is that we will migrate to a host which doesn't support 
IMSIC at all then we should or (a) don't migrate to such host or (b) 
provide an emulation of IMSIC. And option (b) would be terrible from at 
least performance point of view.

>> +static int __init imsic_set_interrupt_extended_prop(struct domain *d,
>> +                                                    void *fdt)
>> +{
>> +    uint32_t len = 0, pos = 0, cpu, phandle;
> 
> At least pos and cpu should be of fixed width types.

I thought that uint32_t is fixed width type, isn't it?

> 
>> +    uint32_t *irq_ext;
>> +    char buf[64];
> 
> Move this one into the loop's scope?

I will do that.

>> +    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++ )
>> +    {
>> +        snprintf(buf, sizeof(buf), "/cpus/cpu@%u/interrupt-controller", cpu);
>> +        phandle = fdt_get_phandle(fdt, fdt_path_offset(fdt, buf));
>> +
>> +        if ( phandle <= 0 )
>> +            return phandle;
> 
> <= is odd to use on an unsigned type. Arm, in a similar situation, uses != .

Agree, it should be !=.

> 
> Yet then - if 0 is an error indicator, aren't you converting this into a
> success indicator for the caller?

I read incorrectly the comment of what fdt_get_phandle() returns. so it 
should be:
         if ( !phandle )
         {
             res = -ENODEV;
             goto out;
         }

goto here as xvfree() should be called in this case.


> 
>> +        irq_ext[pos++] = cpu_to_be32(phandle);
>> +        len += sizeof(uint32_t);
> 
> As before, preferably sizeof(<expression>) to clarify the connection. It's
> pretty obvious here, but still. Yet then - do you really need to maintain
> "len"? Why not use ...
> 
>> +        irq_ext[pos++] = cpu_to_be32(IRQ_S_EXT);
>> +        len += sizeof(uint32_t);
>> +    }
>> +
>> +    res = fdt_property(fdt, "interrupts-extended", irq_ext, len);
> 
> ... "d->max_vcpus * 2 * sizeof(*irq_ext)" here?

Agree, it could and would be better.

> 
>> +    XVFREE(irq_ext);
> 
> This can be just xvfree(), as the variable goes out of scpoe just afterwards
> anyway.
> 
>> +    return res;
>> +}
>> +
>> +int __init imsic_make_dt_node(const struct kernel_info *kinfo)
> 
> So this function is unused until the next patch. Acceptable as long as Eclair
> (or other) scans aren't done for RISC-V, but generally you may want to try to
> get used to order patches such that this is avoided.
> 
>> +{
>> +    uint32_t len;
>> +    const void *data = NULL;
> 
> Why the initializer?

No, need any more, I had a case before so just missed to clean up properly.

> 
>> +    int res = 0;
>> +    void *fdt = kinfo->fdt;
>> +    const struct dt_device_node *host_imsic_node = imsic_cfg.host_node;
>> +    uint32_t *next_phandle = &kinfo->bd.d->arch.next_phandle;
>> +
>> +    res = fdt_begin_node(fdt, host_imsic_node->full_name);
>> +    if ( res )
>> +        return res;
>> +
>> +    data = dt_get_property(host_imsic_node, "compatible", &len);
>> +    if ( !data )
>> +    {
>> +        printk(XENLOG_ERR "%s: Can't find 'compatible' property\n",
>> +               host_imsic_node->full_name);
>> +
>> +        return -ENOENT;
>> +    }
> 
> Move fdt_begin_node() down below here?
> 
>> --- a/xen/arch/riscv/include/asm/imsic.h
>> +++ b/xen/arch/riscv/include/asm/imsic.h
>> @@ -57,11 +57,16 @@ struct imsic_config {
>>       /* MSI */
>>       const struct imsic_msi *msi;
>>   
>> +    /* DT node of IMSIC */
>> +    const struct dt_device_node *host_node;
> 
> Does "host" in the name carry much of a meaning? Maybe better dt_node, to
> (potentially) distinguish it from a NUMA node which also may need tracking
> for an imsic at some point?

Maybe, not too much. Lets rename it.

Thanks.

~ Oleksii