Re: [PATCH v2 20/21] accel/tcg: allow plugin instrumentation to be disable via cflags

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> On Feb 12 11:22, Alex Bennée wrote:
>> Aaron Lindsay <aaron@os.amperecomputing.com> writes:
>> > On Feb 10 22:10, Alex Bennée wrote:
>> >> When icount is enabled and we recompile an MMIO access we end up
>> >> double counting the instruction execution. To avoid this we introduce
>> >> the CF_NOINSTR cflag which disables instrumentation for the next TB.
>> >> As this is part of the hashed compile flags we will only execute the
>> >> generated TB while coming out of a cpu_io_recompile.
>> >
>> > Unfortunately this patch works a little too well!
>> >
>> > With this change, the memory access callbacks registered via
>> > `qemu_plugin_register_vcpu_mem_cb()` are never called for the
>> > re-translated instruction making the IO access, since we've disabled all
>> > instrumentation.
>> 
>> Hmm well we correctly don't instrument stores (as we have already
>> executed the plugin for them) - but of course the load instrumentation
>> is after the fact so we are now missing them.
>
> I do not believe I am seeing memory callbacks for stores, either. Are
> you saying I definitely should be?
>
> My original observation was that the callbacks for store instructions to
> IO followed the same pattern as loads:
>
> 1) Initial instruction callback (presumably as part of larger block)
> 2) Second instruction callback (presumably as part of single-instruction 
> block)
> 3) Memory callback (presumably as part of single-instruction block)
>
> After applying v2 of your patchset I now see only 1), even for stores.

Right - but any pre-instruction instrumentation shouldn't be done in the
(now badly names CF_NOINSTR) case. It's also confusing because we have
pre and post helpers and inline callbacks are always pre (you can only
count so don't see data).

Can you check the patch in my other email and see if that works better?

>
>> > Is it possible to selectively disable only instruction callbacks using
>> > this mechanism, while still allowing others that would not yet have been
>> > called for the re-translated instruction?
>> 
>> Hmmm let me see if I can finesse the CF_NOINSTR logic to allow
>> plugin_gen_insn_end() without the rest? It probably needs a better name
>> for the flag as well. 
>
> Funny, the first time reading through this patch I was unsure for a
> second whether "CF_NOINSTR" stood for "NO INSTRuction callbacks" or "NO
> INSTRumentation"!
>
> -Aaron


-- 
Alex Bennée