On 7/11/20 2:19 PM, Paolo Bonzini wrote: > 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.
Yes, absolutely this is what I wanted to express. I would like to find out what the problem is that appears in s390, I am not sure though that it is actually an s390-specific problem, it could even be a migration qemu-file issue, as apparently just flushing with qemu_fflush(f) "fixes" it. My patch made the stream a bit smaller, and changed the layout of the s390-skeys, which have an interesting field length (32768), I wonder if I got just the right alignment to trigger a bug where the qemu-file buffer is not properly flushed. > > (And by the way, my suggested patch to icount_enabled() was completely > wrong!). > > Paolo > > We will come back to this later, thanks a lot for the exchange! Ciao, Claudio