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

[Ian Campbell <ian.campbell@citrix.com>]

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@huawei.com>
> >>> ---
> >>>   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.

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

Ian.