Re: [PATCH] xen/riscv: bound each IMSIC MMIO regset by the per-group hart slots

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/13/26 11:59 AM, Baptiste Le Duc wrote:
> imsic_init() requires every MMIO register set to be exactly
> IMSIC_HART_SIZE(guest_index_bits) * nr_parent_irqs bytes, i.e. to cover
> the interrupt files of all harts listed in interrupts-extended. This only
> holds when the DT describes a single register set.
>
> When harts are split into groups (riscv,group-index-bits > 0), each group
> has its own register set, which only covers the harts of that group. For
> example, the SpacemiT V100 IMSIC node describes two 2 MiB register sets,
> each holding 64 hart slots of 32 KiB, with 128 parent interrupts in total.
> Xen expects 4 MiB per register set and fails with:
>
>    IMSIC MMIO size is incorrect 2097152, expected MMIO size: 4194304
>
> A register set can also be smaller than its slot space, when the number
> of harts in a group is not a power of two.
>
> Check instead that a register set does not exceed the address space of
> one group of BIT(hart_index_bits) hart slots.
The term "slot space" appears before it is explained: the paragraph
using it comes before the one that introduces "BIT(hart_index_bits)
hart slots". If the check below is changed as suggested, these two
paragraphs could become:

  Check instead that a register set does not run past the end of the
  group it starts in, i.e. BIT(hart_index_bits) hart slots. A register
  set may be smaller than that, e.g. when the number of harts in a group
  is not a power of two.

> While at it, print the MMIO sizes in hex, so they can be compared directly
> with the reg property of the DT node.
>
> Fixes: c9bd8b322ecb ("xen/riscv: imsic_init() implementation")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> ---
>   xen/arch/riscv/imsic.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index 44b8640ea3..255ac44b52 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -416,8 +416,8 @@ int __init imsic_init(const struct dt_device_node *node)
>       for ( unsigned int i = 0; i < nr_mmios; i++ )
>       {
>           unsigned int guest_bits = imsic_cfg.guest_index_bits;
> -        unsigned long expected_mmio_size =
> -            IMSIC_HART_SIZE(guest_bits) * nr_parent_irqs;
> +        unsigned long max_mmio_regset_size =
> +            IMSIC_HART_SIZE(guest_bits) * BIT(imsic_cfg.hart_index_bits, U);
>   
>           rc = dt_device_get_address(node, i, &mmios[i].base_addr,
>                                      &mmios[i].size);
> @@ -442,11 +442,11 @@ int __init imsic_init(const struct dt_device_node *node)
>               goto imsic_init_err;
>           }
>   
> -        if ( mmios[i].size != expected_mmio_size )
> +        if ( mmios[i].size > max_mmio_regset_size )
The address check above masks the hart and guest index bits, so a
register set may start at any hart slot of its group. The binding
describes each reg entry as "Base address of each IMSIC group", i.e. a
register set lies within one group. Bounding only the size doesn't
enforce that: a register set which starts in the middle of a group can
run past the end of the group and still pass.

For example, take the IMSIC node of QEMU virt with two NUMA nodes of
three harts each (guest-index-bits = 3, hart-index-bits = 2, groups of
0x20000) and move register set 0 to <0x28010000 0x18000>. It starts at
slot 2 and claims three slots, so the third one ends 0x8000 past the end
of the group. With this patch Xen accepts the node, and the interrupt
file of the third hart ends up at 0x28020000, where bit 17 is set: that
bit is part of the base address, not of the hart index, so no hart of
group 0 lives there.

Taking the offset of the register set within its group into account
fixes that, and computing the group size with a single BIT(..., UL)
also avoids the 32-bit shift by a DT-provided hart_index_bits:

--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ -415,9 +415,11 @@ int __init imsic_init(const struct dt_device_node 
*node)
     /* Check MMIO register sets */
     for ( unsigned int i = 0; i < nr_mmios; i++ )
     {
-        unsigned int guest_bits = imsic_cfg.guest_index_bits;
-        unsigned long max_mmio_regset_size =
-            IMSIC_HART_SIZE(guest_bits) * 
BIT(imsic_cfg.hart_index_bits, U);
+        /* Address space of one group: BIT(hart_index_bits) hart slots. */
+        unsigned long group_size = BIT(imsic_cfg.guest_index_bits +
+                                       imsic_cfg.hart_index_bits +
+                                       IMSIC_MMIO_PAGE_SHIFT, UL);
+        unsigned long offset;

         rc = dt_device_get_address(node, i, &mmios[i].base_addr,
                                    &mmios[i].size);
@@ -429,9 +431,7 @@ int __init imsic_init(const struct dt_device_node *node)
         }

         base_addr = mmios[i].base_addr;
-        base_addr &= ~(BIT(guest_bits +
-                           imsic_cfg.hart_index_bits +
-                           IMSIC_MMIO_PAGE_SHIFT, UL) - 1);
+        base_addr &= ~(group_size - 1);
         base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) <<
                        imsic_cfg.group_index_shift);
         if ( base_addr != imsic_cfg.base_addr )
@@ -442,11 +442,13 @@ int __init imsic_init(const struct dt_device_node 
*node)
             goto imsic_init_err;
         }

-        if ( mmios[i].size > max_mmio_regset_size )
+        /* A register set must not run past the end of its group. */
+        offset = mmios[i].base_addr & (group_size - 1);
+        if ( mmios[i].size > group_size - offset )
         {
             rc = -EINVAL;
-            printk(XENLOG_ERR "%s: IMSIC MMIO regset %u size %#lx 
larger than max %#lx\n",
-                   node->name, i, mmios[i].size, max_mmio_regset_size);
+            printk(XENLOG_ERR "%s: IMSIC MMIO regset %u (offset %#lx, 
size %#lx) exceeds group size %#lx\n",
+                   node->name, i, offset, mmios[i].size, group_size);
             goto imsic_init_err;
         }
     }

I checked it on top of your patch with QEMU virt (aia=aplic-imsic, two
groups of two and of three harts): Xen boots as with your version, while
the node with the shifted register set is now rejected with:

  interrupt-controller: IMSIC MMIO regset 0 (offset 0x10000, size 
0x18000) exceeds group size 0x20000
Probably it will be better to make print message as follow:

            printk(XENLOG_ERR "%s: IMSIC MMIO regset %u (offset %#lx + 
size %#lx) exceeds group size %#lx\n",
                   node->name, i, offset, mmios[i].size, group_size);


The V100 node from the commit message passes as well, since both of its
register sets are aligned to the group size.

>           {
>               rc = -EINVAL;
> -            printk(XENLOG_ERR "%s: IMSIC MMIO size is incorrect %ld, expected MMIO 
> size: %ld\n",
> -                   node->name, mmios[i].size, expected_mmio_size);
> +            printk(XENLOG_ERR "%s: IMSIC MMIO regset %u size %#lx larger than max 
> %#lx\n",
> +                   node->name, i, mmios[i].size, max_mmio_regset_size);
>               goto imsic_init_err;
>           }
>       }
>
> ---
> base-commit: 97bf370375113caf92fccf04e2877fc6b5ff887f
> change-id: 20261113-fix-imsic-mmio-dt-c3317ac2efec
(out of curiosity) Is it automatically generated by b4?

~ Oleksii