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

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> 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.
>>
>> 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?
>
> Can you try the following fugly patch on top of this series:
>
<snip>
> @@ -120,8 +128,13 @@ void qemu_plugin_register_vcpu_mem_cb(struct 
> qemu_plugin_insn *insn,
>                                        enum qemu_plugin_mem_rw rw,
>                                        void *udata)
>  {
> -    plugin_register_vcpu_mem_cb(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR],
> -                                cb, flags, rw, udata);
> +    if (insn->store_only && (rw & QEMU_PLUGIN_MEM_W)) {
> +        
> plugin_register_vcpu_mem_cb(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR],
> +                                    cb, flags, QEMU_PLUGIN_MEM_W, udata);
> +    } else {
> +        
> plugin_register_vcpu_mem_cb(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR],
> +                                    cb, flags, rw, udata);
> +    }
>  }
<snip>

Actually I'm wondering if I've got my sense the wrong way around. Should
it be loads only:

  void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn,
                                        qemu_plugin_vcpu_mem_cb_t cb,
                                        enum qemu_plugin_cb_flags flags,
                                        enum qemu_plugin_mem_rw rw,
                                        void *udata)
  {
      if (insn->store_only && (rw & QEMU_PLUGIN_MEM_R)) {
          
plugin_register_vcpu_mem_cb(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR],
                                      cb, flags, QEMU_PLUGIN_MEM_R, udata);
      } else {
          
plugin_register_vcpu_mem_cb(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR],
                                      cb, flags, rw, udata);
      }
  }


obviously I'd have to rename the variables :-/

-- 
Alex Bennée