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

[Julien Grall <julien.grall@linaro.org>]

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