Re: [PATCH] x86emul: adjust BSF/BSR behavior as to EFLAGS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2025 11:49, Andrew Cooper wrote:
> On 23/04/2025 7:13 am, Jan Beulich wrote:
>> SDM revision 087 points out that apparently as of quite some time ago on
>> Intel hardware BSF and BSR may alter all arithmetic flags, not just ZF.
>> Because of the inconsistency (and because documentation doesn't look to
>> be quite right about PF), best we can do is simply take the flag values
>> from what the processor produces, just like we do for various other
>> arithmetic insns. (Note also that AMD and Intel have always been
>> disagreeing on arithmetic flags other than ZF.)
> 
> The new footnote Intel have added about "older processors" does match
> AMD, saying "unmodified".
> 
> I think it's clear now that Intel and AMD behaviour was the same
> originally, except AMD wrote "unmodified" and Intel wrote "undefined",
> and Intel used this flexibility to alter the behaviour.

Except it isn't - U in AMD's nomenclature is "undefined", not "unmodified"
(and despite what you wrote above I hope you agree that the two mean
different things). Unmodified flag entries are simply blank.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -5268,16 +5268,14 @@ x86_emulate(
>>          break;
>>  
>>      case X86EMUL_OPC(0x0f, 0xbc): /* bsf or tzcnt */
>> -    {
>> -        bool zf;
>> -
>> -        asm ( "bsf %2,%0" ASM_FLAG_OUT(, "; setz %1")
>> -              : "=r" (dst.val), ASM_FLAG_OUT("=@ccz", "=qm") (zf)
>> -              : "rm" (src.val) );
>> -        _regs.eflags &= ~X86_EFLAGS_ZF;
>>          if ( (vex.pfx == vex_f3) && vcpu_has_bmi1() )
>>          {
>> -            _regs.eflags &= ~X86_EFLAGS_CF;
>> +            bool zf;
>> +
>> +            asm ( "bsf %2,%0" ASM_FLAG_OUT(, "; setz %1")
>> +                  : "=r" (dst.val), ASM_FLAG_OUT("=@ccz", "=qm") (zf)
>> +                  : "rm" (src.val) );
>> +            _regs.eflags &= ~(X86_EFLAGS_ZF | X86_EFLAGS_CF);
>>              if ( zf )
>>              {
>>                  _regs.eflags |= X86_EFLAGS_CF;
>> @@ -5286,25 +5284,23 @@ x86_emulate(
>>              else if ( !dst.val )
>>                  _regs.eflags |= X86_EFLAGS_ZF;
>>          }
>> -        else if ( zf )
>> +        else
>>          {
>> -            _regs.eflags |= X86_EFLAGS_ZF;
>> -            dst.type = OP_NONE;
>> +            emulate_2op_SrcV_srcmem("bsf", src, dst, _regs.eflags);
>> +            if ( _regs.eflags & X86_EFLAGS_ZF )
>> +                dst.type = OP_NONE;
> 
> On Intel, BSF/BSR writes back the destination register.  Notably, it
> gets 0 extended per normal rules,

That's also only on "older processors", as per the other footnote.

> which is why you have to be extra
> careful when using the trick of preloading it with -1; the result must
> be interpreted as (int) even over a 64bit operation.
> 
> This needs an amd_like() qualification to override dst.type.  This
> aspect genuinely is different between them.  Alternatively, we might be
> able to set the operand size always to 64 and write back the entire
> register as the processor gave to us, but I'm not sure if that will have
> effects elsewhere.

Besides (as per above), amd_like() not covering all cases, this would
then further need special treatment for 16-bit opsize. Plus promoting
to 64-bit would require manually clipping the result to 5 bits when
the original size wants 64-bit. That's imo far more complications
than gains.

Further, this patch is really about EFLAGS only; the OP_NONE was there
already before.

> Finally, I'm wary leaving TZCNT/LZCNT using the old logic.  Despite the
> absence of an update in 087, I find it unlikely that they behave
> differently WRT flags (specifically, I severely doubt they've got
> differing circuitry).

They do, I checked. Iirc I even mentioned on Matrix that I'm surprised
by the difference.

> I'd suggest giving them the same fully-emulated treatment as BSF/BSR.

But we're emulating them as the correct insn even if the respective
feature flag isn't set in the host policy. I don't want to break that,
so doing as you suggest would mean further bifurcating the handling
(to deal with the two different cases of what underlying hardware we
run on). This feels like unnecessary extra complexity to me.

Jan