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:

> > > > +    }
> > > > +
> > > > +    imsic_cfg.base_addr = base_addr;
> > > > +    imsic_cfg.base_addr &= ~(BIT(imsic_cfg.guest_index_bits +
> > > > +                   imsic_cfg.hart_index_bits +
> > > > +                   IMSIC_MMIO_PAGE_SHIFT, UL) - 1);
> > > > +    imsic_cfg.base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) <<
> > > > +                   imsic_cfg.group_index_shift);
> > > >           
> > > Besides indentation being bogus here, why is it that you need to mask bits
> > > off of the value read from DT? Wouldn't the expectation be that you get back
> > > the true base address?
> > >         
> > The group index is used to differentiate between clusters/groups. For 
> > example, consider two clusters: - Cluster 1 with cpu0 and cpu1 - Cluster 
> > 2 with cpu2 and cpu3 Then, the reg property in the IMSIC node will 
> > include two entries: reg = <0x0 0xd100000 0x0 0x20000>, <0x0 0x2d100000 
> > 0x0 0x20000>; riscv,guest-index-bits = <3>; riscv,hart-index-bits = <2>; 
> > riscv,group-index-bits = <1>; riscv,group-index-shift = <29>; In this 
> > example: The group index is 1 bit wide (group-index-bits = <1>), It is 
> > located at bit 29 (group-index-shift = <29>) of the address.
> >
> > so imsic_cfg.group_index_bits will be used to distinguish clusters, but 
> > they must have
> >
> > the same base address and this is the reason why the mask is applied.
> >       
> Well, yes, but that doesn't answer my question. All of what you say makes
> sense for the loop elsewhere retrieving all the addresses. Here you
> retrieved only the first of them, and in your example no masking would be
> needed here either. To re-phrase my question: Can the address observed in
> the first entry be other than the lowest one across the set of all
> entries?
>     
It doesn't mentioned explicitly in riscv,imsiscs binding that reg property
is sorted. 
I can write:
  reg = <0x0 0x2d100000 0x0 0x20000>,<0x0 0xd100000 0x0 0x20000>;
And a dtc compiler will compile.

> If not, can there be bits set across all entries that need
> masking off from all of them?
>     
I think that other bits (Hart Index and Guest Index) excepts base address
bits are 0 in dts, but it doesn't guarantee explicitly by IMSIC's dts bindinding.

> > > > --- /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?
>     
Hard to say. It seems it will be just better to use apporoach you suggested below.

Thanks.

~ Oleksii
> > 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.
>
> Jan
>