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
