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

[Julien Grall <julien.grall@linaro.org>]

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.

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

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.

>          if ( res )
>              return res;
>      }
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 073ad33..42af10b 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -636,10 +636,6 @@ static int hip04gic_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 )
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 20cdbc9..8d1a07d 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -622,10 +622,6 @@ static int gicv2_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 )
> 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?

Regards,

-- 
Julien Grall