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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.06.2026 16:21, Oleksii Kurochko wrote:
> 
> 
> On 6/3/26 5:21 PM, Jan Beulich wrote:
>> 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?
> 
> __ro_after_init should be dropped. I will use __read_mostly instead.
> 
>   Plus are you again inheriting a host
>> property into guests without saying why?
> 
> Based on the discussion for similar place around vAPLIC code in the 
> previous patch I will add here or the comment why or introduce 
> VIMISC_NR_IDS and use it instead.
> 
>>
>>> @@ -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.
> 
>  From Arm code I understood that libxl creates the interrupt controller 
> node in userspace before handing the FDT to Xen so this and related 
> functions won't be called by userspace.

Hmm, okay, on the v3 submission I'll have to keep that in mind, and call
paths will need checking accordingly.

>>> --- 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?
> 
> Could you please clarify what kind of annotation you would expect to see 
> here? It's just an address, which is typically used for IMSIC by QEMU, 
> so I chose the same notation for the guest address as well.

Well, how could I have known? The description says nothing, and there are
also no comments. The numbers here look entirely arbitrary, and if someone
wanted to change them, it wouldn't be clear at all what constraints need
taking into consideration.

Jan