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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.12 at 13:00, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 03/08/2012 11:34, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>>>>> 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.
> 
> I'm confused. The registers have the same values at the start and the end of
> the asm statement. How can it possibly matter, even in theory, whether they
> temporarily change in the middle? Is this fairly strong assumption written
> down in the gcc documentation anywhere?

It's in the specification of the & modifier:

"â&â Means (in a particular alternative) that this operand is an
 earlyclobber operand, which is modified before the instruction
 is finished using the input operands. Therefore, this operand
 may not lie in a register that is used as an input operand or as
 part of any memory address."

Of course, here we're not having any other operands, which
is why at least at present getting this wrong does no harm.

>>> 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.
> 
> Yes, that confused me too -- the output constraints on the second asm can
> hardly be wrong, or at least matter, since it never returns! Execution state
> is completely reloaded within the asm statement.

Formally they're wrong too without that change. And the
fact that the asm() does not "return" is irrelevant here, as
the restriction is because of the potential use of the register
inside the asm(), after it already got modified.

Formally it's also not permitted for the asm() to branch
elsewhere, but that is violated in so many places (Linux not
the least) that they can hardly dare to ever come up with
something breaking this.

"Speaking of labels, jumps from one asm to another are not
 supported. The compilerâs optimizers do not know about these
 jumps, and therefore they cannot take account of them when
 deciding how to optimize."

Plus, this likely is really targeting jumps from one asm to another
_within_ one function, albeit that's not being said explicitly.

Jan

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