Re: [PATCH v1 09/14] xen/riscv: aplic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2025 17:21, Oleksii Kurochko wrote:
> 
> On 4/16/25 12:30 PM, Jan Beulich wrote:
>> On 16.04.2025 12:15, Oleksii Kurochko wrote:
>>> On 4/14/25 12:04 PM, Jan Beulich wrote:
>>>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>>>> +    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);
>>>>> +
>>>>> +    /* check imsic mode */
>>>>> +    rc = dt_property_read_u32_array(imsic_node, "interrupts-extended",
>>>>> +                                    irq_range, ARRAY_SIZE(irq_range));
>>>>> +    if ( rc && (rc != -EOVERFLOW) )
>>>>> +        panic("%s: unable to find interrupt-extended in %s node\n",
>>>>> +               node->full_name, imsic_node->full_name);
>>>> Why exactly is EOVERFLOW tolerable here?
>>> QEMU generates two IMSIC device tree nodes: one for M-mode and one for 
>>> S-mode.
>>> For the hypervisor, we don’t really care about the M-mode IMSIC node — 
>>> we're only
>>> interested in the S-mode IMSIC node.
>>>
>>> The IMSIC node includes this information in the|"interrupts-extended"| 
>>> property,
>>> which has the following format:
>>>     interrupt-extended = {<interrupt-controller-phandle>, 
>>> <machine_mode>},...
>>> The number of such|<phandle, mode>| pairs depends on the number of CPUs the 
>>> platform has.
>>>
>>> For our purposes, to determine whether the IMSIC node corresponds to M-mode 
>>> or not, it’s sufficient to read only the first pair and check the mode like 
>>> this:
>>>
>>>     if ( irq_range[1] == IRQ_M_EXT )
>>>
>>> Thereby dt_property_read_u32_array() will return -EOVERFLOW in the case 
>>> when a platfrom
>>> has more then one CPU as we passed irq_range[2] as an argument but the 
>>> amount of values
>>> in "interrupts-extended" property will be (2 * CPUS_NUM).
>>>
>>> I can update the comment above dt_property_read_u32_array() for more 
>>> clearness.
>> Yet my question remains: Why would it be okay to ignore the remaining 
>> entries,
>> and hence accept -EOVERFLOW as kind-of-success?
> 
> Because for other entries the IMSIC mode will be the same and the difference 
> will be only in
> interrupt controller's phandle

And we can blindly take this for granted? Would you mind extending the
comment that's there to include this aspect?

Jan

> which we don't care about in this function and cares only about
> in imisic_init(), look at usage of imsic_get_parent_hartid().