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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan