[PATCH] x86emul: avoid using _PRE_EFLAGS() in a few cases

[PATCH] x86emul: avoid using _PRE_EFLAGS() in a few cases

[Jan Beulich]

The macro expanding to quite a few insns, replace its use by simply
clearing the status flags when the to be executed insn doesn't depend
on their initial state, in cases where this is easily possible. (There
are more cases where the uses are hidden inside macros, and where some
of the users of the macros want guest flags put in place before running
the insn, i.e. the macros can't be updated as easily.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6863,7 +6863,8 @@ x86_emulate(
         }
         opc[2] = 0xc3;
 
-        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
+        _regs.eflags &= ~EFLAGS_MASK;
+        invoke_stub("",
                     _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
                     [eflags] "+g" (_regs.eflags),
                     [tmp] "=&r" (dummy), "+m" (*mmvalp)
@@ -8111,7 +8112,8 @@ x86_emulate(
         opc[2] = 0xc3;
 
         copy_VEX(opc, vex);
-        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
+        _regs.eflags &= ~EFLAGS_MASK;
+        invoke_stub("",
                     _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
                     [eflags] "+g" (_regs.eflags),
                     "=a" (dst.val), [tmp] "=&r" (dummy)
@@ -11698,13 +11700,14 @@ int x86_emul_rmw(
         break;
 
     case rmw_xadd:
+        *eflags &= ~EFLAGS_MASK;
         switch ( state->op_bytes )
         {
             unsigned long dummy;
 
 #define XADD(sz, cst, mod) \
         case sz: \
-            asm ( _PRE_EFLAGS("[efl]", "[msk]", "[tmp]") \
+            asm ( "" \
                   COND_LOCK(xadd) " %"#mod"[reg], %[mem]; " \
                   _POST_EFLAGS("[efl]", "[msk]", "[tmp]") \
                   : [reg] "+" #cst (state->ea.val), \

Ping: [PATCH] x86emul: avoid using _PRE_EFLAGS() in a few cases

[Jan Beulich]

On 02.06.2021 16:37, Jan Beulich wrote:
> The macro expanding to quite a few insns, replace its use by simply
> clearing the status flags when the to be executed insn doesn't depend
> on their initial state, in cases where this is easily possible. (There
> are more cases where the uses are hidden inside macros, and where some
> of the users of the macros want guest flags put in place before running
> the insn, i.e. the macros can't be updated as easily.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Anyone?

Thanks, Jan

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -6863,7 +6863,8 @@ x86_emulate(
>          }
>          opc[2] = 0xc3;
>  
> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
> +        _regs.eflags &= ~EFLAGS_MASK;
> +        invoke_stub("",
>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>                      [eflags] "+g" (_regs.eflags),
>                      [tmp] "=&r" (dummy), "+m" (*mmvalp)
> @@ -8111,7 +8112,8 @@ x86_emulate(
>          opc[2] = 0xc3;
>  
>          copy_VEX(opc, vex);
> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
> +        _regs.eflags &= ~EFLAGS_MASK;
> +        invoke_stub("",
>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>                      [eflags] "+g" (_regs.eflags),
>                      "=a" (dst.val), [tmp] "=&r" (dummy)
> @@ -11698,13 +11700,14 @@ int x86_emul_rmw(
>          break;
>  
>      case rmw_xadd:
> +        *eflags &= ~EFLAGS_MASK;
>          switch ( state->op_bytes )
>          {
>              unsigned long dummy;
>  
>  #define XADD(sz, cst, mod) \
>          case sz: \
> -            asm ( _PRE_EFLAGS("[efl]", "[msk]", "[tmp]") \
> +            asm ( "" \
>                    COND_LOCK(xadd) " %"#mod"[reg], %[mem]; " \
>                    _POST_EFLAGS("[efl]", "[msk]", "[tmp]") \
>                    : [reg] "+" #cst (state->ea.val), \
> 
> 

Re: [PATCH] x86emul: avoid using _PRE_EFLAGS() in a few cases

[Andrew Cooper]

On 02/06/2021 15:37, Jan Beulich wrote:
> The macro expanding to quite a few insns, replace its use by simply
> clearing the status flags when the to be executed insn doesn't depend
> on their initial state, in cases where this is easily possible. (There
> are more cases where the uses are hidden inside macros, and where some
> of the users of the macros want guest flags put in place before running
> the insn, i.e. the macros can't be updated as easily.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Honestly, this is the first time I've looked into _PRE/_POST_EFLAGS() in
detail.  Why is most of this not in C to begin with?

The only bits which need to be in asm are the popf to establish the
stub's flags context, and the pushf to save the resulting state. 
Everything else is better off done by the compiler especially as it
would remove a load of work on temporaries from the stack.

Nevertheless, ...

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -6863,7 +6863,8 @@ x86_emulate(
>          }
>          opc[2] = 0xc3;
>  
> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
> +        _regs.eflags &= ~EFLAGS_MASK;
> +        invoke_stub("",
>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>                      [eflags] "+g" (_regs.eflags),
>                      [tmp] "=&r" (dummy), "+m" (*mmvalp)
> @@ -8111,7 +8112,8 @@ x86_emulate(
>          opc[2] = 0xc3;
>  
>          copy_VEX(opc, vex);
> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
> +        _regs.eflags &= ~EFLAGS_MASK;
> +        invoke_stub("",
>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>                      [eflags] "+g" (_regs.eflags),
>                      "=a" (dst.val), [tmp] "=&r" (dummy)

... this hunk is k{,or}test, which only modified ZF and CF according to
the SDM.

The other flags are not listed as modified, which means they're
preserved, unless you're planning to have Intel issue a correction to
the SDM.

The flags logic for both instructions is custom, so it wouldn't be a
surprise to me if it really did deviate from the normal behaviour of a
test operation.

~Andrew

> @@ -11698,13 +11700,14 @@ int x86_emul_rmw(
>          break;
>  
>      case rmw_xadd:
> +        *eflags &= ~EFLAGS_MASK;
>          switch ( state->op_bytes )
>          {
>              unsigned long dummy;
>  
>  #define XADD(sz, cst, mod) \
>          case sz: \
> -            asm ( _PRE_EFLAGS("[efl]", "[msk]", "[tmp]") \
> +            asm ( "" \
>                    COND_LOCK(xadd) " %"#mod"[reg], %[mem]; " \
>                    _POST_EFLAGS("[efl]", "[msk]", "[tmp]") \
>                    : [reg] "+" #cst (state->ea.val), \
>

Re: [PATCH] x86emul: avoid using _PRE_EFLAGS() in a few cases

[Jan Beulich]

On 28.06.2021 19:14, Andrew Cooper wrote:
> On 02/06/2021 15:37, Jan Beulich wrote:
>> The macro expanding to quite a few insns, replace its use by simply
>> clearing the status flags when the to be executed insn doesn't depend
>> on their initial state, in cases where this is easily possible. (There
>> are more cases where the uses are hidden inside macros, and where some
>> of the users of the macros want guest flags put in place before running
>> the insn, i.e. the macros can't be updated as easily.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Honestly, this is the first time I've looked into _PRE/_POST_EFLAGS() in
> detail.  Why is most of this not in C to begin with?

Ask Keir?

> The only bits which need to be in asm are the popf to establish the
> stub's flags context, and the pushf to save the resulting state. 
> Everything else is better off done by the compiler especially as it
> would remove a load of work on temporaries from the stack.

I'll try to find time to do something along these lines.

> Nevertheless, ...
> 
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -6863,7 +6863,8 @@ x86_emulate(
>>          }
>>          opc[2] = 0xc3;
>>  
>> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>> +        _regs.eflags &= ~EFLAGS_MASK;
>> +        invoke_stub("",
>>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>                      [eflags] "+g" (_regs.eflags),
>>                      [tmp] "=&r" (dummy), "+m" (*mmvalp)
>> @@ -8111,7 +8112,8 @@ x86_emulate(
>>          opc[2] = 0xc3;
>>  
>>          copy_VEX(opc, vex);
>> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>> +        _regs.eflags &= ~EFLAGS_MASK;
>> +        invoke_stub("",
>>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>                      [eflags] "+g" (_regs.eflags),
>>                      "=a" (dst.val), [tmp] "=&r" (dummy)
> 
> ... this hunk is k{,or}test, which only modified ZF and CF according to
> the SDM.
> 
> The other flags are not listed as modified, which means they're
> preserved, unless you're planning to have Intel issue a correction to
> the SDM.

kortest has

"The OF, SF, AF, and PF flags are set to 0."

in its "Flags Affected" section. ktest has

"AF := OF := PF := SF := 0;"

in its "Operation" section.

Jan

Re: [PATCH] x86emul: avoid using _PRE_EFLAGS() in a few cases

[Andrew Cooper]

On 29/06/2021 10:09, Jan Beulich wrote:
> On 28.06.2021 19:14, Andrew Cooper wrote:
>> On 02/06/2021 15:37, Jan Beulich wrote:
>>> The macro expanding to quite a few insns, replace its use by simply
>>> clearing the status flags when the to be executed insn doesn't depend
>>> on their initial state, in cases where this is easily possible. (There
>>> are more cases where the uses are hidden inside macros, and where some
>>> of the users of the macros want guest flags put in place before running
>>> the insn, i.e. the macros can't be updated as easily.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Honestly, this is the first time I've looked into _PRE/_POST_EFLAGS() in
>> detail.  Why is most of this not in C to begin with?
> Ask Keir?
>
>> The only bits which need to be in asm are the popf to establish the
>> stub's flags context, and the pushf to save the resulting state. 
>> Everything else is better off done by the compiler especially as it
>> would remove a load of work on temporaries from the stack.
> I'll try to find time to do something along these lines.
>
>> Nevertheless, ...
>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -6863,7 +6863,8 @@ x86_emulate(
>>>          }
>>>          opc[2] = 0xc3;
>>>  
>>> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>> +        _regs.eflags &= ~EFLAGS_MASK;
>>> +        invoke_stub("",
>>>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>>                      [eflags] "+g" (_regs.eflags),
>>>                      [tmp] "=&r" (dummy), "+m" (*mmvalp)
>>> @@ -8111,7 +8112,8 @@ x86_emulate(
>>>          opc[2] = 0xc3;
>>>  
>>>          copy_VEX(opc, vex);
>>> -        invoke_stub(_PRE_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>> +        _regs.eflags &= ~EFLAGS_MASK;
>>> +        invoke_stub("",
>>>                      _POST_EFLAGS("[eflags]", "[mask]", "[tmp]"),
>>>                      [eflags] "+g" (_regs.eflags),
>>>                      "=a" (dst.val), [tmp] "=&r" (dummy)
>> ... this hunk is k{,or}test, which only modified ZF and CF according to
>> the SDM.
>>
>> The other flags are not listed as modified, which means they're
>> preserved, unless you're planning to have Intel issue a correction to
>> the SDM.
> kortest has
>
> "The OF, SF, AF, and PF flags are set to 0."
>
> in its "Flags Affected" section. ktest has
>
> "AF := OF := PF := SF := 0;"
>
> in its "Operation" section.

Oh - the pseudocode and the text don't match.  How helpful.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>