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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2881 bytes --]


On 4/17/25 5:30 PM, Jan Beulich wrote:
> 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?

I tried to compile dtc with different modes in interrupt-extends and compilation doesn't failed, so
it's not really granted.
Just to be sure, I'll check all items of interrupts-extend not just the first one.

~ Oleksii

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

[-- Attachment #2: Type: text/html, Size: 4158 bytes --]