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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 11/05/2014 03:46 PM, Frediano Ziglio wrote:
>> 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
> 
> This then should look much better.
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2f6bbd5..9b9e696 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -629,9 +629,8 @@ static int gicv2_make_dt_node(const struct domain *d,
>                                const struct dt_device_node *node, void *fdt)
>  {
>      const struct dt_device_node *gic = dt_interrupt_controller;
> -    const void *compatible = NULL;
> +    const void *compatible = NULL, *tmp;
>      u32 len;
> -    __be32 *new_cells, *tmp;

I would prefer if you keep tmp as __be32 *. It documents the real type
of the data. Also, for clarity, I would rename it to cells.

BTW, the commit title/message wasn't really clear. It make thinks you
are fixing the problem for everyone and not only the GICv2.

Regards,

-- 
Julien Grall

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