Re: [PATCH 1/2] irqchip/riscv-imsic: Ignore unused group index shifts

[Anup Patel]

On Sun, Sep 6, 2026 at 1:38 PM Pengpeng Hou <hppiscas@xxxxxxx> wrote:
>
> The IMSIC parser validates and uses group_index_shift even when the
> group index has zero width. The binding permits a zero-width group with
> shift zero, but bits + shift - 1 underflows and rejects that layout.
> APLIC MSI setup also validates HHXS although HHXW is zero.
>
> Only validate and clear group-address bits when the group width is
> nonzero. Use zero for APLIC HHXS when there is no group field. This also
> avoids shifting an empty mask by an otherwise unused shift value.
>
> The issue was found by our static-analysis tool and manually reviewed.
>
> Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver")
> Assisted-by: GPT-5
> Signed-off-by: Pengpeng Hou <hppiscas@xxxxxxx>
> ---
>
> diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
> index fb8d1838..569b3ff4 100644
> --- a/drivers/irqchip/irq-riscv-aplic-msi.c
> +++ b/drivers/irqchip/irq-riscv-aplic-msi.c
> @@ -223,17 +223,21 @@ int aplic_msi_setup(struct device *dev, void __iomem *regs)
>                 return -EINVAL;
>         }
>
> -       /* Find first bit position of group index (HHXS) */
> -       mc->hhxs = imsic_global->group_index_shift;
> -       if (mc->hhxs < (2 * APLIC_xMSICFGADDR_PPN_SHIFT)) {
> -               dev_err(dev, "IMSIC group index shift should be >= %d\n",
> -                       (2 * APLIC_xMSICFGADDR_PPN_SHIFT));
> -               return -EINVAL;
> -       }
> -       mc->hhxs -= (2 * APLIC_xMSICFGADDR_PPN_SHIFT);
> -       if (APLIC_xMSICFGADDRH_HHXS_MASK < mc->hhxs) {
> -               dev_err(dev, "IMSIC group index shift big for APLIC HHXS\n");
> -               return -EINVAL;
> +       if (mc->hhxw) {
> +               /* Find first bit position of group index (HHXS) */
> +               mc->hhxs = imsic_global->group_index_shift;
> +               if (mc->hhxs < (2 * APLIC_xMSICFGADDR_PPN_SHIFT)) {
> +                       dev_err(dev, "IMSIC group index shift should be >= %d\n",
> +                               (2 * APLIC_xMSICFGADDR_PPN_SHIFT));
> +                       return -EINVAL;
> +               }
> +               mc->hhxs -= (2 * APLIC_xMSICFGADDR_PPN_SHIFT);
> +               if (APLIC_xMSICFGADDRH_HHXS_MASK < mc->hhxs) {
> +                       dev_err(dev, "IMSIC group index shift big for APLIC HHXS\n");
> +                       return -EINVAL;
> +               }
> +       } else {
> +               mc->hhxs = 0;

The IMSIC driver already ensures that group_index_shift is atleast 24
irrespective to the value of group_index_bits so this change is not needed
because in the "bits + shift - 1" the shift is always >= 24.

>         }
>
>         /* Compute PPN base */
> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index 9505ddbd..44b9c34c 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> @@ -741,10 +741,12 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode,
>         }
>
>         /* Sanity check group index shift */
> -       i = global->group_index_bits + global->group_index_shift - 1;
> -       if (i >= BITS_PER_LONG) {
> -               pr_err("%pfwP: group index shift too big\n", fwnode);
> -               return -EINVAL;
> +       if (global->group_index_bits) {
> +               i = global->group_index_bits + global->group_index_shift - 1;
> +               if (i >= BITS_PER_LONG) {
> +                       pr_err("%pfwP: group index shift too big\n", fwnode);
> +                       return -EINVAL;
> +               }
>         }
>
>         /* Sanity check number of interrupt identities */
> @@ -773,8 +775,9 @@ static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode,
>         global->base_addr &= ~GENMASK(global->guest_index_bits +
>                                        global->hart_index_bits +
>                                        IMSIC_MMIO_PAGE_SHIFT - 1, 0);
> -       global->base_addr &= ~((BIT(global->group_index_bits) - 1) <<
> -                              global->group_index_shift);
> +       if (global->group_index_bits)
> +               global->base_addr &= ~((BIT(global->group_index_bits) - 1) <<
> +                                      global->group_index_shift);

There is nothing to be fixed here.

For group_index_bits == 0, BIT(group_index_bits) - 1 will be zero
and 0 << any_shift_value will be 0. In other words, global->base_addr
won't change when group_index_bits == 0.

>
>         /* Find number of MMIO register sets */
>         while (!imsic_get_mmio_resource(fwnode, *nr_mmios, &res))
> @@ -854,8 +857,9 @@ int __init imsic_setup_state(struct fwnode_handle *fwnode, void *opaque)
>                 base_addr &= ~GENMASK(global->guest_index_bits +
>                                       global->hart_index_bits +
>                                       IMSIC_MMIO_PAGE_SHIFT - 1, 0);
> -               base_addr &= ~((BIT(global->group_index_bits) - 1) <<
> -                              global->group_index_shift);
> +               if (global->group_index_bits)
> +                       base_addr &= ~((BIT(global->group_index_bits) - 1) <<
> +                                      global->group_index_shift);

Same comment as above.

>                 if (base_addr != global->base_addr) {
>                         rc = -EINVAL;
>                         pr_err("%pfwP: address mismatch for regset %d\n", fwnode, i);
>
> base-commit: 13ca1c0b7d8a3ab1e59cb3e780c1dd7441e22515
>

Overall, this patch fixes nothing.

Regards,
Anup