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

[Claudio Fontana <cfontana@suse.de>]

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