POPF is a horribly expensive instruction, while STI is an optimised fastpath.

Switching POPF for a conditional branch and STI caused an 8% perf improvement
in various linux measurements.  While I don't expect the change to be that
dramatic in Xen, there will be an improvement.

Furthermore, there is the following code generation improvement:

  add/remove: 0/0 grow/shrink: 3/52 up/down: 52/-966 (-914)

owing to not needing to opencode the restriction to just IF in asm.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/include/asm/system.h | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/include/asm/system.h b/xen/arch/x86/include/asm/system.h
index 65e63de69a67..4be235472ecd 100644
--- a/xen/arch/x86/include/asm/system.h
+++ b/xen/arch/x86/include/asm/system.h
@@ -267,13 +267,8 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 })
 #define local_irq_restore(x)                                     \
 ({                                                               \
-    BUILD_BUG_ON(sizeof(x) != sizeof(long));                     \
-    asm volatile ( "pushfq\n\t"                                  \
-                   "andq %0, (%%rsp)\n\t"                        \
-                   "orq  %1, (%%rsp)\n\t"                        \
-                   "popfq"                                       \
-                   : : "i?r" ( ~X86_EFLAGS_IF ),                 \
-                       "ri" ( (x) & X86_EFLAGS_IF ) );           \
+    if ( (x) & X86_EFLAGS_IF )                                   \
+        local_irq_enable();                                      \
 })
 
 static inline int local_irq_is_enabled(void)
-- 
2.30.2

On 20.02.2023 20:47, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/system.h
> +++ b/xen/arch/x86/include/asm/system.h
> @@ -267,13 +267,8 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  })
>  #define local_irq_restore(x)                                     \
>  ({                                                               \
> -    BUILD_BUG_ON(sizeof(x) != sizeof(long));                     \
> -    asm volatile ( "pushfq\n\t"                                  \
> -                   "andq %0, (%%rsp)\n\t"                        \
> -                   "orq  %1, (%%rsp)\n\t"                        \
> -                   "popfq"                                       \
> -                   : : "i?r" ( ~X86_EFLAGS_IF ),                 \
> -                       "ri" ( (x) & X86_EFLAGS_IF ) );           \
> +    if ( (x) & X86_EFLAGS_IF )                                   \
> +        local_irq_enable();                                      \
>  })

Without it being written down anywhere that IRQs cannot be turned off
this way, and without there being a reference to that documentation
in the description, this is introducing a plain bug; I'm sorry to say
it that way. With both of the above fulfilled I'd of course be happy
to see the improvement take effect.

Jan