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

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/07/20 13:49, Claudio Fontana wrote:
>> Apart from the name, icount is more like deterministic execution than
> 
> Maybe we should start choosing names more carefully in a way to express what we mean?

I don't disagree.  For icount in particular however we're about 12 years
too late.

>>  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:
> 
> I think that the existing code in master conflates them together actually.
> Qtest can have its own counter, it does not need to be the icount
> instruction counter.

If you want you can add to your accelerator ops series one for
qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
qemu_start_warp_timer(), that would certainly work for me; those three
are the only non-TCG-specific functions that read use_icount, as far as
I can see.  qemu_start_warp_timer() does have an "if (qtest_enabled())"
even, so it's clearly fishy.

It may even be a good idea for TCG to have three sets of accelerator ops
for respectively multi-threaded, round-robin and icount.

My point is that this patch is not the right way to start the
refactoring because *for now* it's wrong to treat icount as a TCG-only
concept.  Having more separation between accelerators, as well as a
clear interface between core and accelerators is certainly a laudable
goal though.

>> - 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.
> 
> I don't see how this follows, how is using a global use_icount variable better than having this checked using icount_enabled()?

If you can get rid of use_icount using a new accelerator ops member, it
would be even better. :)

> I will come back to this later on, this patch seems to have uncovered an underlying issue, which shows on s390.
> 
> I'd rather now continue investigating that, choosing to try to
> actually understand the issue, rather than hiding it under the
> carpet.

Thanks.  But I don't think it's sweeping anything under the carpet; it's
great if we find a currently latent s390 bug, but it is orthogonal to
the design of that core<->accelerator interface.

(And by the way, my suggested patch to icount_enabled() was completely
wrong!).

Paolo