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

[Jan Beulich <jbeulich@suse.com>]

On 28.08.2024 20:56, Andrew Cooper wrote:
> On 28/08/2024 3:56 pm, Jan Beulich wrote:
>> On 28.08.2024 16:44, Andrew Cooper wrote:
>>> On 27/08/2024 5:07 pm, Jan Beulich wrote:
>>>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>>>> +    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.
>> hvm_emulate_init_once()?
> 
> I meant after emulation.  The value is initialised to 0 at the start of day.
> 
>>
>>> 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.
>> I'm on the edge of asking to do such clearing before emulation, not after
>> processing the dirty bits. That would then be hvm_emulate_init_per_insn(),
>> well centralized.
> 
> Specifically, hvmemul_ctxt should not believe itself to be dirty after a
> call to hvm_emulate_writeback(), because that's the logic to make the
> context no-longer-dirty.

That's one aspect, yes. Debuggability is another. For that retaining state
until it strictly needs clearing out may be helpful. Plus ...

> That said, the more I look at this, the less convinced I am by it.  For
> a function named writeback(), it's doing a very narrow thing that is not
> the usual meaning of the term when it comes to pipelines or insn
> emulation...

... as you say here.

Anyway - I'll leave where to put the clearing to you, just as long as it's
at least mentioned in the description.

Jan