Re: [PATCH v2] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/03/2025 1:52 pm, Jan Beulich wrote:
> On 17.03.2025 14:34, Andrew Cooper wrote:
>> On 17/03/2025 9:09 am, Jan Beulich wrote:
>>> On 14.03.2025 21:49, Andrew Cooper wrote:
>>>> ... which is more consise than the opencoded form, and more efficient when
>>>> compiled.
>>>>
>>>> For production VMs, ~100% of emulations are simple MOVs, so it is likely 
>>>> that
>>>> there are no segments to write back.
>>>>
>>>> Furthermore, now that find_{first,next}_bit() are no longer in use, the
>>>> seg_reg_{accessed,dirty} fields aren't forced to be unsigned long, although
>>>> they do need to remain unsigned int because of __set_bit() elsewhere.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>>> I still can't persuade GCC to do the early exit prior to establishing the
>>>> stack frame, and unlike do_livepatch_work(), it's not critical enough to
>>>> require noinline games.
>>> Then is ...
>>>
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -3022,18 +3022,16 @@ void hvm_emulate_init_per_insn(
>>>>  void hvm_emulate_writeback(
>>>>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>>>>  {
>>>> -    enum x86_segment seg;
>>>> +    struct vcpu *curr;
>>>> +    unsigned int dirty = hvmemul_ctxt->seg_reg_dirty;
>>>>  
>>>> -    seg = find_first_bit(&hvmemul_ctxt->seg_reg_dirty,
>>>> -                         ARRAY_SIZE(hvmemul_ctxt->seg_reg));
>>>> +    if ( likely(!dirty) )
>>>> +        return;
>>> ... this worthwhile at all? I'm surprised anyway that I see you use likely()
>>> here, when generally you argue against its use.
>> No, it's not worth it.  In fact, simplifying makes the function smaller.
>>
>> void hvm_emulate_writeback(
>>     struct hvm_emulate_ctxt *hvmemul_ctxt)
>> {
>>     struct vcpu *curr = current;
>>     unsigned int dirty = hvmemul_ctxt->seg_reg_dirty;
>>
>>     for_each_set_bit ( seg, dirty )
>>         hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
>> }
>>
>> gets a bloat-o-meter score of 131 down to 72 (-59).
> That's surprisingly much.

Indeed.  https://termbin.com/gzd7 is the full side-by-side diff.

I think the key is the removal of ARRAY_SIZE().  With it,
find_first_bit() is emitting code to deal with seg_reg_dirty having
higher bits set.  Also, both BSF expressions have unknown zero-ness, so
are emitting the cmov form (__scanbit() is still on my list of bitops to
work on).

~Andrew