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@suse.com>
--- 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) ||
+ !check_eip(blsi) )
goto fail;
printf("okay\n");
}
@@ -1988,10 +1991,13 @@ int main(int argc, char **argv)
:: "d" (NULL) );
set_insn(blsmsk);
- regs.eflags = 0xac3;
+ regs.eflags = EFLAGS_ALWAYS_SET | X86_EFLAGS_OF | X86_EFLAGS_SF | \
+ X86_EFLAGS_ZF | X86_EFLAGS_CF;
rc = x86_emulate(&ctxt, &emulops);
if ( (rc != X86EMUL_OKAY) || regs.ecx != 0xf || *res != 0xfedcba98 ||
- (regs.eflags & 0xf6b) != 0x202 || !check_eip(blsmsk) )
+ (regs.eflags & (EFLAGS_MASK & ~(X86_EFLAGS_AF | X86_EFLAGS_PF))) !=
+ EFLAGS_ALWAYS_SET ||
+ !check_eip(blsmsk) )
goto fail;
printf("okay\n");
}
@@ -2007,10 +2013,13 @@ int main(int argc, char **argv)
:: "d" (NULL) );
set_insn(blsr);
- regs.eflags = 0xac3;
+ regs.eflags = EFLAGS_ALWAYS_SET | X86_EFLAGS_OF | X86_EFLAGS_ZF | \
+ X86_EFLAGS_CF;
rc = x86_emulate(&ctxt, &emulops);
if ( (rc != X86EMUL_OKAY) || regs.ecx != 0xfedcba90 ||
- (regs.eflags & 0xf6b) != 0x202 || !check_eip(blsr) )
+ (regs.eflags & (EFLAGS_MASK & ~(X86_EFLAGS_AF | X86_EFLAGS_PF))) !=
+ (EFLAGS_ALWAYS_SET | X86_EFLAGS_SF) ||
+ !check_eip(blsr) )
goto fail;
printf("okay\n");
}
@@ -2028,11 +2037,14 @@ int main(int argc, char **argv)
regs.ecx = (unsigned long)res;
regs.edx = 0xff13;
- regs.eflags = 0xa43;
+ regs.eflags = EFLAGS_ALWAYS_SET | X86_EFLAGS_OF | X86_EFLAGS_SF | \
+ X86_EFLAGS_ZF | X86_EFLAGS_CF;
rc = x86_emulate(&ctxt, &emulops);
if ( (rc != X86EMUL_OKAY) || regs.ebx != (*res & 0x7ffff) ||
regs.edx != 0xff13 || *res != 0xfedcba98 ||
- (regs.eflags & 0xf6b) != 0x202 || !check_eip(bzhi) )
+ (regs.eflags & (EFLAGS_MASK & ~(X86_EFLAGS_AF | X86_EFLAGS_PF))) !=
+ EFLAGS_ALWAYS_SET ||
+ !check_eip(bzhi) )
goto fail;
printf("okay\n");
}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@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
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
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@suse.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
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
© 2016 - 2025 Red Hat, Inc.