On 27/08/2024 5:07 pm, Jan Beulich wrote:
> 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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>>
>> 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.
My general conclusion is that GCC doesn't try very hard to optimise
likely() early exits.
I suspect there's some reasonably low hanging fruit to be found there.
>
>> - 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).
Oh, yes. Nothing anywhere in Xen ever clears these segment dirty bits.
I suspect the worst that will go wrong is that we'll waste time
re-{VMWRITE,memcpy}-ing the segment registers into the VMCS/VMCB, but
the logic in Xen is definitely not right.
~Andrew
