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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

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