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

[Julien Grall]

On 25/09/15 17:10, Ian Campbell wrote:
> 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.

I forgot to answer to this bit. Printing the first compatible node will
be very easy, the others will require a bit more work (maybe we could
re-use a bit of dt_device_is_compatible?).

I think a generic function to dump DT node in Xen would be very useful.
I can give a look when I have time as a follow-up.

Regards,


-- 
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Julien Grall]

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Julien Grall]

On 28/09/15 16:55, Ian Campbell wrote:
> On Mon, 2015-09-28 at 16:44 +0100, Julien Grall wrote:
>>> 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
> 
> "unconditionally using" and "paths", and probably s/some/certain/.
> 
> 
>> specific node (for instance /psci, /memory and /hypervisor).
>>
>> Print a warning message on the console to let know user the user if we
> 
> "to let the user know"
> 
>> re-use one of this node.
> 
> "these nodes"
> 
>> Note that the content of most those nodes are very common and they
> 
> "most of those is"
> 
>> 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 don't think they are "buggy", perhaps more like "unusual".

I will use "unusual".

Regards,

-- 
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

On Mon, 2015-09-28 at 16:44 +0100, Julien Grall wrote:
> > 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

"unconditionally using" and "paths", and probably s/some/certain/.


> specific node (for instance /psci, /memory and /hypervisor).
> 
> Print a warning message on the console to let know user the user if we

"to let the user know"

> re-use one of this node.

"these nodes"

> Note that the content of most those nodes are very common and they

"most of those is"

> 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 don't think they are "buggy", perhaps more like "unusual".

Ian

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

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.

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

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?

> +/*
> + * 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?


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Julien Grall]

Xen is using unconditionnally some device tree path to create DOM0
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.

Signed-off-by: Julien Grall 
---
 xen/arch/arm/domain_build.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 651d75e..2670431 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1205,6 +1205,13 @@ static int handle_node(struct domain *d, struct 
kernel_info *kinfo,
 DT_MATCH_TIMER,
 { /* sentinel */ },
 };
+static const struct dt_device_match reserved_matches[] __initconst =
+{
+DT_MATCH_PATH("/psci"),
+DT_MATCH_PATH("/memory"),
+DT_MATCH_PATH("/hypervisor"),
+{ /* sentinel */ },
+};
 struct dt_device_node *child;
 int res;
 const char *name;
@@ -1252,6 +1259,14 @@ static int handle_node(struct domain *d, struct 
kernel_info *kinfo,
 return 0;
 }
 
+/*
+ * Xen is using some path for its own purpose. Warn if a node
+ * 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",
+   path);
+
 res = handle_device(d, node);
 if ( res)
 return res;
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel