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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@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?
> 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@xxxxxxxxxx>