Re: [PATCH v2 2/3] xen: arm: propagate gic's #interrupt-cells property to dom0.

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

On Mon, 2015-03-16 at 16:01 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 12/03/15 17:17, Ian Campbell wrote:
> > This is similar to 816f5bb1f074 "xen: arm: propagate gic's
> > should propagate (rather than invent our own value) since this value
> > is used to size fields within other properties within the tree.
> > I'm not sure why I didn't do this as part of 816f5bb1f074. I think
> > probably just because #interrupt-cells must always be 3 for a GIC
> > whereas #address-cells can legitimately differ. Regardless, I think we
> > might as well do this in common code.
> 
> Hmmm... We are creating some interrupt ourself assuming the number of
> interrupt cells is 3. So it makes sense to hard-code (not really invent)
> the value.

I'll move the addition to common code but leave it as hard coded then.

> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/domain_build.c |   18 +++++++++++++-----
> >  xen/arch/arm/gic-hip04.c    |    4 ----
> >  xen/arch/arm/gic-v2.c       |    4 ----
> >  xen/arch/arm/gic-v3.c       |    4 ----
> >  4 files changed, 13 insertions(+), 17 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index ab4ad65..2a2fc2b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -784,8 +784,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
> >  {
> >      const struct dt_device_node *gic = dt_interrupt_controller;
> >      int res = 0;
> > -    const void *addrcells;
> > -    u32 addrcells_len;
> > +    const void *cells;
> > +    u32 cells_len;
> >  
> >      /*
> >       * Xen currently supports only a single GIC. Discard any secondary
> > @@ -815,10 +815,18 @@ static int make_gic_node(const struct domain *d, void *fdt,
> >              return res;
> >      }
> >  
> > -    addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
> > -    if ( addrcells )
> > +    cells = dt_get_property(gic, "#address-cells", &cells_len);
> > +    if ( cells )
> >      {
> > -        res = fdt_property(fdt, "#address-cells", addrcells, addrcells_len);
> > +        res = fdt_property(fdt, "#address-cells", cells, cells_len);
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    cells = dt_get_property(gic, "#interrupt-cells", &cells_len);
> > +    if ( cells )
> > +    {
> > +        res = fdt_property(fdt, "#interrupt-cells", cells, cells_len);
> 
> The #interrupt-cells as to be present at any time for the GIC. So I
> don't think it's worth to check if it presents. Maybe an ASSERT would be
> enough?

With the change discussed above it becomes moot.

> Also, I would check somewhere that the value is effectively 3 otherwise
> we are in trouble for the timer/evtchn interrupt creation. Though, it
> was there before too.

Probably somewhere should but I'm not sure where.

> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index ab80670..528500a 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1102,10 +1102,6 @@ static int gicv3_make_dt_node(const struct domain *d,
> >      if ( res )
> >          return res;
> >  
> > -    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> > -    if ( res )
> > -        return res;
> > -
> >      res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> >      if ( res )
> >          return res;
> > 
> 
> While you move #interrupt-cells to common code. Could you move
> interrupt-controller too?

I suppose I may as well.

Ian.