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

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Claudio Fontana" <cfontana@suse.de>,
	"Thomas Huth" <thuth@redhat.com>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	"Markus Armbruster" <armbru@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Colin Xu <colin.xu@intel.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	qemu-devel@nongnu.org, haxm-team@intel.com,
	Wenchao Wang <wenchao.wang@intel.com>,
	Sunil Muthuswamy <sunilmut@microsoft.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 3/3] cpu-timers, icount: new modules
Date: Sat, 11 Jul 2020 14:19:10 +0200	[thread overview]
Message-ID: <22228280-f3b4-3f64-d2ba-30cfc47c8b0d@redhat.com> (raw)
In-Reply-To: <9630c685-0a37-a1e7-4614-9d692988a799@suse.de>

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