Re: [PATCH 3/3] cpu-timers, icount: new modules

[Paolo Bonzini]

On 11/07/20 11:14, Claudio Fontana wrote:
> On 7/11/20 12:45 AM, Paolo Bonzini wrote:
>> On 10/07/20 06:36, Thomas Huth wrote:
>>>
>>> In short this goes away if I again set icount to enabled for qtest,
>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>
>>> qtest was forcing icount and shift=0 by creating qemu options, in order to 
>>> misuse its counter feature,
>>> instead of using a separate counter.
>>
>> Why would it need a separate counter?  In both cases it's a
>> manually-updated counter that is used for QEMU_CLOCK_VIRTUAL.  The only
>> difference is that shift > 0 doesn't make sense for qtest.
> 
> I think I would reverse the question. Why reuse for qtest a counter that has 
> absolutely nothing to do with it?
> 
> qtest has nothing to do with instruction counting.

Apart from the name, icount is more like deterministic execution than
instruction counting (it's not a coincidence that record/replay is
fundamentally based on icount).  qtests need to be deterministic and
describe which qtest instructions run before a given timer fires and
which run after.

And in both cases, determinism is achieved by controlling the
advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
icount that is shared by qtest and TCG, and I think the problem is that
this patch conflates all of them together:

- the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
qemu-timer and should not be carved out into a separate module.  This
includes the use_icount variable, which should be kept in core QEMU code.

- the fact qtest uses -icount instead of configuring the variables
directly is definitely a hack and can be removed.

- the adaptive frequency adjustment is definitely TCG specific, and so
are the particular functions in cpus.c that test icount_enabled() and
broke with this patch.  All this code should be included in the TCG
module only or, before that, should be made conditional on $(CONFIG_TCG).

So I think this patch should have been the last, not the first. :)  Once
you move all the accelerator runtime code from cpus.c to separate files,
it will be possible to move the frequency adjustment and deadline
management code into accel/tcg.  And then it will be obvious which code
is not TCG-specific and can be extracted for convenience into a
cpu-timers.c file.

Thanks,

Paolo