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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan