Re: [PATCH v3 8/8] xen/arm: Handle translated addresses for hardware domains

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

On 11/05/2014 03:13 PM, Frediano Ziglio wrote:
> How does sound something like this (already tested, it's working).

The idea looks good to me. Few comments below.

Also, I'm wondering if we could create a generic function for this
purpose. The code of the GICv3 suffers of the same problem.

> Perhaps just to be paranoid a test on len after reading reg property
would be perfect.

The property/node has been validated in the GICv2 initialization.
Checking again here is not necessary.

> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2f6bbd5..2c4d097 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -632,7 +632,7 @@ static int gicv2_make_dt_node(const struct domain *d,
>      const void *compatible = NULL;
>      u32 len;
>      __be32 *new_cells, *tmp;
> -    int res = 0;
> +    int res = 0, na, ns;
>  
>      compatible = dt_get_property(gic, "compatible", &len);
>      if ( !compatible )
> @@ -664,15 +664,27 @@ static int gicv2_make_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
> +    /* copy GICC and GICD regions */
> +    na = dt_n_addr_cells(node);
> +    ns = dt_n_size_cells(node);
> +
> +    if ( na != dt_n_addr_cells(gic) || ns != dt_n_size_cells(gic) )
> +        return -FDT_ERR_XEN(EINVAL);

Not necessary, the caller of this function already check that gic (i.e
dt_interrupt_controller) == node.

If you really to be safe, I would add ASSERT(gic == node);

> +
> +    tmp = (__be32 *) dt_get_property(gic, "reg", &len);

The cast is not necessary.

> +    if ( !tmp )
> +    {
> +        dprintk(XENLOG_ERR, "Can't find reg property for the gic node\n");
> +        return -FDT_ERR_XEN(ENOENT);
> +    }
> +
> +    len = dt_cells_to_size(na + ns);
>      len *= 2; /* GIC has two memory regions: Distributor + CPU interface */
>      new_cells = xzalloc_bytes(len);
>      if ( new_cells == NULL )
>          return -FDT_ERR_XEN(ENOMEM);
>  
> -    tmp = new_cells;
> -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> +    memcpy(new_cells, tmp, len);

You don't need to copy the data in the temporary variable. You can
directly use fdt_property with the right len. Smth like:

fdt_property(fdt, "reg", tmp, len);

Regards,

-- 
Julien Grall