Re: [PATCH v1 08/14] xen/riscv: imsic_init() implementation

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/16/25 8:31 AM, Jan Beulich wrote:

> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/include/asm/imsic.h
> > > > @@ -0,0 +1,66 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +
> > > > +/*
> > > > + * xen/arch/riscv/imsic.h
> > > > + *
> > > > + * RISC-V Incoming MSI Controller support
> > > > + *
> > > > + * (c) 2023 Microchip Technology Inc.
> > > > + */
> > > > +
> > > > +#ifndef ASM__RISCV__IMSIC_H
> > > > +#define ASM__RISCV__IMSIC_H
> > > > +
> > > > +#include <xen/types.h>
> > > > +
> > > > +#define IMSIC_MMIO_PAGE_SHIFT   12
> > > > +#define IMSIC_MMIO_PAGE_SZ      (1UL << IMSIC_MMIO_PAGE_SHIFT)
> > > > +
> > > > +#define IMSIC_MIN_ID            63
> > > > +#define IMSIC_MAX_ID            2048
> > > > +
> > > > +struct imsic_msi {
> > > > +    paddr_t base_addr;
> > > > +    unsigned long offset;
> > > > +};
> > > > +
> > > > +struct imsic_mmios {
> > > > +    paddr_t base_addr;
> > > > +    unsigned long size;
> > > > +    bool harts[NR_CPUS];
> > > An array of bool - won't a bitmap do here? Even then I wouldn't be overly
> > > happy to see it dimensioned by NR_CPUS.
> > Bitmap will fit here well. But for DECLARE_BITMAP() is necessary the size
> > of bitmap so NR_CPUS should be used again.
> > Could you please remind me why it isn't good to use it?
> > Because NR_CPUS not always equal to an amount of physical cpus?
> "Not equal" wouldn't be overly problematic. But NR_CPUS=4000 and the actual
> number of CPUs being 4 would be wasteful in general. More when its wider
> than a bit that's needed per CPU, but where would you draw the line if you
> permitted use of NR_CPUS here?
>
> > Should I use non-static version of bitmap declaration? (if we have such...)
> That's simply "unsigned long *" then, or - at the tail of a dynamically
> allocated struct - possibly unsigned long[].
>
> > > > +};
> > > > +
> > > > +struct imsic_config {
> > > > +    /* base address */
> > > > +    paddr_t base_addr;
> > > > +
> > > > +    /* Bits representing Guest index, HART index, and Group index */
> > > > +    unsigned int guest_index_bits;
> > > > +    unsigned int hart_index_bits;
> > > > +    unsigned int group_index_bits;
> > > > +    unsigned int group_index_shift;
> > > > +
> > > > +    /* imsic phandle */
> > > > +    unsigned int phandle;
> > > > +
> > > > +    /* number of parent irq */
> > > > +    unsigned int nr_parent_irqs;
> > > > +
> > > > +    /* number off interrupt identities */
> > > > +    unsigned int nr_ids;
> > > > +
> > > > +    /* mmios */
> > > > +    unsigned int nr_mmios;
> > > > +    struct imsic_mmios *mmios;
> > > > +
> > > > +    /* MSI */
> > > > +    struct imsic_msi msi[NR_CPUS];
> > > You surely can avoid wasting perhaps a lot of memory by allocating this
> > > based on the number of CPUs in use?
> > It make sense. I'll allocate then this dynamically.
> Or, as per above, when put at the tail and the struct itself is
> dynamically allocated, use struct imsic_msi[]. We even have dedicated
> xmalloc() flavors for this kind of allocation.
>     
Do you mean xzalloc_flex_struct()?

I think, I can't use for both of the cases (allocation of mmios and msi).
For msi[] then it is needed to allocate imsic_config also dynamically, isn't it?
So something like:
 imsic_config = xzalloc_flex_struct(struct imsic_config, msi, NR_CPUS).
But now it is allocated statically.

For *mmios and harts[] (a member inside struct imsic_mmios):
  mmios = xzalloc_flex_struct(struct imsic_mmios, harts, NR_CPUS); // NR_CPUs just for example...
It will allocate only one mmios, but it is needed mmios[nr_mmios].
Maybe, something like _xmalloc((offsetof(struct imsic_mmios, harts[NR_CPUS])) * NR_CPUS, sizeof(struct imsic_mmios)) will work.

Am I missing something?

~ Oleksii