Re: [PATCH v2] xen: arm: remove /xen-core-devices node from dom0 dtb

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

On 11/22/2013 03:07 PM, Ian Campbell wrote:
> The intention of adding this node to contain the GIC, timer and memory nodes
> (in 1c08d6004ea7) was to allow us to control the #address-cells and
>
> However in the case of the memory node the #*-cells are always taken from the
> root node (see ePAPR 3.1, "the following nodes shall be present at the root...
> memory node"). This caused breakage on the arndale platform.
>
> In addition it is not valid to just create sub-nodes like this. Unless they
> declare themselves as a bus then they will not necessarily be enumerated
> (although Linux currently does so in practice).
>
> Therefore:
>   - Move the memory node back to the top level.
>   - Insert the timer and primary gic nodes in the same location as the host
>     DTB, replacing the originals. Note that the nodes here may be marked as in
>     use by Xen and therefore the check must be before we discard nodes used by
>     Xen.
>   - Drop any secondary gics.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2:
> Discard secondary gics.
> Left parameter as "parent" for now since that is used everywhere.
> ---

[..]

> @@ -792,7 +749,15 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>           DT_MATCH_COMPATIBLE("arm,psci"),
>           DT_MATCH_PATH("/cpus"),
>           DT_MATCH_TYPE("memory"),
> +        { /* sentinel */ },
> +    };
> +    static const struct dt_device_match gic_matches[] __initconst =
> +    {
>           DT_MATCH_GIC,
> +        { /* sentinel */ },
> +    };
> +    static const struct dt_device_match timer_matches[] __initconst =
> +    {
>           DT_MATCH_TIMER,
>           { /* sentinel */ },
>       };
> @@ -806,11 +771,28 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>       DPRINT("handle %s\n", path);
>
>       /* Skip theses nodes and the sub-nodes */
> -    if ( dt_match_node(skip_matches, np ) ||
> -         platform_device_is_blacklisted(np) ||
> -         dt_device_used_by(np) == DOMID_XEN )
> +    if ( dt_match_node(skip_matches, np ) )

spurious space before ')'


> +    {
> +        DPRINT("  Skip it (matched)\n");
> +        return 0;
> +    }
> +    if ( platform_device_is_blacklisted(np) )
>       {
> -        DPRINT("  Skip it!\n");
> +        DPRINT("  Skip it (blacklisted)\n");
> +        return 0;
> +    }
> +
> +    /* Replace these nodes with our own. Note that the original may be
> +     * used_by DOMID_XEN so this check comes first. */
> +    if ( dt_match_node(gic_matches, np ) )

same here

> +        return make_gic_node(d, kinfo->fdt, np);
> +    if ( dt_match_node(timer_matches, np ) )

same here

> +        return make_timer_node(d, kinfo->fdt, np);
> +
> +    /* Skip nodes used by Xen */
> +    if ( dt_device_used_by(np) == DOMID_XEN )
> +    {
> +        DPRINT("  Skip it (used by Xen)\n");
>           return 0;
>       }
>

[..]

Except the minor coding style issue:

Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall