Re: [Xen-devel] [Patch 1/3 v2] x86/irq: local_irq_restore() should not blindly popf

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.10.13 at 20:37, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 21/10/2013 19:30, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx> wrote:
> 
>>>>  #define read_segment_register(name)                             \
>>>>  ({  u16 __sel;                                                  \
>>>> @@ -159,15 +160,19 @@ static always_inline unsigned long __cmpxchg(
>>>>  #define local_irq_restore(x)                                     \
>>>>  ({                                                               \
>>>>      BUILD_BUG_ON(sizeof(x) != sizeof(long));                     \
>>>> -    asm volatile ( "push" __OS " %0 ; popf" __OS                 \
>>>> -                   : : "g" (x) : "memory", "cc" );               \
>>>> +    asm volatile (                                               \
>>>> +    "pushf" __OS "\n\t"                                          \
>>>> +    "and" __OS " %0, (%%" __OP "sp)\n\t"                         \
>>>> +    "orw %1, (%%" __OP "sp)\n\t"                                 \
>>>> +    "popf" __OS "\n\t" : : "g" ( ~X86_EFLAGS_IF ),               \
>>> Would this be better as a constant constraint ("i")?
>> 
>> I was wondering what the best practice for this would be.
>> 
>> In most cases, I would imagine that an immediate would be used.
>> However, as this is a define and therefore forcibly inlined everywhere
>> it is used, it is just possible that the compiler could find a
>> ~X86_EFLAGS_IF already in context, and optimise down to an "and r64,r/m64".
> 
> Oh, g includes i, I forgot that. Well your choice is best then.

Sorry, but no. "g" also includes "m", and
- the other operand of both operations is a memory operand
  already, so this one can't also be a memory one,
- on a non-debug build (without frame pointers) an eventual
  %rsp-relative memory location would be broken due to the
  shifted stack offsets resulting from the PUSHF.
Hence both constraints can at best be "ri".

Further I have a hard time seeing how the "orw" used above
can even have built successfully: If a register gets picked
(which ought to be the common case), opcode suffix and
register name ought to collide. And "orw" is a bad choice here
anyway, in that this is a 2-byte write following an 8-byte one.

And finally - what's the point of using __OS in new assembly
constructs? I was actually considering cleaning up all this hard
to read cruft, since we no longer care about the 32-bit case.

Jan


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