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

From: Claudio Fontana <cfontana@suse.de>
To: "Paolo Bonzini" <pbonzini@redhat.com>,
	"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: Wed, 8 Jul 2020 17:00:14 +0200	[thread overview]
Message-ID: <f89f249d-dbc4-779b-5b53-fc408461f072@suse.de> (raw)
In-Reply-To: <aa45a793-35b1-d3bd-18a8-4c52ad888029@redhat.com>

On 7/8/20 4:34 PM, Paolo Bonzini wrote:
> On 29/06/20 11:35, Claudio Fontana wrote:
>> refactoring of cpus.c continues with cpu timer state extraction.
>>
>> cpu-timers: responsible for the cpu timers state, and for access to
>> cpu clocks and ticks.
>>
>> icount: counts the TCG instructions executed. As such it is specific to
>> the TCG accelerator. Therefore, it is built only under CONFIG_TCG.
>>
>> One complication is due to qtest, which misuses icount to warp time
>> (qtest_clock_warp). In order to solve this problem, detach instead qtest
>> from icount, and use a trivial separate counter for it.
>>
>> This requires fixing assumptions scattered in the code that
>> qtest_enabled() implies icount_enabled().
>>
>> No functionality change.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 
> Claudio,
> 
> this weirdly enough causes iotest 267 (i.e. basically vmstate
> save/restore) to break on s390:
> 
> +Unexpected storage key flag data: 0
> +error while loading state for instance 0x0 of device 's390-skeys'
> +Error: Error -22 while loading VM state
> 
> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
> multiple parts, specifically separating any rename or introducing of
> includes from the final file move?

Hi Paolo,

will take a look!

Is this captured by some travis / cirrus-ci / anything I can easily see the result of?

> 
> Also, the patch breaks --disable-tcg, which is easily fixed by changing
> the prototype for icount_enabled() to
> 
> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
> 	extern bool icount_enabled(void);
> 	#else
> 	#define icount_enabled() 0
> 	#endif
> 
> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
> this change in the next version.
> 
> Paolo
> 

Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).

Will take a look at the introduction of this #defines in place of variables,
as this mechanisms will not work in the future for target-specific modules.

Ciao, will let you know what I find,

Claudio