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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2024 15:57, Andrew Cooper wrote:
> ... which is more consise than the opencoded form.
> 
> Also, 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 practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Pulling current out into curr is good for code generation.  When using current
> in the loop, GCC can't retain the calculation across the call to
> hvm_set_segment_register() and is forced to re-read from the cpu_info block.
> 
> However, if curr is initialised, it's calculated even in the likely path...

That's a little odd, as I don't think I can spot what would force the compiler
into doing so. As a wild guess, ...

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2908,18 +2908,18 @@ 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;

... is the order of these two possibly relevant? Yet of course it's not the
end of the world whichever way it's done.

> -    seg = find_first_bit(&hvmemul_ctxt->seg_reg_dirty,
> -                         ARRAY_SIZE(hvmemul_ctxt->seg_reg));
> +    if ( likely(!dirty) )
> +        return;
>  
> -    while ( seg < ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
> -    {
> -        hvm_set_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
> -        seg = find_next_bit(&hvmemul_ctxt->seg_reg_dirty,
> -                            ARRAY_SIZE(hvmemul_ctxt->seg_reg),
> -                            seg+1);
> -    }
> +    curr = current;
> +
> +    for_each_set_bit ( seg, dirty )
> +        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
> +
> +    hvmemul_ctxt->seg_reg_dirty = 0;

Why is this suddenly appearing here? You don't mention it in the description,
so it's not clear whether you found a (however minor) issue, or whether
that's purely cosmetic (yet then it's still an extra store we could do
without).

Jan