All of lore.kernel.org
 help / color / mirror / Atom feed
From: Aaron Lindsay via <qemu-devel@nongnu.org>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: qemu-devel@nongnu.org, cota@braap.org, kuhn.chenqun@huawei.com,
	robhenry@microsoft.com, mahmoudabdalghany@outlook.com,
	Richard Henderson <richard.henderson@linaro.org>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH  v2 20/21] accel/tcg: allow plugin instrumentation to be disable via cflags
Date: Fri, 12 Feb 2021 09:31:12 -0500	[thread overview]
Message-ID: <YCaRMAPlhBLbDIDZ@strawberry.localdomain> (raw)
In-Reply-To: <87a6s9v7ia.fsf@linaro.org>

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.

> > 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


  reply	other threads:[~2021-02-12 14:32 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-10 22:10 [PATCH v2 00/21] plugins/next pre-PR (hwprofile, regression fixes, icount count fix) Alex Bennée
2021-02-10 22:10 ` [PATCH v2 01/21] hw/virtio/pci: include vdev name in registered PCI sections Alex Bennée
2021-02-10 22:10 ` [PATCH v2 02/21] plugins: add API to return a name for a IO device Alex Bennée
2021-02-10 22:10 ` [PATCH v2 03/21] plugins: new hwprofile plugin Alex Bennée
2021-02-10 22:10 ` [PATCH v2 04/21] contrib: Don't use '#' flag of printf format Alex Bennée
2021-02-10 22:10 ` [PATCH v2 05/21] contrib: Fix some code style problems, ERROR: "foo * bar" should be "foo *bar" Alex Bennée
2021-02-10 22:10 ` [PATCH v2 06/21] contrib: Add spaces around operator Alex Bennée
2021-02-10 22:10 ` [PATCH v2 07/21] contrib: space required after that ',' Alex Bennée
2021-02-10 22:10 ` [PATCH v2 08/21] contrib: Open brace '{' following struct go on the same line Alex Bennée
2021-02-10 22:10 ` [PATCH v2 09/21] accel/tcg/plugin-gen: fix the call signature for inline callbacks Alex Bennée
2021-02-10 22:10 ` [PATCH v2 10/21] exec: Move TranslationBlock typedef to qemu/typedefs.h Alex Bennée
2021-02-11 10:14   ` Philippe Mathieu-Daudé
2021-02-11 10:24     ` Alex Bennée
2021-02-10 22:10 ` [PATCH v2 11/21] accel/tcg: Create io_recompile_replay_branch hook Alex Bennée
2021-02-11 10:12   ` Philippe Mathieu-Daudé
2021-02-10 22:10 ` [PATCH v2 12/21] target/mips: Create mips_io_recompile_replay_branch Alex Bennée
2021-02-11 10:10   ` Philippe Mathieu-Daudé
2021-02-10 22:10 ` [PATCH v2 13/21] target/sh4: Create superh_io_recompile_replay_branch Alex Bennée
2021-02-11 10:13   ` Philippe Mathieu-Daudé
2021-02-10 22:10 ` [PATCH v2 14/21] tests/plugin: expand insn test to detect duplicate instructions Alex Bennée
2021-02-10 22:10 ` [PATCH v2 15/21] tests/acceptance: add a new set of tests to exercise plugins Alex Bennée
2021-02-11 10:31   ` Philippe Mathieu-Daudé
2021-02-11 18:59     ` Wainer dos Santos Moschetta
2021-02-11 19:51   ` Wainer dos Santos Moschetta
2021-02-10 22:10 ` [PATCH v2 16/21] accel/tcg: actually cache our partial icount TB Alex Bennée
2021-02-11 10:21   ` Philippe Mathieu-Daudé
2021-02-11 18:48     ` Richard Henderson
2021-02-12 15:40       ` Philippe Mathieu-Daudé
2021-02-12 17:06         ` Alex Bennée
2021-02-11 18:48   ` Richard Henderson
2021-02-10 22:10 ` [PATCH v2 17/21] accel/tcg: cache single instruction TB on pending replay exception Alex Bennée
2021-02-11 19:12   ` Richard Henderson
2021-02-11 20:00     ` Alex Bennée
2021-02-10 22:10 ` [PATCH v2 18/21] accel/tcg: re-factor non-RAM execution code Alex Bennée
2021-02-11 19:19   ` Richard Henderson
2021-02-10 22:10 ` [PATCH v2 19/21] accel/tcg: remove CF_NOCACHE and special cases Alex Bennée
2021-02-10 22:10 ` [PATCH v2 20/21] accel/tcg: allow plugin instrumentation to be disable via cflags Alex Bennée
2021-02-12  0:53   ` Aaron Lindsay via
2021-02-12 11:22     ` Alex Bennée
2021-02-12 14:31       ` Aaron Lindsay via [this message]
2021-02-12 14:59         ` Alex Bennée
2021-02-12 14:43     ` Alex Bennée
2021-02-12 15:41       ` Aaron Lindsay via
2021-02-12 16:04         ` Alex Bennée
2021-02-12 16:50           ` Aaron Lindsay via
2021-02-12 17:19             ` Alex Bennée
2021-02-16 10:34             ` Alex Bennée
2021-02-17 16:32               ` Aaron Lindsay via
2021-02-12 16:00       ` Alex Bennée
2021-02-12 17:04         ` Aaron Lindsay via
2021-02-10 22:10 ` [PATCH v2 21/21] tests/acceptance: add a new tests to detect counting errors Alex Bennée
2021-02-11 10:24   ` Philippe Mathieu-Daudé
2021-02-11 19:56   ` Wainer dos Santos Moschetta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YCaRMAPlhBLbDIDZ@strawberry.localdomain \
    --to=qemu-devel@nongnu.org \
    --cc=aaron@os.amperecomputing.com \
    --cc=alex.bennee@linaro.org \
    --cc=cota@braap.org \
    --cc=kuhn.chenqun@huawei.com \
    --cc=mahmoudabdalghany@outlook.com \
    --cc=pbonzini@redhat.com \
    --cc=richard.henderson@linaro.org \
    --cc=robhenry@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.