Re: [Xen-devel] [PATCH 4/8] xen/arm: Warn when a device tree path will be re-used by Xen
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
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
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
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
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
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