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

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

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


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 which we don't care about in this function and cares only about
in imisic_init(), look at usage of imsic_get_parent_hartid().

>
>>>> +    aplic.regs = ioremap(paddr, size);
>>>> +    if ( !aplic.regs )
>>>> +        panic("%s: unable to map\n", node->full_name);
>>>> +
>>>> +    /* Setup initial state APLIC interrupts */
>>>> +    aplic_init_hw_interrupts();
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct intc_hw_operations __ro_after_init aplic_ops = {
>>> const or __ro_after_init?
>> What’s wrong with using both?|const| ensures the variable can't be changed at compile time,
>> while|__ro_after_init| makes it read-only at runtime after initialization is complete.
> No, const makes it read-only at compile- and run-time.__ro_after_init,
> putting the item into a special section, makes it writable at init-time.
> Due to the const, the compiler wouldn't emit any writes. But we can
> also avoid stray writes by having the item live in .rodata.

Oh, right, `const` will add the variable to .rodata.
Then I think it is enough to have `const` as aplic_ops is going to be initialized once and
then only read will happen.

>
>> Probably,|__initconst| would be a better fit:
>>     static const struct intc_hw_operations __initconst aplic_ops = {
>>
>> Or even|__initconstrel|, since the|struct intc_hw_operations| contains pointers.
> Well, if this variable isn't accessed post-init, sure. That seems pretty
> unlikely though, considering it contains pointers to hook functions.

Sure, .init section is going to be freed after init-time.

~ Oleksii


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