From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. Date: Tue, 14 May 2013 15:54:32 +0100 Message-ID: <1368543272.14264.41.camel@zakaz.uk.xensource.com> References: <1368462160.537.125.camel@zakaz.uk.xensource.com> <1368462182-10317-4-git-send-email-ian.campbell@citrix.com> <1368523975.14264.22.camel@zakaz.uk.xensource.com> <5192248402000078000D5E7F@nat28.tlf.novell.com> <1368526880.14264.29.camel@zakaz.uk.xensource.com> <5192666A02000078000D60F8@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5192666A02000078000D60F8@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "Keir (Xen.org)" , Stefano Stabellini , Ian Jackson , "Tim (Xen.org)" , "xen-devel@lists.xen.org" , Julien Grall List-Id: xen-devel@lists.xenproject.org On Tue, 2013-05-14 at 15:29 +0100, Jan Beulich wrote: > >>> On 14.05.13 at 12:21, Ian Campbell wrote: > > On Tue, 2013-05-14 at 10:48 +0100, Jan Beulich wrote: > >> >>> On 14.05.13 at 11:32, Ian Campbell 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 = (XEN) Considering platform 00000000002654f8 = (XEN) Considering platform 0000000000265538 = (XEN) Considering platform 0000000000265578 = (XEN) Considering platform 00000000002655b8 = (XEN) Considering platform 00000000002655f8 = 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 = (XEN) Considering platform 0025fe08 = (XEN) Considering platform 0025fe28 = (XEN) Considering platform 0025fe48 = 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: