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
