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: Fri, 17 May 2013 15:34:32 +0100 Message-ID: <1368801272.24012.49.camel@hastur.hellion.org.uk> 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> <1368543272.14264.41.camel@zakaz.uk.xensource.com> <1368543416.14264.42.camel@zakaz.uk.xensource.com> <1368544071.14264.46.camel@zakaz.uk.xensource.com> <5192787B02000078000D61D3@nat28.tlf.novell.com> <1368549665.14264.60.camel@zakaz.uk.xensource.com> <5193518E02000078000D647F@nat28.tlf.novell.com> <1368611254.11138.6.camel@hastur.hellion.org.uk> <5193995402000078000D6733@nat28.tlf.novell.com> <1368625620.11138.37.camel@hastur.hellion.org.uk> <5194CE7402000078000D6AF6@nat28.tlf.novell.com> <1368785245.24012.32.camel@hastur.hellion.org.uk> <519624EF02000078000D6F8F@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <519624EF02000078000D6F8F@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 Fri, 2013-05-17 at 11:39 +0100, Jan Beulich wrote: > >>> On 17.05.13 at 12:07, Ian Campbell wrote: > > On Thu, 2013-05-16 at 11:17 +0100, Jan Beulich wrote: > >> >>> On 15.05.13 at 15:47, Ian Campbell wrote: > >> > On Wed, 2013-05-15 at 13:19 +0100, Jan Beulich wrote: > >> >> >>> On 15.05.13 at 11:47, Ian Campbell wrote: > >> >> > "<" fails because it does process the (non-existent) first entry in the > >> >> > array. This happened to be "safe" in the case I saw but it wouldn't be > >> >> > in general. > >> >> > >> >> Okay, I misread one of your earlier responses then. Did you do > >> >> the necessary auditing already, or should I put this on my todo > >> >> list? > >> > > >> > I haven't done an audit. I put a very quicly grepped list in a previous > >> > mail but it is surely incomplete. > >> > >> So I went through all of them - the only other ones that can be > >> potentially empty are .ctors and .xsm_initcalls.init (I didn't check > >> whether ARM has any guaranteed .ex_table.pre uses though). > > > > On a random arm64 binary which I have here both ex_table and > > ex_table.pre are empty... > > Indeed. Yet arch/arm/ also has no reference to > __{start,stop}__{,_pre}_ex_table, so this is no problem. Out of > curiosity - there's nothing you ever do in ARM on behalf of the > user than can fault? That's right, we access guest memory with domain_map_page and never by using a guest virtual address directly (since we don't share page tables with the guest). The mechanism for translating guest virtual addresses into MFNs has an explicit error return and doesn't fault. > >> Both use "<", and the compiler translates this safely on x86. My > >> ARM assembly skills are still lacking, but afaict the early exit being > >> done with "popcs" / "b.cs" should be safe too, as they cover the > >> "==" case (APSR.C being set for x <= y). Thus I wonder what > >> code you saw being generated for the "<" case... > > > > 00000000 : > > 0: e92d4038 push {r3, r4, r5, lr} > > 4: e59f4020 ldr r4, [pc, #32] ; 2c > > 8: e59f5020 ldr r5, [pc, #32] ; 30 > > c: e1540005 cmp r4, r5 > > 10: 28bd8038 popcs {r3, r4, r5, pc} > > 14: e1a00004 mov r0, r4 > > 18: e2844004 add r4, r4, #4 > > 1c: ebfffffe bl 0 > > 20: e1540005 cmp r4, r5 > > 24: 3afffffa bcc 14 > > 28: e8bd8038 pop {r3, r4, r5, pc} > > > > So indeed I think you are correct that the popcs will do the right > > thing, I obviously missed the update of PC via that instruction when I > > looked at this before. > > But you still said that in practice you observed one unwanted > iteration of such loops - how does that fit together? You're right, I was sure I did observe that, but trying again now I don't. I think I may just have been confused earlier -- probably confusing my debug print after the loop with the one from inside. Sorry for the noise. > >> And btw., for both 32- and 64-bit ARM, other than for x86, I see > >> empty structure instances occupy zero bytes (and hence distinct > >> symbols end up at the same address), so the compiler is conflicting > >> with itself here. > > > > I imagine this is as much to do with the architecture ABI as the > > compiler. > > So do I, but this doesn't make this any less of a compiler bug. Is there a requirement (e.g. from the C standard) that an empty struct take at least one byte, or more likely I suppose something about different instances having different addresses which sort of implies it? Ian