Re: [Xen-devel] [PATCH] x86: fix wait code asm() constraints

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.12 at 12:04, Keir Fraser <keir@xxxxxxx> wrote:
> On 03/08/2012 09:40, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> In __prepare_to_wait(), properly mark early clobbered registers. By
>> doing so, we at once eliminate the need to save/restore rCX and rDI.
>> 
>> In check_wakeup_from_wait(), make the current constraints match by
>> removing the code that actuall alters registers. By adjusting the
>> resume address in __prepare_to_wait(), we can simply re-use the copying
>> operation there (rather than doing a second pointless copy in the
>> opposite direction after branching to the resume point), which at once
>> eliminates the need for re-loading rCX and rDI inside the asm().
> 
> First of all, this is code improvement, rather than a bug fix, right? The
> asm constraints are correct for the code as it is, I believe.

No, the constraints aren't really correct at present (yet this is
not visible as a functional bug in any way) - from a formal
perspective, the early clobber specification is needed on _any_
operand that doesn't retain its value throughout an asm(). Any
future compiler could derive something from this that we don't
intend.

> It also seems the patch splits into two independent parts:
> 
>  A. Not sure whether the trade-off of the rCX/rDI save/restore versus more
> complex asm constraints makes sense.
> 
>  B. Separately, the adjustment of the restore return address, and avoiding
> needing to reload rCX/rDI after label 1, as well as avoiding the copy in
> check_wakeup_from_wait(), is very nice.
> 
> I'm inclined to take the second part only, and make it clearer in the
> changeset comment that it is not a bug fix.
> 
> What do you think?

The patch could be split, yes, but where exactly the split(s)
should be isn't that obvious to me. And as it's fixing the same
kind of issue on both asm()-s, it seemed sensible to keep the
changes together.

Jan


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