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