[PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[Andre Przywara]

At the moment we re-generate the Dom0 GICv3 DT node, by creating the
"reg" property from scratch using our previously parsed and
translated(!) host addresses. However we then write the *absolute*
addresses into the new node, not considering possible "range" mappings
in any of the GIC's parent nodes. So whenever one of the parents has a
non-empty ranges property, Dom0 will wrongly translate the addresses.
Properly incorporating the ranges properties sounds tedious, so let's
just copy the first part of the reg property instead (as we do for GICv2),
since the addresses for Dom0 are identical to those from the hardware.

The mainline kernel DT for the Espressobin board with an Marvell 3720 SoC
has the GIC in such an translated bus, so this patch allows this board
to boot properly (after adding support for the SoC's UART).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/gic-v3.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index a0d290b55c..6b17abd0a1 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1147,10 +1147,9 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
                                     const struct dt_device_node *gic,
                                     void *fdt)
 {
-    const void *compatible = NULL;
-    uint32_t len;
-    __be32 *new_cells, *tmp;
-    int i, res = 0;
+    const void *compatible, *hw_reg;
+    uint32_t len, new_len;
+    int res;
 
     compatible = dt_get_property(gic, "compatible", &len);
     if ( !compatible )
@@ -1173,27 +1172,21 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
+    new_len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
     /*
      * GIC has two memory regions: Distributor + rdist regions
      * CPU interface and virtual cpu interfaces accessesed as System registers
      * So cells are created only for Distributor and rdist regions
      */
-    len = len * (d->arch.vgic.nr_regions + 1);
-    new_cells = xzalloc_bytes(len);
-    if ( new_cells == NULL )
-        return -FDT_ERR_XEN(ENOMEM);
-
-    tmp = new_cells;
-
-    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
+    new_len = new_len * (d->arch.vgic.nr_regions + 1);
 
-    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
-        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
-                     d->arch.vgic.rdist_regions[i].size);
+    hw_reg = dt_get_property(gic, "reg", &len);
+    if ( !hw_reg )
+        return -FDT_ERR_XEN(ENOENT);
+    if ( new_len > len )
+	return -FDT_ERR_XEN(ERANGE);
 
-    res = fdt_property(fdt, "reg", new_cells, len);
-    xfree(new_cells);
+    res = fdt_property(fdt, "reg", hw_reg, new_len);
     if ( res )
         return res;
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[Amit Tomer]

Hi,

Thanks for the patch.

On Tue, Jan 30, 2018 at 3:05 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> At the moment we re-generate the Dom0 GICv3 DT node, by creating the
> "reg" property from scratch using our previously parsed and
> translated(!) host addresses. However we then write the *absolute*
> addresses into the new node, not considering possible "range" mappings
> in any of the GIC's parent nodes. So whenever one of the parents has a
> non-empty ranges property, Dom0 will wrongly translate the addresses.
> Properly incorporating the ranges properties sounds tedious, so let's
> just copy the first part of the reg property instead (as we do for GICv2),
> since the addresses for Dom0 are identical to those from the hardware.
>
> The mainline kernel DT for the Espressobin board with an Marvell 3720 SoC
> has the GIC in such an translated bus, so this patch allows this board
> to boot properly (after adding support for the SoC's UART).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-v3.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index a0d290b55c..6b17abd0a1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1147,10 +1147,9 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>                                      const struct dt_device_node *gic,
>                                      void *fdt)
>  {
> -    const void *compatible = NULL;
> -    uint32_t len;
> -    __be32 *new_cells, *tmp;
> -    int i, res = 0;
> +    const void *compatible, *hw_reg;
> +    uint32_t len, new_len;
> +    int res;
>
>      compatible = dt_get_property(gic, "compatible", &len);
>      if ( !compatible )
> @@ -1173,27 +1172,21 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>
> -    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
> +    new_len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>      /*
>       * GIC has two memory regions: Distributor + rdist regions
>       * CPU interface and virtual cpu interfaces accessesed as System registers
>       * So cells are created only for Distributor and rdist regions
>       */
> -    len = len * (d->arch.vgic.nr_regions + 1);
> -    new_cells = xzalloc_bytes(len);
> -    if ( new_cells == NULL )
> -        return -FDT_ERR_XEN(ENOMEM);
> -
> -    tmp = new_cells;
> -
> -    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
>
> -    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> -        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
> -                     d->arch.vgic.rdist_regions[i].size);
> +    hw_reg = dt_get_property(gic, "reg", &len);
> +    if ( !hw_reg )
> +        return -FDT_ERR_XEN(ENOENT);
> +    if ( new_len > len )
> +       return -FDT_ERR_XEN(ERANGE);
>
> -    res = fdt_property(fdt, "reg", new_cells, len);
> -    xfree(new_cells);
> +    res = fdt_property(fdt, "reg", hw_reg, new_len);
>      if ( res )
>          return res;
>
> --
> 2.14.1
>

Tested on Espresso bin:

Tested-by: Amit Singh Tomar <amittomer25@gmail.com>

Thanks
-Amit

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[Julien Grall]

Hi Andre,

On 30/01/18 09:35, Andre Przywara wrote:
> @@ -1173,27 +1172,21 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       if ( res )
>           return res;
>   
> -    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
> +    new_len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>       /*
>        * GIC has two memory regions: Distributor + rdist regions
>        * CPU interface and virtual cpu interfaces accessesed as System registers
>        * So cells are created only for Distributor and rdist regions
>        */
> -    len = len * (d->arch.vgic.nr_regions + 1);
> -    new_cells = xzalloc_bytes(len);
> -    if ( new_cells == NULL )
> -        return -FDT_ERR_XEN(ENOMEM);
> -
> -    tmp = new_cells;
> -
> -    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
>   
> -    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> -        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
> -                     d->arch.vgic.rdist_regions[i].size);
> +    hw_reg = dt_get_property(gic, "reg", &len);
> +    if ( !hw_reg )
> +        return -FDT_ERR_XEN(ENOENT);
> +    if ( new_len > len )
> +	return -FDT_ERR_XEN(ERANGE);

The indentation looks wrong here.

With that fixed:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[Stefano Stabellini]

On Tue, 30 Jan 2018, Andre Przywara wrote:
> At the moment we re-generate the Dom0 GICv3 DT node, by creating the
> "reg" property from scratch using our previously parsed and
> translated(!) host addresses. However we then write the *absolute*
> addresses into the new node, not considering possible "range" mappings
> in any of the GIC's parent nodes. So whenever one of the parents has a
> non-empty ranges property, Dom0 will wrongly translate the addresses.
> Properly incorporating the ranges properties sounds tedious, so let's
> just copy the first part of the reg property instead (as we do for GICv2),
> since the addresses for Dom0 are identical to those from the hardware.
> 
> The mainline kernel DT for the Espressobin board with an Marvell 3720 SoC
> has the GIC in such an translated bus, so this patch allows this board
> to boot properly (after adding support for the SoC's UART).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

There is one code style issue below, but I'll fix it on commit.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/arch/arm/gic-v3.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index a0d290b55c..6b17abd0a1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1147,10 +1147,9 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>                                      const struct dt_device_node *gic,
>                                      void *fdt)
>  {
> -    const void *compatible = NULL;
> -    uint32_t len;
> -    __be32 *new_cells, *tmp;
> -    int i, res = 0;
> +    const void *compatible, *hw_reg;
> +    uint32_t len, new_len;
> +    int res;
>  
>      compatible = dt_get_property(gic, "compatible", &len);
>      if ( !compatible )
> @@ -1173,27 +1172,21 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
> +    new_len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>      /*
>       * GIC has two memory regions: Distributor + rdist regions
>       * CPU interface and virtual cpu interfaces accessesed as System registers
>       * So cells are created only for Distributor and rdist regions
>       */
> -    len = len * (d->arch.vgic.nr_regions + 1);
> -    new_cells = xzalloc_bytes(len);
> -    if ( new_cells == NULL )
> -        return -FDT_ERR_XEN(ENOMEM);
> -
> -    tmp = new_cells;
> -
> -    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
>  
> -    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> -        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
> -                     d->arch.vgic.rdist_regions[i].size);
> +    hw_reg = dt_get_property(gic, "reg", &len);
> +    if ( !hw_reg )
> +        return -FDT_ERR_XEN(ENOENT);
> +    if ( new_len > len )
> +	return -FDT_ERR_XEN(ERANGE);
>  
> -    res = fdt_property(fdt, "reg", new_cells, len);
> -    xfree(new_cells);
> +    res = fdt_property(fdt, "reg", hw_reg, new_len);
>      if ( res )
>          return res;
>  
> -- 
> 2.14.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[André Przywara]

