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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/5/26 9:31 AM, Jan Beulich wrote:
> 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.
I will update the commit message with clarification why __init is used 
here and connected functions.
> > > > --- 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.
I will add the comment or update the commit message with information why 
this numbers were choosen.
Thanks.

~ Oleksii