diff mbox

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

Message ID B944B469BF5302468AC6EB05E56CC70D17B3CE@lhreml509-mbb
State New
Headers show

Commit Message

Frediano Ziglio Nov. 5, 2014, 3:46 p.m. UTC 
> 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.

Comments

Julien Grall Nov. 5, 2014, 3:51 p.m. UTC | #1 
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 Nov. 5, 2014, 3:52 p.m. UTC | #2 
On 11/05/2014 03:51 PM, Julien Grall wrote:
>> 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.

s/cells/regs/.  Sorry
diff mbox

Patch

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;
     int res = 0;
 
     compatible = dt_get_property(gic, "compatible", &len);
@@ -664,18 +663,18 @@  static int gicv2_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
+    /* copy GICC and GICD regions */
+    tmp = dt_get_property(gic, "reg", &len);
+    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(dt_n_addr_cells(node) + dt_n_size_cells(node));
     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);
 
-    res = fdt_property(fdt, "reg", new_cells, len);
-    xfree(new_cells);
+    res = fdt_property(fdt, "reg", tmp, len);
 
     return res;
 }