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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

> 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;
     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;
 }
-- 
1.9.1

Frediano


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