Re: [Xen-devel] [PATCH v8 2/4] xen/arm: Check for interrupt controller directly

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: Monday, March 09, 2015 4:09 PM
> 
> On Mon, 2015-03-09 at 12:55 +0200, Julien Grall wrote:
> >
> > On 05/03/2015 18:36, Ian Campbell wrote:
> > > On Tue, 2015-03-03 at 14:45 +0000, Julien Grall wrote:
> > >> Hello Frediano,
> > >>
> > >> On 03/03/15 11:19, Frediano Ziglio wrote:
> > >>> This check allow to detect mail interrupt controller even if it
> > >>> does
> > >>
> > >> main
> > >>
> > >>> not match one of the standard ones.
> > >>> This allow boards with non standard controllers to be handled
> > >>> correctly without having to manually edit the global list every
> time.
> > >>>
> > >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> > >>> ---
> > >>>   xen/arch/arm/domain_build.c | 2 +-
> > >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/xen/arch/arm/domain_build.c
> > >>> b/xen/arch/arm/domain_build.c index 9f1f59f..83951a3 100644
> > >>> --- a/xen/arch/arm/domain_build.c
> > >>> +++ b/xen/arch/arm/domain_build.c
> > >>> @@ -1069,7 +1069,7 @@ static int handle_node(struct domain *d,
> > >>> struct kernel_info *kinfo,
> > >>>
> > >>>       /* Replace these nodes with our own. Note that the original
> may be
> > >>>        * used_by DOMID_XEN so this check comes first. */
> > >>> -    if ( dt_match_node(gic_matches, node) )
> > >>> +    if ( node == dt_interrupt_controller ||
> > >>> + dt_match_node(gic_matches, node) )
> > >>>           return make_gic_node(d, kinfo->fdt, node);
> > >>
> > >> What about if the device tree exposes multiple GICs? By mistake we
> > >> will expose the secondaries GIC if they are not standard.
> > >
> > > Does the existing code here not insert a primary gic node into the
> > > dom0 tree for every gic node which find, that doesn't sound like it
> > > can be right!
> >
> > The current code doesn't insert any secondary gic (see the check in
> > make_gic_node) in the DT.
> 
> Ah, I missed that, yes that would avoid the issue for sure.
> 

I'm wondering if the final results can always work. I mean, a bare metal kernel 
will see all interrupt controllers while under Xen kernel receive only the 
primary one (virtualized). Now the question is "What are topology of secondary 
controllers?" I mean if a device is supposed so send some interrupt throw a 
secondary controller and nor kernel nor xen handle this device we lose the 
ability to use interrupts from this device?

> > With this version of the patch secondary gics was added to the DOM0
> DT
> 
> How? Does that same check in make_gic_node not prevent it?
> 
> > > Is the right pattern:
> > >      if ( node == dt_interrupt_controller )
> > >           return make_gic_node(d, kinfo->fdt, node);
> > >      else if ( device_get_class(node) == DEVICE_GIC )
> > >      {
> > >           DPRINT(" Secondary GIC, skip it\n");
> > >           return 0;/* Skip it */
> > >      }
> > > (incorporating the suggestion to match class from further down
> thread)?
> > >
> > > Anyway, I don't think what Frediano proposes in v9 of this series
> > > makes any of this worse, so I don't propose to block the series
> based on it.
> >
> > The solution on the v9 was the right one.
> 
> Good.
> 
> > I though I sent an email to review it but it looks like not :/
> 
> I don't recall seeing it.
> 

I remember something about. What this perhaps 
(http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg00396.html) ?

Frediano

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel