Re: [PATCH 1/2] x86emul: correct EFLAGS testing for BMI1/BMI2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2024 00:46, Andrew Cooper wrote:
> On 12/11/2024 2:59 pm, Jan Beulich wrote:
>> Apparently I blindly copied the constants from the BEXTR case, where SF
>> indeed wants leaving out. For BLSI, BLSMSK, BLSR, and BZHI SF is
>> defined, and hence wants checking. This is noticable in particular for
>> BLSR, where with the input we use SF will be set in the result (and
>> hence is being switched to be clear on input).
>>
>> Convert to using named constants we have available, while omitting DF,
>> TF, as well as the MBZ bits 3 and 5 from the masking values in the
>> checks of the produced output. For BZHI also set SF on input, expecting
>> it to transition to clear.
>>
>> Fixes: 771daacd197a ("x86emul: support BMI1 insns")
>> Fixes: 8e20924de13d ("x86emul: support BMI2 insns")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

> However, there's a related bug elsewhere.  I recently learnt that the
> rotate instructions are different between vendors.  AMD leaves CF/OF
> well defined, others preserved, while Intel has CF well defined, and
> others undefined (seemingly zero in practice, but clearly there's a very
> old processor which wasn't).

Quoting from the PM's RCL page:

"For 1-bit rotates, the instruction sets the OF flag to the logical xor
 of the CF bit (after the rotate) and the most significant bit of the
 result. When the rotate count is greater than 1, the OF flag is undefined.
 When the rotate count is 0, no flags are affected."

That's the same as Intel behavior. If we were to test anything beyond what
the SDM says, we'd at least need a reference to where that behavior is
specified / described.

Further, considering ...

> We test RCL and happen to fall into a common subset between vendors.  At
> least the emulator itself dispatches to real instructions, so guests
> ought to see the behaviour correct for the CPU.

... this behavior, I'm not even sure I see where there would be a bug. We
could in principle tighten the test for the AMD case, or we could add a
comment. Yet neither would really look like a bugfix to me.

>> --- a/tools/tests/x86_emulator/test_x86_emulator.c
>> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
>> @@ -1969,10 +1969,13 @@ int main(int argc, char **argv)
>>  
>>          *res        = 0xfedcba98;
>>          regs.edx    = (unsigned long)res;
>> -        regs.eflags = 0xac2;
>> +        regs.eflags = EFLAGS_ALWAYS_SET | X86_EFLAGS_OF | X86_EFLAGS_SF | \
>> +                      X86_EFLAGS_ZF;
>>          rc = x86_emulate(&ctxt, &emulops);
>>          if ( (rc != X86EMUL_OKAY) || regs.ecx != 8 || *res != 0xfedcba98 ||
>> -             (regs.eflags & 0xf6b) != 0x203 || !check_eip(blsi) )
>> +             (regs.eflags & (EFLAGS_MASK & ~(X86_EFLAGS_AF | 
>> X86_EFLAGS_PF))) !=
>> +              (EFLAGS_ALWAYS_SET | X86_EFLAGS_CF) ||
> 
> As an observation, this is really wanting for an EFL_SYM() helper like
> the others I have in XTF  (I haven't needed one for flags specifically).
> 
> The verbosity definitely interferes with the clarity.

Hmm, yes - added to the TODO list.

Jan