[PATCH] xen/dts: Don't translate invalid address

[PATCH] xen/dts: Don't translate invalid address

[Julien Grall]

ePAR specifies that if the property "ranges" doesn't exist in a bus node:

"it is assumed that no mapping exists between children of node and the parent
address space".

Modify dt_number_of_address to check if the list of ranges are valid. Return
0 (ie there is zero range) if the list is not valid.

This patch has been tested on the Arndale where the bug can occur with the
'/hdmi' node.

Reported-by: <tsahee@gmx.com>
Signed-off-by: Julien Grall <julien.grall@linaro.org>

---

This patch is a bug fix for Xen 4.4. Without it, Xen can't boot on the Arndale
because it's unable to translate a wrong address.
---
 xen/common/device_tree.c |   31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 84e709d..9b9a348 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -93,7 +93,7 @@ struct dt_bus
 {
     const char *name;
     const char *addresses;
-    int (*match)(const struct dt_device_node *parent);
+    bool_t (*match)(const struct dt_device_node *parent);
     void (*count_cells)(const struct dt_device_node *child,
                         int *addrc, int *sizec);
     u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
@@ -793,6 +793,18 @@ int dt_n_size_cells(const struct dt_device_node *np)
 /*
  * Default translator (generic bus)
  */
+static bool_t dt_bus_default_match(const struct dt_device_node *parent)
+{
+    /* Root node doesn't have "ranges" property */
+    if ( parent->parent == NULL )
+        return 1;
+
+    /* The default bus is only used when the "ranges" property exists.
+     * Otherwise we can't translate the address
+     */
+    return (dt_get_property(parent, "ranges", NULL) != NULL);
+}
+
 static void dt_bus_default_count_cells(const struct dt_device_node *dev,
                                 int *addrc, int *sizec)
 {
@@ -819,7 +831,7 @@ static u64 dt_bus_default_map(__be32 *addr, const __be32 *range,
      * If the number of address cells is larger than 2 we assume the
      * mapping doesn't specify a physical address. Rather, the address
      * specifies an identifier that must match exactly.
-     */
+      */
     if ( na > 2 && memcmp(range, addr, na * 4) != 0 )
         return DT_BAD_ADDR;
 
@@ -856,7 +868,7 @@ static const struct dt_bus dt_busses[] =
     {
         .name = "default",
         .addresses = "reg",
-        .match = NULL,
+        .match = dt_bus_default_match,
         .count_cells = dt_bus_default_count_cells,
         .map = dt_bus_default_map,
         .translate = dt_bus_default_translate,
@@ -871,7 +883,6 @@ static const struct dt_bus *dt_match_bus(const struct dt_device_node *np)
     for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ )
         if ( !dt_busses[i].match || dt_busses[i].match(np) )
             return &dt_busses[i];
-    BUG();
 
     return NULL;
 }
@@ -890,7 +901,10 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev,
     parent = dt_get_parent(dev);
     if ( parent == NULL )
         return NULL;
+
     bus = dt_match_bus(parent);
+    if ( !bus )
+        return NULL;
     bus->count_cells(dev, &na, &ns);
 
     if ( !DT_CHECK_ADDR_COUNT(na) )
@@ -994,6 +1008,8 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
     if ( parent == NULL )
         goto bail;
     bus = dt_match_bus(parent);
+    if ( !bus )
+        goto bail;
 
     /* Count address cells & copy address locally */
     bus->count_cells(dev, &na, &ns);
@@ -1026,6 +1042,11 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
 
         /* Get new parent bus and counts */
         pbus = dt_match_bus(parent);
+        if ( pbus == NULL )
+        {
+            dt_printk("DT: %s is not a valid bus\n", parent->full_name);
+            break;
+        }
         pbus->count_cells(dev, &pna, &pns);
         if ( !DT_CHECK_COUNTS(pna, pns) )
         {
@@ -1164,6 +1185,8 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
         return 0;
 
     bus = dt_match_bus(parent);
+    if ( !bus )
+        return 0;
     bus->count_cells(dev, &na, &ns);
 
     if ( !DT_CHECK_COUNTS(na, ns) )
-- 
1.7.10.4

Re: [PATCH] xen/dts: Don't translate invalid address

[Julien Grall]

Hello all,

Any comments on this patch?

Sincerely yours,

On 12/13/2013 02:31 AM, Julien Grall wrote:
> ePAR specifies that if the property "ranges" doesn't exist in a bus node:
>
> "it is assumed that no mapping exists between children of node and the parent
> address space".
>
> Modify dt_number_of_address to check if the list of ranges are valid. Return
> 0 (ie there is zero range) if the list is not valid.
>
> This patch has been tested on the Arndale where the bug can occur with the
> '/hdmi' node.
>
> Reported-by: <tsahee@gmx.com>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> ---
>
> This patch is a bug fix for Xen 4.4. Without it, Xen can't boot on the Arndale
> because it's unable to translate a wrong address.
> ---
>   xen/common/device_tree.c |   31 +++++++++++++++++++++++++++----
>   1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 84e709d..9b9a348 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -93,7 +93,7 @@ struct dt_bus
>   {
>       const char *name;
>       const char *addresses;
> -    int (*match)(const struct dt_device_node *parent);
> +    bool_t (*match)(const struct dt_device_node *parent);
>       void (*count_cells)(const struct dt_device_node *child,
>                           int *addrc, int *sizec);
>       u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
> @@ -793,6 +793,18 @@ int dt_n_size_cells(const struct dt_device_node *np)
>   /*
>    * Default translator (generic bus)
>    */
> +static bool_t dt_bus_default_match(const struct dt_device_node *parent)
> +{
> +    /* Root node doesn't have "ranges" property */
> +    if ( parent->parent == NULL )
> +        return 1;
> +
> +    /* The default bus is only used when the "ranges" property exists.
> +     * Otherwise we can't translate the address
> +     */
> +    return (dt_get_property(parent, "ranges", NULL) != NULL);
> +}
> +
>   static void dt_bus_default_count_cells(const struct dt_device_node *dev,
>                                   int *addrc, int *sizec)
>   {
> @@ -819,7 +831,7 @@ static u64 dt_bus_default_map(__be32 *addr, const __be32 *range,
>        * If the number of address cells is larger than 2 we assume the
>        * mapping doesn't specify a physical address. Rather, the address
>        * specifies an identifier that must match exactly.
> -     */
> +      */
>       if ( na > 2 && memcmp(range, addr, na * 4) != 0 )
>           return DT_BAD_ADDR;
>
> @@ -856,7 +868,7 @@ static const struct dt_bus dt_busses[] =
>       {
>           .name = "default",
>           .addresses = "reg",
> -        .match = NULL,
> +        .match = dt_bus_default_match,
>           .count_cells = dt_bus_default_count_cells,
>           .map = dt_bus_default_map,
>           .translate = dt_bus_default_translate,
> @@ -871,7 +883,6 @@ static const struct dt_bus *dt_match_bus(const struct dt_device_node *np)
>       for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ )
>           if ( !dt_busses[i].match || dt_busses[i].match(np) )
>               return &dt_busses[i];
> -    BUG();
>
>       return NULL;
>   }
> @@ -890,7 +901,10 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev,
>       parent = dt_get_parent(dev);
>       if ( parent == NULL )
>           return NULL;
> +
>       bus = dt_match_bus(parent);
> +    if ( !bus )
> +        return NULL;
>       bus->count_cells(dev, &na, &ns);
>
>       if ( !DT_CHECK_ADDR_COUNT(na) )
> @@ -994,6 +1008,8 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
>       if ( parent == NULL )
>           goto bail;
>       bus = dt_match_bus(parent);
> +    if ( !bus )
> +        goto bail;
>
>       /* Count address cells & copy address locally */
>       bus->count_cells(dev, &na, &ns);
> @@ -1026,6 +1042,11 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
>
>           /* Get new parent bus and counts */
>           pbus = dt_match_bus(parent);
> +        if ( pbus == NULL )
> +        {
> +            dt_printk("DT: %s is not a valid bus\n", parent->full_name);
> +            break;
> +        }
>           pbus->count_cells(dev, &pna, &pns);
>           if ( !DT_CHECK_COUNTS(pna, pns) )
>           {
> @@ -1164,6 +1185,8 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
>           return 0;
>
>       bus = dt_match_bus(parent);
> +    if ( !bus )
> +        return 0;
>       bus->count_cells(dev, &na, &ns);
>
>       if ( !DT_CHECK_COUNTS(na, ns) )
>

-- 
Julien Grall

Re: [PATCH] xen/dts: Don't translate invalid address

[Ian Campbell]

On Fri, 2013-12-13 at 02:31 +0000, Julien Grall wrote:
> ePAR specifies that if the property "ranges" doesn't exist in a bus node:
> 
> "it is assumed that no mapping exists between children of node and the parent
> address space".
> 
> Modify dt_number_of_address to check if the list of ranges are valid. Return
> 0 (ie there is zero range) if the list is not valid.
> 
> This patch has been tested on the Arndale where the bug can occur with the
> '/hdmi' node.
> 
> Reported-by: <tsahee@gmx.com>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
> 
> This patch is a bug fix for Xen 4.4. Without it, Xen can't boot on the Arndale
> because it's unable to translate a wrong address.
> ---
>  xen/common/device_tree.c |   31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 84e709d..9b9a348 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -93,7 +93,7 @@ struct dt_bus
>  {
>      const char *name;
>      const char *addresses;
> -    int (*match)(const struct dt_device_node *parent);
> +    bool_t (*match)(const struct dt_device_node *parent);
>      void (*count_cells)(const struct dt_device_node *child,
>                          int *addrc, int *sizec);
>      u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
> @@ -793,6 +793,18 @@ int dt_n_size_cells(const struct dt_device_node *np)
>  /*
>   * Default translator (generic bus)
>   */
> +static bool_t dt_bus_default_match(const struct dt_device_node *parent)
> +{
> +    /* Root node doesn't have "ranges" property */
> +    if ( parent->parent == NULL )

Calling the parameter "parent" led to me confusedly wondering why it was
the grandparent which mattered. I suppose "parent" in this sense means
the "parent bus" as opposed to parent node? Or does it just fall out of
the fact that in the caller it is the parent which is passed through?

Can we call it something else, like "bus" or "node" or something else
appropriate?

> +        return 1;
> +
> +    /* The default bus is only used when the "ranges" property exists.
> +     * Otherwise we can't translate the address
> +     */
> +    return (dt_get_property(parent, "ranges", NULL) != NULL);
> +}
> +
>  static void dt_bus_default_count_cells(const struct dt_device_node *dev,
>                                  int *addrc, int *sizec)
>  {
> @@ -819,7 +831,7 @@ static u64 dt_bus_default_map(__be32 *addr, const __be32 *range,
>       * If the number of address cells is larger than 2 we assume the
>       * mapping doesn't specify a physical address. Rather, the address
>       * specifies an identifier that must match exactly.
> -     */
> +      */

Spurious w/s change.

Other that those two changes the patch looks good.

Ian.

Re: [PATCH] xen/dts: Don't translate invalid address

[Julien Grall]

On 01/06/2014 03:24 PM, Ian Campbell wrote:
> On Fri, 2013-12-13 at 02:31 +0000, Julien Grall wrote:
>> ePAR specifies that if the property "ranges" doesn't exist in a bus node:
>>
>> "it is assumed that no mapping exists between children of node and the parent
>> address space".
>>
>> Modify dt_number_of_address to check if the list of ranges are valid. Return
>> 0 (ie there is zero range) if the list is not valid.
>>
>> This patch has been tested on the Arndale where the bug can occur with the
>> '/hdmi' node.
>>
>> Reported-by: <tsahee@gmx.com>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>
>> This patch is a bug fix for Xen 4.4. Without it, Xen can't boot on the Arndale
>> because it's unable to translate a wrong address.
>> ---
>>   xen/common/device_tree.c |   31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 84e709d..9b9a348 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -93,7 +93,7 @@ struct dt_bus
>>   {
>>       const char *name;
>>       const char *addresses;
>> -    int (*match)(const struct dt_device_node *parent);
>> +    bool_t (*match)(const struct dt_device_node *parent);
>>       void (*count_cells)(const struct dt_device_node *child,
>>                           int *addrc, int *sizec);
>>       u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
>> @@ -793,6 +793,18 @@ int dt_n_size_cells(const struct dt_device_node *np)
>>   /*
>>    * Default translator (generic bus)
>>    */
>> +static bool_t dt_bus_default_match(const struct dt_device_node *parent)
>> +{
>> +    /* Root node doesn't have "ranges" property */
>> +    if ( parent->parent == NULL )
>
> Calling the parameter "parent" led to me confusedly wondering why it was
> the grandparent which mattered. I suppose "parent" in this sense means
> the "parent bus" as opposed to parent node? Or does it just fall out of
> the fact that in the caller it is the parent which is passed through?
>
> Can we call it something else, like "bus" or "node" or something else
> appropriate?


Right, the name is confusing. It took me few minutes, even if I wrote 
the code, to understand it :). I blindly copied the code from Linux 
of_bus structure.

The best name would be "node", because the function is trying to find if 
the parameters is a bus or not.

>> +        return 1;
>> +
>> +    /* The default bus is only used when the "ranges" property exists.
>> +     * Otherwise we can't translate the address
>> +     */
>> +    return (dt_get_property(parent, "ranges", NULL) != NULL);
>> +}
>> +
>>   static void dt_bus_default_count_cells(const struct dt_device_node *dev,
>>                                   int *addrc, int *sizec)
>>   {
>> @@ -819,7 +831,7 @@ static u64 dt_bus_default_map(__be32 *addr, const __be32 *range,
>>        * If the number of address cells is larger than 2 we assume the
>>        * mapping doesn't specify a physical address. Rather, the address
>>        * specifies an identifier that must match exactly.
>> -     */
>> +      */
>
> Spurious w/s change.
>
> Other that those two changes the patch looks good.

I will fix it.



-- 
Julien Grall