On 30/01/18 19:04, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Andre Przywara wrote:
>> At the moment we re-generate the Dom0 GICv3 DT node, by creating the
>> "reg" property from scratch using our previously parsed and
>> translated(!) host addresses. However we then write the *absolute*
>> addresses into the new node, not considering possible "range" mappings
>> in any of the GIC's parent nodes. So whenever one of the parents has a
>> non-empty ranges property, Dom0 will wrongly translate the addresses.
>> Properly incorporating the ranges properties sounds tedious, so let's
>> just copy the first part of the reg property instead (as we do for GICv2),
>> since the addresses for Dom0 are identical to those from the hardware.
>>
>> The mainline kernel DT for the Espressobin board with an Marvell 3720 SoC
>> has the GIC in such an translated bus, so this patch allows this board
>> to boot properly (after adding support for the SoC's UART).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> There is one code style issue below, but I'll fix it on commit.

Argh, indeed, Julien found this as well.
Thanks for the smooth handling!

Cheers,
Andre.

> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
>> ---
>>  xen/arch/arm/gic-v3.c | 29 +++++++++++------------------
>>  1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a0d290b55c..6b17abd0a1 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1147,10 +1147,9 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>>                                      const struct dt_device_node *gic,
>>                                      void *fdt)
>>  {
>> -    const void *compatible = NULL;
>> -    uint32_t len;
>> -    __be32 *new_cells, *tmp;
>> -    int i, res = 0;
>> +    const void *compatible, *hw_reg;
>> +    uint32_t len, new_len;
>> +    int res;
>>  
>>      compatible = dt_get_property(gic, "compatible", &len);
>>      if ( !compatible )
>> @@ -1173,27 +1172,21 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>>      if ( res )
>>          return res;
>>  
>> -    len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>> +    new_len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>>      /*
>>       * GIC has two memory regions: Distributor + rdist regions
>>       * CPU interface and virtual cpu interfaces accessesed as System registers
>>       * So cells are created only for Distributor and rdist regions
>>       */
>> -    len = len * (d->arch.vgic.nr_regions + 1);
>> -    new_cells = xzalloc_bytes(len);
>> -    if ( new_cells == NULL )
>> -        return -FDT_ERR_XEN(ENOMEM);
>> -
>> -    tmp = new_cells;
>> -
>> -    dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
>> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
>>  
>> -    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
>> -        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
>> -                     d->arch.vgic.rdist_regions[i].size);
>> +    hw_reg = dt_get_property(gic, "reg", &len);
>> +    if ( !hw_reg )
>> +        return -FDT_ERR_XEN(ENOENT);
>> +    if ( new_len > len )
>> +	return -FDT_ERR_XEN(ERANGE);
>>  
>> -    res = fdt_property(fdt, "reg", new_cells, len);
>> -    xfree(new_cells);
>> +    res = fdt_property(fdt, "reg", hw_reg, new_len);
>>      if ( res )
>>          return res;
>>  
>> -- 
>> 2.14.1
>>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel