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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/14/25 11:32 AM, Jan Beulich wrote:

> On 08.04.2025 17:57, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -0,0 +1,286 @@
> > +/* SPDX-License-Identifier: MIT */
> > +
> > +/*
> > + * xen/arch/riscv/imsic.c
> > + *
> > + * RISC-V Incoming MSI Controller support
> > + *
> > + * (c) 2023 Microchip Technology Inc.
> > + * (c) 2024 Vates
> >       
> No 2025 here (if already the years matter)?
>     
I think it doesn't really matter and it could be just dropped.

> > + */
> > +
> > +#include <xen/const.h>
> > +#include <xen/device_tree.h>
> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +#include <xen/macros.h>
> > +#include <xen/xmalloc.h>
> > +
> > +#include <asm/imsic.h>
> > +
> > +static struct imsic_config imsic_cfg;
> > +
> > +const struct imsic_config *imsic_get_config(void)
> >       
> Does this need to return a pointer to non-const?
>     
No, I think it could be const struct imsic_config * const as a return type.
It isn't expected that the caller will change a pointer.

> > +{
> > +    return &imsic_cfg;
> > +}
> > +
> > +static int __init imsic_get_parent_hartid(struct dt_device_node *node,
> > +                                          unsigned int index,
> > +                                          unsigned long *hartid)
> > +{
> > +    int res;
> > +    unsigned long hart;
> > +    struct dt_phandle_args args;
> > +
> > +    /* Try the new-style interrupts-extended first */
> >       
> The comment says "first", but then ...
>
> > +    res = dt_parse_phandle_with_args(node, "interrupts-extended",
> > +                                     "#interrupt-cells", index, &args);
> > +    if ( !res )
> > +    {
> > +        res = riscv_of_processor_hartid(args.np->parent, &hart);
> > +        if ( res < 0 )
> > +            return -EINVAL;
> > +
> > +        *hartid = hart;
> > +    }
> > +    return res;
> > +}
> >       
> ... nothing else is being tried.
>     
A stale comment, we decided to support only interrupt-extended to be in line with
Linux device tree bindings.
I'll drop this comment.

> Also, nit: Blank line please ahead of the main "return" of a function.
>
> Further - any particular reason to discard riscv_of_processor_hartid()'s
> error code on the error path?
>     
No particular reason, just overlooked that we could really return just `res`.


> > +static int imsic_parse_node(struct dt_device_node *node,
> > +                     unsigned int *nr_parent_irqs)
> >       
>     
(for me: fix an indentation)
> > +{
> > +    int rc;
> > +    unsigned int tmp;
> > +    paddr_t base_addr;
> > +
> > +    /* Find number of parent interrupts */
> > +    *nr_parent_irqs = dt_number_of_irq(node);
> > +    if ( !*nr_parent_irqs )
> > +    {
> > +        printk(XENLOG_ERR "%s: no parent irqs available\n", node->name);
> > +        return -ENOENT;
> > +    }
> > +
> > +    /* Find number of guest index bits in MSI address */
> > +    rc = dt_property_read_u32(node, "riscv,guest-index-bits",
> > +                              &imsic_cfg.guest_index_bits);
> > +    if ( !rc )
> >       
> It is confusing to store a bool return value in a local "int" variable,
> just to then use it as boolean. Is the local var needed at all here?
>     
'int` is used because of ...:
  rc = dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL);
... at the end of imsic_parse_node().

Agree, we can just drop `rc` variable and return err code explicitly.

> > +        imsic_cfg.guest_index_bits = 0;
> > +    tmp = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT;
> > +    if ( tmp < imsic_cfg.guest_index_bits )
> > +    {
> > +        printk(XENLOG_ERR "%s: guest index bits too big\n", node->name);
> > +        return -ENOENT;
> > +    }
> > +
> > +    /* Find number of HART index bits */
> > +    rc = dt_property_read_u32(node, "riscv,hart-index-bits",
> > +                              &imsic_cfg.hart_index_bits);
> > +    if ( !rc )
> > +    {
> > +        /* Assume default value */
> > +        imsic_cfg.hart_index_bits = fls(*nr_parent_irqs);
> > +        if ( BIT(imsic_cfg.hart_index_bits, UL) < *nr_parent_irqs )
> > +            imsic_cfg.hart_index_bits++;
> > +    }
> > +    tmp = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT -
> > +          imsic_cfg.guest_index_bits;
> >       
>     tmp -= imsic_cfg.guest_index_bits;
>
> ? (And then similarly further down.)
>     
Yes, it would be simpler.

