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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/1/26 1:55 PM, Jan Beulich wrote:
> 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).
Then GUEST_IMSIC_MAX_IDS (or *_MSIS) would be better.

~ Oleksii