[PATCH] lib: utils: irqchip: handle IMSIC mask boundaries

[Anup Patel]

On Thu, Aug 27, 2026 at 5:25 PM Pengpeng Hou <pengpeng at iscas.ac.cn> wrote:
>
> imsic_data_check() accepts field geometries that reach the native word
> width, but then constructs masks with 1UL << bits.  A full-width low field
> therefore shifts by BITS_PER_LONG.  The group endpoint calculation also
> underflows when group_index_bits is zero, even though an absent group field
> has no shift semantics.
>
> Build all geometry masks through a checked helper.  Represent a full-width
> field as ~0UL, keep zero-width fields as a zero mask, and validate the group
> shift only when a group field exists.  Reuse the resulting masks for base
> canonicalization and regset-size alignment.
>
> Fixes: 9f73669959dd ("lib: utils/irqchip: Add IMSIC library")
>
> Signed-off-by: Pengpeng Hou <pengpeng at iscas.ac.cn>

Overall direction of the patch is good but some needs improvements
from readability perspective.

> ---
> Base-commit: 4e79fd7de59f1b2899092c1a84ce68c8ebc68f93
>
>  lib/utils/irqchip/imsic.c | 72 +++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
> index f6da9f7..5f6eed5 100644
> --- a/lib/utils/irqchip/imsic.c
> +++ b/lib/utils/irqchip/imsic.c
> @@ -300,10 +300,30 @@ static int imsic_warm_irqchip_init(struct sbi_irqchip_device *dev)
>         return 0;
>  }
>
> +static int imsic_make_field_mask(u32 bits, u32 shift,
> +                                unsigned long *mask)

Better to make this as inline function named sbi_make_field_mask
and move to sbi_bitops.h

> +{
> +       if (!bits) {
> +               *mask = 0;
> +               return 0;
> +       }
> +
> +       if (shift >= BITS_PER_LONG || bits > BITS_PER_LONG - shift)
> +               return SBI_EINVAL;
> +
> +       if (bits == BITS_PER_LONG)
> +               *mask = ~0UL;
> +       else
> +               *mask = ((1UL << bits) - 1) << shift;

This should be:

*mask = GENMASK(bits + shift - 1, shift);

> +
> +       return 0;
> +}
> +
>  int imsic_data_check(struct imsic_data *imsic)
>  {
> -       u32 i, tmp;
> -       unsigned long base_addr, addr, mask;
> +       u32 i, low_index_bits, tmp;

Drop low_index_bits

> +       unsigned long addr, base_addr, group_mask, low_mask, regset_mask;

s/low_mask/hart_mask/
s/regset_mask/guest_mask/

> +       int rc;
>
>         /* Sanity checks */
>         if (!imsic ||
> @@ -325,13 +345,29 @@ int imsic_data_check(struct imsic_data *imsic)
>         if (tmp < imsic->group_index_bits)
>                 return SBI_EINVAL;
>
> -       tmp = IMSIC_MMIO_PAGE_SHIFT + imsic->guest_index_bits +
> -             imsic->hart_index_bits;
> -       if (imsic->group_index_shift < tmp)
> -               return SBI_EINVAL;
> -       tmp = imsic->group_index_bits + imsic->group_index_shift - 1;
> -       if (tmp >= BITS_PER_LONG)
> -               return SBI_EINVAL;
> +       low_index_bits = IMSIC_MMIO_PAGE_SHIFT + imsic->guest_index_bits +
> +                        imsic->hart_index_bits;
> +       rc = imsic_make_field_mask(low_index_bits, 0, &low_mask);
> +       if (rc)
> +               return rc;
> +
> +       rc = imsic_make_field_mask(imsic->guest_index_bits +
> +                                  IMSIC_MMIO_PAGE_SHIFT, 0, &regset_mask);
> +       if (rc)
> +               return rc;
> +

Replace above with:

rc = imsic_make_field_mask(imsic->guest_index_bits,
IMSIC_MMIO_PAGE_SHIFT, &guest_mask);
if (rc)
     return rc;

rc = imsic_make_field_mask(imsic->hart_index_bits,
IMSIC_MMIO_PAGE_SHIFT + imsic->guest_index_bits, &hart_mask);
if (rc)
     return rc;

> +       if (imsic->group_index_bits) {
> +               if (imsic->group_index_shift < low_index_bits)
> +                       return SBI_EINVAL;

The above check should be:

if (imsic->group_index_shift < IMSIC_MMIO_PAGE_SHIFT +
imsic->guest_index_bits + imsic->hart_index_bits)
    return SBI_EINVAL;

> +
> +               rc = imsic_make_field_mask(imsic->group_index_bits,
> +                                          imsic->group_index_shift,
> +                                          &group_mask);
> +               if (rc)
> +                       return rc;
> +       } else {
> +               group_mask = 0;
> +       }
>
>         /*
>          * Number of interrupt identities should be 1 less than
> @@ -346,23 +382,15 @@ int imsic_data_check(struct imsic_data *imsic)
>
>         /* Match patter of each regset */
>         base_addr = imsic->regs[0].addr;
> -       base_addr &= ~((1UL << (imsic->guest_index_bits +
> -                                imsic->hart_index_bits +
> -                                IMSIC_MMIO_PAGE_SHIFT)) - 1);
> -       base_addr &= ~(((1UL << imsic->group_index_bits) - 1) <<
> -                       imsic->group_index_shift);
> +       base_addr &= ~low_mask;
> +       base_addr &= ~group_mask;

This can simply be:

base_addr &= ~(group_mask | hart_mask | guest_mask);

>         for (i = 0; i < IMSIC_MAX_REGS && imsic->regs[i].size; i++) {
> -               mask = (1UL << imsic->guest_index_bits) * IMSIC_MMIO_PAGE_SZ;
> -               mask -= 1UL;
> -               if (imsic->regs[i].size & mask)
> +               if (imsic->regs[i].size & regset_mask)

This check must be:

if (imsic->regs[i].size & (guest_mask | (IMSIC_MMIO_PAGE_SZ - 1)))
    return SBI_EINVAL;

>                         return SBI_EINVAL;
>
>                 addr = imsic->regs[i].addr;
> -               addr &= ~((1UL << (imsic->guest_index_bits +
> -                                        imsic->hart_index_bits +
> -                                        IMSIC_MMIO_PAGE_SHIFT)) - 1);
> -               addr &= ~(((1UL << imsic->group_index_bits) - 1) <<
> -                               imsic->group_index_shift);
> +               addr &= ~low_mask;
> +               addr &= ~group_mask;

Same comment as above:

addr &= ~(group_mask | hart_mask | guest_mask);

>                 if (base_addr != addr)
>                         return SBI_EINVAL;
>         }
> --
> 2.50.1 (Apple Git-155)
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Regards,
Anup