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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.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.

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