> > +    if ( tmp < imsic_cfg.hart_index_bits )
> > +    {
> > +        printk(XENLOG_ERR "%s: HART index bits too big\n", node->name);
> > +        return -ENOENT;
> > +    }
> > +
> > +    /* Find number of group index bits */
> > +    rc = dt_property_read_u32(node, "riscv,group-index-bits",
> > +                              &imsic_cfg.group_index_bits);
> > +    if ( !rc )
> > +        imsic_cfg.group_index_bits = 0;
> > +    tmp = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT -
> > +          imsic_cfg.guest_index_bits - imsic_cfg.hart_index_bits;
> > +    if ( tmp < imsic_cfg.group_index_bits )
> > +    {
> > +        printk(XENLOG_ERR "%s: group index bits too big\n", node->name);
> > +        return -ENOENT;
> > +    }
> > +
> > +    /* Find first bit position of group index */
> > +    tmp = IMSIC_MMIO_PAGE_SHIFT * 2;
> > +    rc = dt_property_read_u32(node, "riscv,group-index-shift",
> > +                              &imsic_cfg.group_index_shift);
> > +    if ( !rc )
> > +        imsic_cfg.group_index_shift = tmp;
> > +    if ( imsic_cfg.group_index_shift < tmp )
> > +    {
> > +        printk(XENLOG_ERR "%s: group index shift too small\n", node->name);
> > +        return -ENOENT;
> > +    }
> > +    tmp = imsic_cfg.group_index_bits + imsic_cfg.group_index_shift - 1;
> > +    if ( tmp >= BITS_PER_LONG )
> > +    {
> > +        printk(XENLOG_ERR "%s: group index shift too big\n", node->name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Find number of interrupt identities */
> > +    rc = dt_property_read_u32(node, "riscv,num-ids", &imsic_cfg.nr_ids);
> > +    if ( !rc )
> > +    {
> > +        printk(XENLOG_ERR "%s: number of interrupt identities not found\n",
> > +               node->name);
> > +        return -ENOENT;
> > +    }
> > +
> > +    if ( (imsic_cfg.nr_ids < IMSIC_MIN_ID) ||
> > +         (imsic_cfg.nr_ids >= IMSIC_MAX_ID) ||
> >       
> Something named "max" normally wants to decribe the highest valid value,
> not the first out-of-range one.
>     
But it is the maximum of identities for IMISIC according to the AIA specs:
```
( MRIF stands for Memory-Resident Interrupt File )

All MRIFs are the size to accommodate 2047 valid interrupt identities,
the maximum allowed for an IMSIC interrupt file. If a system’s actual IMSICs
have interrupt files that implement only N interrupt identities, N < 2047,
then the contents of MRIFs for identities greater than N may be ignored by
softwareand
```
and prefix IMSIC (IMO) emphasizes this.

Would it be better to rename to IMSIC_MAX_ALLOWED_ID? And then IMSIC_MIN_ALLOWED_ID?

> > +         ((imsic_cfg.nr_ids & IMSIC_MIN_ID) != IMSIC_MIN_ID) )
> > +    {
> > +        printk(XENLOG_ERR "%s: invalid number of interrupt identities\n",
> > +               node->name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Compute base address */
> > +    imsic_cfg.nr_mmios = 0;
> > +    rc = dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL);
> > +    if (rc)
> >       
> Nit: Style.
>
> > +    {
> > +        printk(XENLOG_ERR "%s: first MMIO resource not found\n", node->name);
> > +        return -EINVAL;
> >       
> Discarding "rc" again?
>     
Agree, it could be just `rc`, but considering of that this functions always returns
-EINVAL in case of the error and that `rc` could be dropped at all, we just explicitly
return here -EINVAL.

> > +    }
> > +
> > +    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.

> > +    /* Find number of MMIO register sets */
> > +    imsic_cfg.nr_mmios++;
> > +    while ( !dt_device_get_address(node, imsic_cfg.nr_mmios, &base_addr, NULL) )
> > +        imsic_cfg.nr_mmios++;
> >       
> And the base addresses of these aren't of interest? Oh, I see they're
> fetched again further down.
>     
It is fetched again because we are using imsic_cfg.nr_mmios to calculate the size of
imsic_cfg.mmios array:
  imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, imsic_cfg.nr_mmios);

> Also - use do-while here?
>     
It could be, I'll update that.
> > +        if ( base_addr != imsic_cfg.base_addr )
> > +        {
> > +            rc = -EINVAL;
> > +            printk(XENLOG_ERR "%s: address mismatch for regset %d\n",
> > +                   node->name, i);
> > +            goto imsic_init_err;
> > +        }
> >       
> Oh, all of the addresses need to (sufficiently) match.
>
> > +    xfree(imsic_cfg.mmios);
> >       
> Better use XFREE() in cases like this one?
>     
I think, yes.

> > --- /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?
Should I use non-static version of bitmap declaration? (if we have such...)

> > +};
> > +
> > +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.

Thanks!

~ Oleksii