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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/10/13 09:35, Jan Beulich wrote:
>>>> On 21.10.13 at 20:37, Keir Fraser <keir.xen@gmail.com> wrote:
>> On 21/10/2013 19:30, "Andrew Cooper" <andrew.cooper3@citrix.com> 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".

Ok - I can change this.

>
> 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.

GCC correctly picks a 2-byte register given the orw.  Looking at the
disassembly, it usually chooses %r12w

Why is symmetry of writes important here?  We are possibly setting bit 9
alone.

>
> 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.

I am happy to remove __OS/__OP if that is considered a good thing moving
forward - I was merely using the prevailing style.

~Andrew