diff mbox

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

Message ID B944B469BF5302468AC6EB05E56CC70D17B396@lhreml509-mbb
State New
Headers show

Commit Message

Frediano Ziglio Nov. 5, 2014, 3:13 p.m. UTC 
> Hi Frediano,
> 
> On 11/05/2014 09:41 AM, Frediano Ziglio wrote:
> > Translated address could have an offset applied to them.
> > Replicate same value for device node to avoid improper address
> > computation in the OS.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > ---
> >  xen/arch/arm/gic-v2.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > 2f6bbd5..271074d 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -67,8 +67,10 @@
> >  /* Global state */
> >  static struct {
> >      paddr_t dbase;            /* Address of distributor registers */
> > +    paddr_t dbase_raw;        /* Untranslated address of distributor
> registers */
> >      void __iomem * map_dbase; /* IO mapped Address of distributor
> registers */
> >      paddr_t cbase;            /* Address of CPU interface registers
> */
> > +    paddr_t cbase_raw;        /* Untranslated address of CPU
> interface registers */
> >      void __iomem * map_cbase[2]; /* IO mapped Address of CPU
> interface registers */
> >      paddr_t hbase;            /* Address of virtual interface
> registers */
> >      void __iomem * map_hbase; /* IO Address of virtual interface
> > registers */ @@ -671,8 +673,17 @@ static int gicv2_make_dt_node(const
> struct domain *d,
> >          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);
> > +
> > +    if ( is_hardware_domain(d) )
> > +    {
> 
> This check is pointless, as said on an earlier version of this series,
> gicv2_make_dt_node is only used to create DOM0 (hardware domain) device
> tree.
> 
> Regards,
> 
> --
> Julien Grall

How does sound something like this (already tested, it's working). Perhaps just to be paranoid a test on len after reading reg property would be perfect.

Comments

Julien Grall Nov. 5, 2014, 3:28 p.m. UTC | #1 
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,
diff mbox

Patch

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);
+
+    tmp = (__be32 *) 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(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);
 
     res = fdt_property(fdt, "reg", new_cells, len);
     xfree(new_cells);