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

[Keir Fraser <keir.xen@gmail.com>]

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.

Acked-by: Keir Fraser <keir@xen.org>

> ~Andrew