Re: [PATCH] x86/bitops: Replace __scanbit() with a simpler tzcnt()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2026 22:17, Andrew Cooper wrote:
> On 11/11/2026 8:33 am, Jan Beulich wrote:
>> On 10.11.2026 16:31, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/bitops.h
>>> +++ b/xen/arch/x86/include/asm/bitops.h
>>> @@ -320,18 +320,23 @@ extern unsigned int __find_first_zero_bit(
>>>  extern unsigned int __find_next_zero_bit(
>>>      const unsigned long *addr, unsigned int size, unsigned int offset);
>>>  
>>> -static always_inline unsigned int __scanbit(unsigned long val, unsigned 
>>> int max)
>>> +/*
>>> + * Search for the first set bit in @x, returning BITS_PER_LONG if @x was 0.
>>> + * See arch_ffs() for safety discussions.
>>> + */
>>> +static always_inline unsigned int tzcnt(unsigned long x)
>>>  {
>>> -    if ( __builtin_constant_p(max) && max == BITS_PER_LONG )
>>> -        alternative_io("bsf %[in],%[out]; cmovz %[max],%k[out]",
>>> -                       "rep; bsf %[in],%[out]",
>>> -                       X86_FEATURE_BMI1,
>>> -                       [out] "=&r" (val),
>>> -                       [in] "r" (val), [max] "r" (max));
>>> -    else
>>> -        asm ( "bsf %1,%0 ; cmovz %2,%k0"
>>> -              : "=&r" (val) : "r" (val), "r" (max) );
>>> -    return (unsigned int)val;
>>> +    unsigned int r;
>>> +
>>> +    /*
>>> +     * The destination register still needs pre-loading for older CPUs 
>>> which
>>> +     * will execute TZCNT as BSF.
>>> +     */
>>> +    asm ( "tzcnt %[x], %q[r]"
>>> +          : [r] "=r" (r)
>>> +          : [x] "rm" (x), "[r]" (BITS_PER_LONG) );
>>> +
>>> +    return r;
>>>  }
>> By having r and the function return type be unsigned int, when the actual
>> insn really is a qword operation, you force call sites to do the zero-
>> extension if they want an unsigned long quantity. When both are unsigned
>> long, no zero-extension needs performing by the compiler. Truncation to
>> unsigned int (or less) won't require any extra insn(s) anyway. While this
>> doesn't look as if it would make a difference for present uses, future
>> ones may benefit.
> 
> This matches the other helpers, so if we want to change something, it
> wants doing consistently.

Okay, as said - this was only a suggestion. As to doing consistently: Yes
and no. Yes as to the overall final outcome, but less so "all in one go".
Like we transitioned bool_t -> bool over time, and like we transition to
{,u}int<N>_t piecemeal, I expect the same would be true for such
helpers. See how e.g. you have used unsigned long for the local vars in
rdmsr(). (Albeit yes, rdmsrl() has them like that, too.)

> I picked it this way around because it improved code generation for
> preloading the destination register.
> 
> https://godbolt.org/z/qjoMPh3Kb
> 
> The zero-extension is as you say, but as the output is strictly in the
> range 0 to 64, it shouldn't ever need to be in a long as opposed to an int.
> 
> What I don't understand is why the zero-extension is being inserted. 
> There's no way to produce a 32bit value without the upper bits having
> been cleared,

For asm() the compiler can't know, as there are ways to produce 32-bit
values in registers with the upper bits not cleared. That may be what
you're ...

> and if you write a 64bit value but lie to the compiler
> about it being 32bit then that's your own fault for having lied about
> your constraints.

.. alluding to here, but that's not something the compiler can rely
upon. I.e. it does not see it as "your fault". When you say the output
is 32-bit, it has to treat it like this. After all explicit truncation
of a value may have been meant.

Jan