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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2026 13:21, Oleksii Kurochko wrote:
> On 6/29/26 5:19 PM, Jan Beulich wrote:
>> 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.
> 
> I will rename to GUEST_IMSIC_NUM_IDS then it will be fully aligned with 
> dts property name.
> 
> Then it makes sense to rename guest_num_msis to guest_num_ids.

That's not going to help address my remark. I specifically referred to ...

>>> +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()?
> 
> Agree, I will add the following to imsic_parse_node() after 
> guest_num_msis is init-ed:
> 
> guest_num_msis = min(GUEST_IMSIC_NUM_MSIS + 0U, guest_num_msis);

... the sole use of the constant (here). The number of MSIs (or IDs or
whatever else) isn't GUEST_IMSIC_NUM_MSIS; that's merely an upper bound
(which in turn is there for an unknown to me reason).

>>> --- 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?
> 
> I am not aware of such.
> 
> I mention it explicitly as machine mode IMSIC exists and it is a 
> separate DT node for that.

Ah, okay. Then maybe keep the comment as is. It's not quite clear to me
whether, from an abstract perspective, "machine mode" could make sense
for guests. If it can't, "supervisor-mode" would be redundant with
"guest's".

Jan