diff mbox series

x86emul: avoid using _PRE_EFLAGS() in a few cases

Message ID 9285f566-e352-9265-e9e3-e9a1e15ce7d5@suse.com (mailing list archive)
State New
Headers show
Series x86emul: avoid using _PRE_EFLAGS() in a few cases | expand

Commit Message

Jan Beulich June 2, 2021, 2:37 p.m. UTC
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>

Comments

Jan Beulich June 28, 2021, 12:14 p.m. UTC | #1
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), \
> 
>
Andrew Cooper June 28, 2021, 5:14 p.m. UTC | #2
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), \
>
Jan Beulich June 29, 2021, 9:09 a.m. UTC | #3
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
Andrew Cooper June 29, 2021, 10 a.m. UTC | #4
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>
diff mbox series

Patch

--- 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), \