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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.10.13 at 16:09, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 21/10/2013 14:58, "David Vrabel" <david.vrabel@xxxxxxxxxx> wrote:
> 
>> On 21/10/13 14:41, Andrew Cooper wrote:
>>> local_irq_restore() should only be concerned with possibly changing the
>>> interrupt flag.  A blind popf could corrupt other system flags.
>>> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> CC: Keir Fraser <keir@xxxxxxx>
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> ---
>>>  xen/include/asm-x86/system.h |    6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
>>> index 6ab7d56..cbf0f6a 100644
>>> --- a/xen/include/asm-x86/system.h
>>> +++ b/xen/include/asm-x86/system.h
>>> @@ -159,8 +159,10 @@ 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" );               \
>>> +    if ( x & X86_EFLAGS_IF )                                     \
>>> +        local_irq_enable();                                      \
>>> +    else                                                         \
>>> +        local_irq_disable();                                     \
>>>  })
>> 
>> This adds a branch in a potentially hot path.
>> 
>> Is the local_irq_disable() needed? Interrupts should already be disabled
>> on entry.
> 
> If that is always true, and so we get rid of the else, then
> local_irq_restore() should ASSERT it on entry.

Let's not go that route - the macro is supposed to do what it
says - restore the prior state of the interrupt flag.

To deal with the two branches the code adds I'd rather
recommend merging current and to-be-restored flags - the
necessary "and" and "or" are quite likely faster than any
mis-predicted branch (and perhaps as fast as a correctly
predicted one).

Jan


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