Re: [PATCH 4/8] xen/arm: Warn when a device tree path will be re-used by Xen

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 25/09/15 17:10, Ian Campbell wrote:
> On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
>> Xen is using unconditionnally some device tree path to create DOM0
> 
> "unconditionally"
> 
>> specific node (for instance /psci, /memory and /hypervisor).
>>
>> Rather than blindly add new nodes with the same, print a warning message
>> on the console to let know the user that something may go wrong.
> 
> So it seems to me that /psci and /memory should be pretty common and that
> warning when we deliberately replace them with our own contents seems
> wrong.
>
> I think the bit which the commit message misses out is that we already
> filter out psci and such by compatible string before this point, so this
> warning is only for the case where there is a /psci which is not compatible
> "arm,psci" or "arm,psci-0.2" etc, i.e. it is something strange and special.

Correct. And it's the same for the node /memory. We are removing the
node only when the type is "memory".


> And that case does indeed seem worth warning about but the commit message
> needs to be clearer about the circumstances of the logging.

What about:

"Xen is using unconditionally some device tree path to create DOM0
specific node (for instance /psci, /memory and /hypervisor).

Print a warning message on the console to let know user the user if we
re-use one of this node.

Note that the content of most those nodes are very common and they
should have already been skipped via the compatible string or type
string. This warning is here to catch buggy device-tree and compatible
string that we may not yet support in Xen."

> I wonder if it would be worth printing the compatible strings of the node
> we are clobbering? There's a good chance they will be e.g. "arm,psci-0.3"
> (with no backwards compat) or something and knowing that might be useful.
> But maybe it's too much code to worry about.
> 
> BTW, how did you trip over this?

No specific issue. I wrote this patch because I was thinking to move the
interrupt-controller node in /. Although, I gave up on this idea and I
though this patch would still be useful to catch early buggy DT.

> 
>> +    /*
>> +     * Xen is using some path for its own purpose. Warn if a node
> 
> "paths" and "purposes".
> 
>> +     * already exists with the same path.
>> +     */
>> +    if ( dt_match_node(reserved_matches, node) )
>> +        printk(XENLOG_WARNING "WARNING: Path %s is reserved, skip the
>> node\n",
> 
> Rather than skipping we are actually replacing it with our version. Maybe
> say that?

I would rather keep "skipping" because we may decide to not use this
path based on DOM0 configuration.

Though I can rework the message with:

"WARNING: Patch %s is reserved, skip the node as we may re-use the path.\n"

Regards,

-- 
Julien Grall