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 <[email protected]>
> ---
> CC: Jan Beulich <[email protected]>
> CC: Roger Pau Monné <[email protected]>
>
> 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
