On 02/12/2011 08:54, "Jan Beulich" <JBeulich@suse.com> wrote:> Keir, > > do you recall what it was that 20976:f8692cc67d67 was supposed to fix? > The code is clearly wrong now on x86-64 - inline asm that uses push/pop > (or other stack pointer manipulation instructions) and memory accesses > that may reference the stack (even if not through %rsp) can''t be > expected to work without -mno-red-zone.I totally missed the red-zone constraint. :-(> The question is whether to use that command line option, or whether to > correct the inline assembly (besides the purpose of your change, I also > wonder why this isn''t coded the obvious way, with rBX and rDX explicitly > named in the constraints - on 32-bit this may be to reduce register > pressure, but on 64-bit it''s entirely unclear).I think reg constraint failures had only been reported on 32-bit. So how about the attached patch? -- Keir> Thanks, Jan >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 02/12/2011 15:26, "Olaf Hering" <olaf@aepfle.de> wrote:> On Fri, Dec 02, Keir Fraser wrote: > >> I think reg constraint failures had only been reported on 32-bit. So how >> about the attached patch? > > + : "0" (input[0]), "1" (count), "S" (_regs) > > _regs is undeclared in 32bit, I think it should be regs.Oops! Fixed! Thanks, Keir> Olaf
Keir, do you recall what it was that 20976:f8692cc67d67 was supposed to fix? The code is clearly wrong now on x86-64 - inline asm that uses push/pop (or other stack pointer manipulation instructions) and memory accesses that may reference the stack (even if not through %rsp) can''t be expected to work without -mno-red-zone. The question is whether to use that command line option, or whether to correct the inline assembly (besides the purpose of your change, I also wonder why this isn''t coded the obvious way, with rBX and rDX explicitly named in the constraints - on 32-bit this may be to reduce register pressure, but on 64-bit it''s entirely unclear). Thanks, Jan
On 02/12/2011 16:47, "Olaf Hering" <olaf@aepfle.de> wrote:> On Fri, Dec 02, Keir Fraser wrote: > >> I think reg constraint failures had only been reported on 32-bit. So how >> about the attached patch? > > So finally I was able to test this change, and it fixes the reported segfault. > Thanks!I will sweep this through into 4.0/4.1 as part of a general backport sweep next week. -- Keir> Olaf
>>> On 02.12.11 at 07:02, Keir Fraser <keir.xen@gmail.com> wrote: > On 02/12/2011 08:54, "Jan Beulich" <JBeulich@suse.com> wrote: >> The question is whether to use that command line option, or whether to >> correct the inline assembly (besides the purpose of your change, I also >> wonder why this isn''t coded the obvious way, with rBX and rDX explicitly >> named in the constraints - on 32-bit this may be to reduce register >> pressure, but on 64-bit it''s entirely unclear). > > I think reg constraint failures had only been reported on 32-bit. So how > about the attached patch?Looks good. Acked-by: Jan Beulich <jbeulich@novell.com> While we only got problems with this on 4.1.2, I would suggest to also put this back into 4.0-testing. Thanks, Jan E-mail confidentiality notice. This message is intended for the addressees only. It may be private, confidential and may be covered by legal professional privilege or other confidentiality requirements. If you are not one of the intended recipients, please notify the sender immediately on +44 0 20-8215-3000 and delete the message from all locations in your computer network. Do not copy this email or use it for any purpose or disclose its contents to any person:to do so maybe unlawful.
On Fri, Dec 02, Keir Fraser wrote:> I think reg constraint failures had only been reported on 32-bit. So how > about the attached patch?+ : "0" (input[0]), "1" (count), "S" (_regs) _regs is undeclared in 32bit, I think it should be regs. Olaf
On Fri, Dec 02, Keir Fraser wrote:> I think reg constraint failures had only been reported on 32-bit. So how > about the attached patch?So finally I was able to test this change, and it fixes the reported segfault. Thanks! Olaf