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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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>

That's horribly subtle, but I think I've been staring at the manuals
long enough.

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).

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.

>
> --- 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.

~Andrew