Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Chris (Christopher) Brand]

[-- Attachment #1.1: Type: text/plain, Size: 2920 bytes --]

I've been trying to figure out why Xen only reports 2GB on my ARM platform that actually has 3GB, and I think I've found a bug, but I'm not familiar enough with the Xen code to fix it.

The relevant parts of my dts are:
/dts-v1/;

/ {
     model = "Broadcom STB (7445d0)";
     compatible = "brcm,bcm7445d0", "brcm,brcmstb";
     #address-cells = <0x2>;
     #size-cells = <0x2>;
     interrupt-parent = <0x1>;

     memory {
           #address-cells = <0x1>;
           #size-cells = <0x1>;
           device_type = "memory";
           reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;

           region@10000000 {
                contiguous-region;
                reg = <0x10000000 0x1f800000>;
                linux,phandle = <0x2>;
                phandle = <0x2>;
           };

           region@30000000 {
                contiguous-region;
                reg = <0x30000000 0x10000000>;
                linux,phandle = <0x3>;
                phandle = <0x3>;
           };

           region@40000000 {
                contiguous-region;
                reg = <0x40000000 0x40000000>;
                linux,phandle = <0x4>;
                phandle = <0x4>;
           };

           region@80000000 {
                contiguous-region;
                reg = <0x80000000 0x40000000>;
                linux,phandle = <0x5>;
                phandle = <0x5>;
           };
     };

As you can see, it specifies 0 + 1GB + 1GB + 1GB. When I run "xl info" in Dom0, though, it reports "Total 2048".

Digging into the code, I found that in bootinfo.mem.nr_banks is 2 rather than the expected 3 (or 4?). That turned out to be because in process_memory_node(), address_cells and size_cells were both 2 and so the prop_len of 32 was resulting in "banks" being set to 2.

Looking at device_tree_for_each_node(), it looks like something is wrong because it contains this loop:
    for ( node = 0, depth = 0;
          node >=0 && depth >= 0;
          node = fdt_next_node(fdt, node, &depth) )
    {
                [...]
        ret = func(fdt, node, name, depth,
                   address_cells[depth-1], size_cells[depth-1], data);
which looks like it will read before the start of the array for the first node, when depth=0. My first instinct was to remove those two "-1"s, but the resulting code didn't run, so I figured I'd try to enlist some help :) Of course it's possible that my problem is unrelated to this, but reading before the start of the array definitely seems like a bug that should be fixed (although in practice those values are never used for node 0). Looking through the history, it seems to have been like that since the function was first introduced (http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=9cf4a9a467171c8a2c3d97c2bfadb1bc8b15a3d6).

I'm happy to test out any patches.

Chris
(Not subscribed to the list)


[-- Attachment #1.2: Type: text/html, Size: 13988 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[David Vrabel]

On 23/06/15 00:02, Chris (Christopher) Brand wrote:
> I’ve been trying to figure out why Xen only reports 2GB on my ARM
> platform that actually has 3GB, and I think I’ve found a bug, but I’m
> not familiar enough with the Xen code to fix it.
> 
> The relevant parts of my dts are:
> 
> /dts-v1/;
> / {
> 
>      model = "Broadcom STB (7445d0)";
>      compatible = "brcm,bcm7445d0", "brcm,brcmstb";
>      #address-cells = <0x2>;
>      #size-cells = <0x2>;
>      interrupt-parent = <0x1>;
> 
>      memory {
>            #address-cells = <0x1>;
>            #size-cells = <0x1>;
>            device_type = "memory";
>            reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;

It's been a while since I've looked at device tree stuff but I think you
need 64-bit values for this reg property because the parent node has
#address-cells == 0x2 and #size-cells == 0x2.

David

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Ian Campbell]

On Tue, 2015-06-23 at 10:44 +0100, David Vrabel wrote:
> On 23/06/15 00:02, Chris (Christopher) Brand wrote:
> > I’ve been trying to figure out why Xen only reports 2GB on my ARM
> > platform that actually has 3GB, and I think I’ve found a bug, but I’m
> > not familiar enough with the Xen code to fix it.
> > 
> > The relevant parts of my dts are:
> > 
> > /dts-v1/;
> > / {
> > 
> >      model = "Broadcom STB (7445d0)";
> >      compatible = "brcm,bcm7445d0", "brcm,brcmstb";
> >      #address-cells = <0x2>;
> >      #size-cells = <0x2>;
> >      interrupt-parent = <0x1>;
> > 
> >      memory {
> >            #address-cells = <0x1>;
> >            #size-cells = <0x1>;
> >            device_type = "memory";
> >            reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
> 
> It's been a while since I've looked at device tree stuff but I think you
> need 64-bit values for this reg property because the parent node has
> #address-cells == 0x2 and #size-cells == 0x2.

Yes, the prevailing sizes will be 0x2 here, since the 0x1 only apply to
_children_. However you still need to write the cells as separate 32-bit
entries, so the above encodes to memory regions from 0x0.0 to
0x0.40000000 and 0x0.40000000 to 0x0.80000000 (using . to show where the
cell boundary lies).

I don't know this platform, but that seems a plausible description of 2x
1GB regions.

I've not yet looked at the code closer to see what's going on.

Ian.


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

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Julien Grall]

Hi,

On 23/06/2015 10:44, David Vrabel wrote:
> On 23/06/15 00:02, Chris (Christopher) Brand wrote:
>> I’ve been trying to figure out why Xen only reports 2GB on my ARM
>> platform that actually has 3GB, and I think I’ve found a bug, but I’m
>> not familiar enough with the Xen code to fix it.
>>
>> The relevant parts of my dts are:
>>
>> /dts-v1/;
>> / {
>>
>>       model = "Broadcom STB (7445d0)";
>>       compatible = "brcm,bcm7445d0", "brcm,brcmstb";
>>       #address-cells = <0x2>;
>>       #size-cells = <0x2>;
>>       interrupt-parent = <0x1>;
>>
>>       memory {
>>             #address-cells = <0x1>;
>>             #size-cells = <0x1>;
>>             device_type = "memory";
>>             reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
>
> It's been a while since I've looked at device tree stuff but I think you
> need 64-bit values for this reg property because the parent node has
> #address-cells == 0x2 and #size-cells == 0x2.

I think they are already on 64-bit values, otherwise you would have a 
bank starting at 0 of a size 0 which seems odd.

Anyway, the format of this memory node is not supported on Xen and I 
wasn't able to find a bindings somewhere. Will extend my point by 
answering to his mail.

Cheers,

-- 
Julien Grall

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Ian Campbell]

On Tue, 2015-06-23 at 10:57 +0100, Ian Campbell wrote:
> On Tue, 2015-06-23 at 10:44 +0100, David Vrabel wrote:
> > On 23/06/15 00:02, Chris (Christopher) Brand wrote:
> > > I’ve been trying to figure out why Xen only reports 2GB on my ARM
> > > platform that actually has 3GB, and I think I’ve found a bug, but I’m
> > > not familiar enough with the Xen code to fix it.
> > > 
> > > The relevant parts of my dts are:
> > > 
> > > /dts-v1/;
> > > / {
> > > 
> > >      model = "Broadcom STB (7445d0)";
> > >      compatible = "brcm,bcm7445d0", "brcm,brcmstb";
> > >      #address-cells = <0x2>;
> > >      #size-cells = <0x2>;
> > >      interrupt-parent = <0x1>;
> > > 
> > >      memory {
> > >            #address-cells = <0x1>;
> > >            #size-cells = <0x1>;
> > >            device_type = "memory";
> > >            reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
> > 
> > It's been a while since I've looked at device tree stuff but I think you
> > need 64-bit values for this reg property because the parent node has
> > #address-cells == 0x2 and #size-cells == 0x2.
> 
> Yes, the prevailing sizes will be 0x2 here, since the 0x1 only apply to
> _children_. However you still need to write the cells as separate 32-bit
> entries, so the above encodes to memory regions from 0x0.0 to
> 0x0.40000000 and 0x0.40000000 to 0x0.80000000 (using . to show where the
> cell boundary lies).
> 
> I don't know this platform, but that seems a plausible description of 2x
> 1GB regions.

I read the original report backwards, thinking 2GB was expected, but
rereading I see that 3GB was expected. In which case I think there is a
region missing in the DTB (or the hardware really only has 2GB).

Ian.


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

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Julien Grall]

Hi,

On 23/06/2015 00:02, Chris (Christopher) Brand wrote:
> I’ve been trying to figure out why Xen only reports 2GB on my ARM
> platform that actually has 3GB, and I think I’ve found a bug, but I’m
> not familiar enough with the Xen code to fix it.
> /dts-v1/;
>
> / {
>
>       model = "Broadcom STB (7445d0)";
>
>       compatible = "brcm,bcm7445d0", "brcm,brcmstb";
>
>       #address-cells = <0x2>;
>
>       #size-cells = <0x2>;
>
>       interrupt-parent = <0x1>;
>
>       memory {
>
>             #address-cells = <0x1>;
>
>             #size-cells = <0x1>;
>
>             device_type = "memory";
>
>             reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;

As said by David, Xen will parse "reg" using the #address-cells and 
#size-cells of the parent. So it's normal to see 2GB. Does the same 
device tree reports 3GB on Linux? I suspect no.

Although, what the rest of the node used for? Do we expect to parse it? 
I wasn't able to find a suitable bindings in the docs...

Regards,

-- 
Julien Grall

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Ian Campbell]

On Tue, 2015-06-23 at 11:08 +0100, Ian Campbell wrote:
> On Tue, 2015-06-23 at 10:57 +0100, Ian Campbell wrote:
> > On Tue, 2015-06-23 at 10:44 +0100, David Vrabel wrote:
> > > On 23/06/15 00:02, Chris (Christopher) Brand wrote:
> > > > I’ve been trying to figure out why Xen only reports 2GB on my ARM
> > > > platform that actually has 3GB, and I think I’ve found a bug, but I’m
> > > > not familiar enough with the Xen code to fix it.
> > > > 
> > > > The relevant parts of my dts are:
> > > > 
> > > > /dts-v1/;
> > > > / {
> > > > 
> > > >      model = "Broadcom STB (7445d0)";
> > > >      compatible = "brcm,bcm7445d0", "brcm,brcmstb";
> > > >      #address-cells = <0x2>;
> > > >      #size-cells = <0x2>;
> > > >      interrupt-parent = <0x1>;
> > > > 
> > > >      memory {
> > > >            #address-cells = <0x1>;
> > > >            #size-cells = <0x1>;
> > > >            device_type = "memory";
> > > >            reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
> > > 
> > > It's been a while since I've looked at device tree stuff but I think you
> > > need 64-bit values for this reg property because the parent node has
> > > #address-cells == 0x2 and #size-cells == 0x2.
> > 
> > Yes, the prevailing sizes will be 0x2 here, since the 0x1 only apply to
> > _children_. However you still need to write the cells as separate 32-bit
> > entries, so the above encodes to memory regions from 0x0.0 to
> > 0x0.40000000 and 0x0.40000000 to 0x0.80000000 (using . to show where the
> > cell boundary lies).

And to further clarify an apparent misunderstanding in the first mail: A
reg property is a list of <address> <size> pairs, with each consuming
the appropriate #foo-cells (so 2 for both here, meaning each entry is 4
cells in total).

Ian.


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

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Ian Campbell]

On Tue, 2015-06-23 at 11:08 +0100, Julien Grall wrote:

> >             reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
> 
> Although, what the rest of the node used for? Do we expect to parse it? 
> I wasn't able to find a suitable bindings in the docs...

A reg can encode multiple regions, by just listing them one after the
other. It'll be in the generic docs about reg properties I expect,
there's nothing special about a reg property in a memory node in this
regard.

Xen parses these just fine, via the loop and use of device_tree_get_reg
in process_memory_node

Ian.

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Julien Grall]

On 23/06/2015 11:27, Ian Campbell wrote:
> On Tue, 2015-06-23 at 11:08 +0100, Julien Grall wrote:
>
>>>              reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
>>
>> Although, what the rest of the node used for? Do we expect to parse it?
>> I wasn't able to find a suitable bindings in the docs...
>
> A reg can encode multiple regions, by just listing them one after the
> other. It'll be in the generic docs about reg properties I expect,
> there's nothing special about a reg property in a memory node in this
> regard.
>
> Xen parses these just fine, via the loop and use of device_tree_get_reg
> in process_memory_node

I know that a "reg" can encode multiple regions... My question was 
related to the "rest of the node" and not the rest of the property... i.e

    region@10000000 {

                 contiguous-region;

                 reg = <0x10000000 0x1f800000>;

                 linux,phandle = <0x2>;

                 phandle = <0x2>;

            };

AFAICT we don't parse it.

Regards,

-- 
Julien Grall

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Ian Campbell]

On Tue, 2015-06-23 at 11:32 +0100, Julien Grall wrote:
> 
> On 23/06/2015 11:27, Ian Campbell wrote:
> > On Tue, 2015-06-23 at 11:08 +0100, Julien Grall wrote:
> >
> >>>              reg = <0x0 0x0 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
> >>
> >> Although, what the rest of the node used for? Do we expect to parse it?
> >> I wasn't able to find a suitable bindings in the docs...
> >
> > A reg can encode multiple regions, by just listing them one after the
> > other. It'll be in the generic docs about reg properties I expect,
> > there's nothing special about a reg property in a memory node in this
> > regard.
> >
> > Xen parses these just fine, via the loop and use of device_tree_get_reg
> > in process_memory_node
> 
> I know that a "reg" can encode multiple regions... My question was 
> related to the "rest of the node" and not the rest of the property... i.e
> 
>     region@10000000 {
> 
>                  contiguous-region;
> 
>                  reg = <0x10000000 0x1f800000>;
> 
>                  linux,phandle = <0x2>;
> 
>                  phandle = <0x2>;
> 
>             };
> 
> AFAICT we don't parse it.

Indeed, but I'm pretty sure that whatever they are it doesn't relate to
the "only seeing 2GB out of 3GB issue", that's solely down to the main
bit of the memory node only encoding 2GB AFAICT.

Ian.

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Chris (Christopher) Brand]

>As said by David, Xen will parse "reg" using the #address-cells and #size-cells of the parent. So it's normal to see 2GB. Does the same device tree reports 3GB on Linux? I suspect no.

I'm not in the office today, so I can't test this, but I believe the answer is "yes". I have this log from an earlier run where I ran Linux without Xen. I *believe* used that same DT:
[    0.000000] Memory: 208924K/3145728K available (5557K kernel code, 280K rwdata, 1740K rodata, 3892K init, 213K bss, 36836K reserved, 2899968K cma-reserved, 0K highmem)

Chris

Re: Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Chris (Christopher) Brand]

>>As said by David, Xen will parse "reg" using the #address-cells and #size-cells of the parent. So it's normal to see 2GB. Does the same device tree reports 3GB on Linux? I suspect no.
>
>I'm not in the office today, so I can't test this, but I believe the answer is "yes".

But in fact the answer is indeed "no". The log that I found was from a different device tree.

I still think the code shouldn't be reading before the start of the two arrays, but it does function correctly.

Chris

[PATCH] xen: arm: bootfdt: Avoid reading off the front of *_cells array

[Ian Campbell]

In device_tree_for_each_node the call to the callback was using
{address,size}_cells[depth - 1], which at depth 0 could read off the
front of the array.

We already handled this correctly in the rest of the loop so fixup
this instance as well.

Reported-by: Chris (Christopher) Brand <chris.brand@broadcom.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Chris (Christopher) Brand <chris.brand@broadcom.com>
---
 xen/arch/arm/bootfdt.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e100233..74d208b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -100,6 +100,7 @@ static int __init device_tree_for_each_node(const void *fdt,
           node = fdt_next_node(fdt, node, &depth) )
     {
         const char *name = fdt_get_name(fdt, node, NULL);
+        u32 as, ss;
 
         if ( depth >= DEVICE_TREE_MAX_DEPTH )
         {
@@ -108,14 +109,15 @@ static int __init device_tree_for_each_node(const void *fdt,
             continue;
         }
 
-        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells",
-                                depth > 0 ? address_cells[depth-1] : 0);
-        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells",
-                                depth > 0 ? size_cells[depth-1] : 0);
+        as = depth > 0 ? address_cells[depth-1] : 0;
+        ss = depth > 0 ? size_cells[depth-1] : 0;
 
+        address_cells[depth] = device_tree_get_u32(fdt, node,
+                                                   "#address-cells", as);
+        size_cells[depth] = device_tree_get_u32(fdt, node,
+                                                "#size-cells", ss);
 
-        ret = func(fdt, node, name, depth,
-                   address_cells[depth-1], size_cells[depth-1], data);
+        ret = func(fdt, node, name, depth, as, ss, data);
         if ( ret != 0 )
             return ret;
     }
-- 
2.1.4

Re: [PATCH] xen: arm: bootfdt: Avoid reading off the front of *_cells array

[Julien Grall]

Hi Ian,

On 16/07/2015 10:50, Ian Campbell wrote:
> In device_tree_for_each_node the call to the callback was using
> {address,size}_cells[depth - 1], which at depth 0 could read off the
> front of the array.
>
> We already handled this correctly in the rest of the loop so fixup
> this instance as well.
>
> Reported-by: Chris (Christopher) Brand <chris.brand@broadcom.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Chris (Christopher) Brand <chris.brand@broadcom.com>

Good catch! Do you plan to backport to Xen 4.5 and apply to Xen 4.6?

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: bootfdt: Avoid reading off the front of *_cells array

[Ian Campbell]

On Thu, 2015-07-16 at 14:06 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 16/07/2015 10:50, Ian Campbell wrote:
> > In device_tree_for_each_node the call to the callback was using
> > {address,size}_cells[depth - 1], which at depth 0 could read off the
> > front of the array.
> >
> > We already handled this correctly in the rest of the loop so fixup
> > this instance as well.
> >
> > Reported-by: Chris (Christopher) Brand <chris.brand@broadcom.com>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Chris (Christopher) Brand <chris.brand@broadcom.com>
> 
> Good catch! Do you plan to backport to Xen 4.5 and apply to Xen 4.6?

Yes on both counts. This is a bugfix as far as 4.6 is concerned.

> Reviewed-by: Julien Grall <julien.grall@citrix.com>
> 
> Regards,
> 

Re: [PATCH] xen: arm: bootfdt: Avoid reading off the front of *_cells array

[Ian Campbell]

On Thu, 2015-07-16 at 14:06 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 16/07/2015 10:50, Ian Campbell wrote:
> > In device_tree_for_each_node the call to the callback was using
> > {address,size}_cells[depth - 1], which at depth 0 could read off the
> > front of the array.
> >
> > We already handled this correctly in the rest of the loop so fixup
> > this instance as well.
> >
> > Reported-by: Chris (Christopher) Brand <chris.brand@broadcom.com>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Chris (Christopher) Brand <chris.brand@broadcom.com>
> 
> Good catch! Do you plan to backport to Xen 4.5 and apply to Xen 4.6?
> 
> Reviewed-by: Julien Grall <julien.grall@citrix.com>

Applied and queue for backport, thanks.