Re: [Xen-devel] [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-05-14 at 15:29 +0100, Jan Beulich wrote:
> >>> On 14.05.13 at 12:21, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Tue, 2013-05-14 at 10:48 +0100, Jan Beulich wrote:
> >> >>> On 14.05.13 at 11:32, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> >> > Sadly this turns out to be a bit more complicated :-( CCing Jan and Keir
> >> > since this was wider implications (this construct is used elsewhere) and
> >> > Ian J because he pointed this out.
> >> > 
> >> > TL;DR: There is probably a problem here, but I'm not sure it is one
> >> > worth worrying about, at least not yet/for 4.3.
> >> > 
> >> > The problem is that the compiler is allowed to assume that:
> >> >         extern const struct platform_desc _splatform[], _eplatform[];
> >> > Defines two *distinct* arrays, which therefore can never be equal. Hence
> >> > it optimises 
> >> >     for ( platform = _splatform; platform != _eplatform; platform++ )
> >> > with the assumption that there must always be at least one iteration.
> >> 
> >> Did you actually observe gcc doing this?
> > 
> > Yes, leading to an infinite loop.
> > 
> > With my "fixed" version (with the <) it just did one needless iteration
> > of whatever happened to be at _splatform =_eplatform, which luckily was
> > accessible data. It happens to be the device data (which likely suffers
> > the same issue).
> > 
> > I've also semi-confirmed by staring at the disassembly...
> 
> Just compiled (-O2) this
> 
> extern unsigned _s[], _e[];
> 
> void u(unsigned*);
> 
> void test(void) {
>       unsigned*p;
> 
>       for(p = _s; p != _e; ++p)
>               u(p);
> }
> 
> with gcc 4.7.3 for ix86, x86-64, and arm32, and none of them
> produced an infinite loop. And as said before, I'd be surprised a
> non-buggy gcc would, considering that with empty structures
> (which gcc allows) you can have zero-sized objects, and hence
> two named objects at the same address. And similarly, they
> support attributes to create symbol aliases, and for those it
> would again be necessary to account for the addresses of two
> distinct symbols to actually resolve to the same address.
> 
> What compiler version and switches were you using for the case
> where you spotted bad code to be generated?

gcc-linaro-aarch64-linux-gnu-4.7-2013.01-20130125_linux doing a standard
arm64 build of Xen.

With the attached patch with the original != check in the for loop:
        (XEN) Platforms: 0000000000265478-0000000000265478
        (XEN)   Considering platform 0000000000265478 = PL011 UART
        (XEN)   Considering platform 00000000002654b8 = <NULL>
        (XEN)   Considering platform 00000000002654f8 = <NULL>
        (XEN)   Considering platform 0000000000265538 = <NULL>
        (XEN)   Considering platform 0000000000265578 = <NULL>
        (XEN)   Considering platform 00000000002655b8 = <NULL>
        (XEN)   Considering platform 00000000002655f8 = <NULL>

If I use < in the for loop instead I get:
        (XEN) Platforms: 0000000000265478-0000000000265478
        (XEN) Finished with platform 0000000000265478 = PL011 UART
        (XEN) WARNING: Unrecognized/unsupported device tree compatible list

(so it seems I was wrong about getting a spurious check of zeroeth
element in this case)

I also just tried the same with arm32 and got:
        (XEN) Platforms: 0025fdc8-0025fdc8
        (XEN)   Considering platform 0025fdc8 = PL011 UART
        (XEN)   Considering platform 0025fde8 = <NULL>
        (XEN)   Considering platform 0025fe08 = <NULL>
        (XEN)   Considering platform 0025fe28 = <NULL>
        (XEN)   Considering platform 0025fe48 = <NULL>
        
or:

        (XEN) Platforms: 0025fdc8-0025fdc8
        (XEN) Finished with platform 0025fdc8 = PL011 UART
        (XEN) WARNING: Unrecognized/unsupported device tree compatible list

The test patch, mostly concerned with removing platforms to force an
empty list:
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 1335110..f12947e 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -60,13 +60,18 @@ int __init platform_init(void)
 
     ASSERT(platform == NULL);
 
+    printk("Platforms: %p-%p\n", _splatform, _eplatform);
+
     /* Looking for the platform description */
-    for ( platform = _splatform; platform < _eplatform; platform++ )
+    for ( platform = _splatform; platform != _eplatform; platform++ )
     {
+        printk("  Considering platform %p = %s\n", platform, platform->name);
         if ( platform_is_compatible(platform) )
             break;
     }
 
+    printk("Finished with platform %p = %s\n", platform, platform->name);
+
     /* We don't have specific operations for this platform */
     if ( platform == _eplatform )
     {
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index ff2b65b..358ec70 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -1,2 +1,2 @@
 obj-y += vexpress.o
-obj-y += exynos5.o
+#obj-y += exynos5.o
diff --git a/xen/arch/arm/platforms/vexpress.c 
b/xen/arch/arm/platforms/vexpress.c
index 8fc30c4..f689d7a 100644
--- a/xen/arch/arm/platforms/vexpress.c
+++ b/xen/arch/arm/platforms/vexpress.c
@@ -92,6 +92,7 @@ out:
     return ret;
 }
 
+#if 0
 /*
  * TODO: Get base address from the device tree
  * See arm,vexpress-reset node
@@ -129,6 +130,7 @@ PLATFORM_START(vexpress, "VERSATILE EXPRESS")
     .compatible = vexpress_dt_compat,
     .reset = vexpress_reset,
 PLATFORM_END
+#endif
 
 /*
  * Local variables:



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