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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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().
> >       
>