Re: [PATCH v2 11/16] xen/riscv: aplic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2025 18:09, Oleksii Kurochko wrote:
> On 5/15/25 11:06 AM, Jan Beulich wrote:
>>> +    /* Set interrupt type and default priority for all interrupts */
>>> +    for ( i = 1; i <= aplic_info.num_irqs; i++ )
>>> +    {
>>> +        writel(0, &aplic.regs->sourcecfg[i - 1]);
>> What guarantees the loop to not run past the array's size?
> 
> riscv,aplic binding:
>    
> https://github.com/torvalds/linux/blob/a5806cd506af5a7c19bcd596e4708b5c464bfd21/Documentation/devicetree/bindings/interrupt-controller/riscv%2Caplic.yaml#L57
> Should I add an ASSERT() or panic() at the moment where num_irqs are
> initialized to double check a binding?

I may be paranoid, but I don't really trust anything coming from DT. Hence
yes, just like you do in various other situations, perhaps best to panic()
if too large a value is read. Or, if that's feasible, simply cap at the
compiled-in count.

>>> +static int __init cf_check aplic_init(void)
>>> +{
>>> +    int rc;
>>> +    dt_phandle imsic_phandle;
>>> +    uint32_t irq_range[num_possible_cpus() * 2];
>> Are you going to have enough stack space when num_possible_cpus() is pretty
>> large?
> 
> When num_possible_cpus() will be pretty large then it will better to allocate 
> irq_range[]
> dynamically.
> 
> Does it make sense to re-write now?

Well, this kind of runtime-sized stack allocation should go away anyway.
Plus you don't want to leave a trap like this in the code. Whether using
dynamic allocation is the choice to address those concerns you'll need
to judge.

>>> +    const struct dt_device_node *node = aplic_info.node;
>>> +
>>> +    /* Check for associated imsic node */
>>> +    rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
>>> +    if ( !rc )
>>> +        panic("%s: IDC mode not supported\n", node->full_name);
>>> +
>>> +    imsic_node = dt_find_node_by_phandle(imsic_phandle);
>>> +    if ( !imsic_node )
>>> +        panic("%s: unable to find IMSIC node\n", node->full_name);
>>> +
>>> +    rc = dt_property_read_u32_array(imsic_node, "interrupts-extended",
>>> +                                    irq_range, ARRAY_SIZE(irq_range));
>>> +    if ( rc )
>>> +        panic("%s: unable to find interrupt-extended in %s node\n",
>>> +              __func__, imsic_node->full_name);
>>> +
>>> +    if ( irq_range[1] == IRQ_M_EXT )
>> How do you know the array has had 2 or ...
>>
>>> +        /* Machine mode imsic node, ignore this aplic node */
>>> +        return 0;
>>> +
>>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(irq_range); i += 2 )
>>> +    {
>>> +        if ( irq_range[i + 1] != irq_range[1] )
>>> +            panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
>>> +    }
>> ... or even all of the slots populated? Anything not populated by the DT
>> function will still have the stack contents that had been left by earlier
>> call chains. (The loop also makes little sense to start from 0.)
> 
> Do you mean I asked allocated irq_range[8*2] but DT node has only 
> irq_range[4*2]?
> Then it will be really an issue. And out-of-range access will occur in
> dt_property_read_variable_u32_array(). I need another way to check then...

I described the opposite situation (not the full array being populated),
but yes - both are a problem.

Jan