trace_FOO_tcg bit-rotted?

Message ID 87eefnwd0l.fsf@linaro.org
State New
Headers show
Series
  • trace_FOO_tcg bit-rotted?
Related show

Commit Message

Alex Bennée April 6, 2021, 4 p.m.
Hi,

It's been awhile since I last played with this but I think we are
suffering from not having some test cases for tracing code
generation/execution in the tree. I tried adding a simple trace point to
see if I could track ERET calls:

--8<---------------cut here---------------start------------->8---
--8<---------------cut here---------------end--------------->8---

According to the tracing docs I the:

  trace_eret_tcg(s->current_el, dst);

Should:

  Instead of using these two events, you should instead use the function
  "trace_<eventname>_tcg" during translation (TCG code generation). This function
  will automatically call "trace_<eventname>_trans", and will generate the
  necessary TCG code to call "trace_<eventname>_exec" during guest code execution.

But it falls down with the following:

  ../../target/arm/translate-a64.c: In function ‘disas_uncond_b_reg’:
  ../../target/arm/translate-a64.c:2307:9: error: implicit declaration of function ‘trace_eret_tcg’; did you mean ‘trace_eret_exec’? [-Werror=implicit-function-declaration]
           trace_eret_tcg(s->current_el, dst);
           ^~~~~~~~~~~~~~
           trace_eret_exec
  ../../target/arm/translate-a64.c:2307:9: error: nested extern declaration of ‘trace_eret_tcg’ [-Werror=nested-externs]
  cc1: all warnings being treated as errors
  ninja: build stopped: subcommand failed.

So I'm wondering what needs to be done to fix this? Given the one other
tracepoint is in the general tcg-op.c is this just some build stuff to
do with how the tracepoint segments are generated?

-- 
Alex Bennée

Comments

no-reply@patchew.org April 6, 2021, 5:24 p.m. | #1
Patchew URL: https://patchew.org/QEMU/87eefnwd0l.fsf@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 87eefnwd0l.fsf@linaro.org
Subject: trace_FOO_tcg bit-rotted?

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210329150129.121182-1-sgarzare@redhat.com -> patchew/20210329150129.121182-1-sgarzare@redhat.com
 * [new tag]         patchew/87eefnwd0l.fsf@linaro.org -> patchew/87eefnwd0l.fsf@linaro.org
Switched to a new branch 'test'
800b730 trace_FOO_tcg bit-rotted?

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 26 lines checked

Commit 800b730c849f (trace_FOO_tcg bit-rotted?) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/87eefnwd0l.fsf@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Laurent Vivier April 7, 2021, 9:08 a.m. | #2
Le 06/04/2021 à 18:00, Alex Bennée a écrit :
> Hi,

> 

> It's been awhile since I last played with this but I think we are

> suffering from not having some test cases for tracing code

> generation/execution in the tree. I tried adding a simple trace point to

> see if I could track ERET calls:

> 

> --8<---------------cut here---------------start------------->8---

> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

> index 0b42e53500..0d643f78fe 100644

> --- a/target/arm/translate-a64.c

> +++ b/target/arm/translate-a64.c

> @@ -36,6 +36,7 @@

>  #include "exec/log.h"

>  

>  #include "trace-tcg.h"

> +#include "trace.h"

>  #include "translate-a64.h"

>  #include "qemu/atomic128.h"

>  

> @@ -2302,6 +2303,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)

>          default:

>              goto do_unallocated;

>          }

> +

> +        trace_eret_tcg(s->current_el, dst);

> +

>          if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {

>              gen_io_start();

>          }

> diff --git a/target/arm/trace-events b/target/arm/trace-events

> index 41c63d7570..2d4fca16a1 100644

> --- a/target/arm/trace-events

> +++ b/target/arm/trace-events

> @@ -1,5 +1,10 @@

>  # See docs/devel/tracing.txt for syntax documentation.

>  

> +# translate-a64.c

> +# Mode: softmmu

> +# Targets: TCG(aarch64-softmmu)

> +tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", "exec_eret: EL%d to EL%"PRId64


If I read correctly, the name should be eret_tcg()
And I'm not sure TCGv will be accepted as a parameter type, use uint64_t instead (and %PRIu64)

Thanks,
Laurent
Alex Bennée April 9, 2021, 4:29 p.m. | #3
Laurent Vivier <laurent@vivier.eu> writes:

> Le 06/04/2021 à 18:00, Alex Bennée a écrit :

>> Hi,

>> 

>> It's been awhile since I last played with this but I think we are

>> suffering from not having some test cases for tracing code

>> generation/execution in the tree. I tried adding a simple trace point to

>> see if I could track ERET calls:

>> 

>> --8<---------------cut here---------------start------------->8---

>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

>> index 0b42e53500..0d643f78fe 100644

>> --- a/target/arm/translate-a64.c

>> +++ b/target/arm/translate-a64.c

>> @@ -36,6 +36,7 @@

>>  #include "exec/log.h"

>>  

>>  #include "trace-tcg.h"

>> +#include "trace.h"

>>  #include "translate-a64.h"

>>  #include "qemu/atomic128.h"

>>  

>> @@ -2302,6 +2303,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)

>>          default:

>>              goto do_unallocated;

>>          }

>> +

>> +        trace_eret_tcg(s->current_el, dst);

>> +

>>          if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {

>>              gen_io_start();

>>          }

>> diff --git a/target/arm/trace-events b/target/arm/trace-events

>> index 41c63d7570..2d4fca16a1 100644

>> --- a/target/arm/trace-events

>> +++ b/target/arm/trace-events

>> @@ -1,5 +1,10 @@

>>  # See docs/devel/tracing.txt for syntax documentation.

>>  

>> +# translate-a64.c

>> +# Mode: softmmu

>> +# Targets: TCG(aarch64-softmmu)

>> +tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", "exec_eret: EL%d to EL%"PRId64

>

> If I read correctly, the name should be eret_tcg()

> And I'm not sure TCGv will be accepted as a parameter type, use

> uint64_t instead (and %PRIu64)


This was my confusion. I thought the trace-events file was prefixed with
tcg like guest_mem_before:

  vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", "vaddr=0x%016"PRIx64" info=%d"

and that signalled the tools to generate _trans, _exec and _tcg hooks in
the generated files. The trace code (see other patch) also has logic to
translate natural TCG types into the natives types as well signalling
which values are only visible for the _exec portion.

Maybe I'm over thinking this. Perhaps all the TCG tracing use cases are
just as easily supported with TCG plugins now and we should deprecate
this unused bit of complexity. I certainly understand the plugin
interactions better ;-)

>

> Thanks,

> Laurent



-- 
Alex Bennée
Stefan Hajnoczi April 12, 2021, 3 p.m. | #4
On Fri, Apr 09, 2021 at 05:29:08PM +0100, Alex Bennée wrote:
> 

> Laurent Vivier <laurent@vivier.eu> writes:

> 

> > Le 06/04/2021 à 18:00, Alex Bennée a écrit :

> >> Hi,

> >> 

> >> It's been awhile since I last played with this but I think we are

> >> suffering from not having some test cases for tracing code

> >> generation/execution in the tree. I tried adding a simple trace point to

> >> see if I could track ERET calls:

> >> 

> >> --8<---------------cut here---------------start------------->8---

> >> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

> >> index 0b42e53500..0d643f78fe 100644

> >> --- a/target/arm/translate-a64.c

> >> +++ b/target/arm/translate-a64.c

> >> @@ -36,6 +36,7 @@

> >>  #include "exec/log.h"

> >>  

> >>  #include "trace-tcg.h"

> >> +#include "trace.h"

> >>  #include "translate-a64.h"

> >>  #include "qemu/atomic128.h"

> >>  

> >> @@ -2302,6 +2303,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)

> >>          default:

> >>              goto do_unallocated;

> >>          }

> >> +

> >> +        trace_eret_tcg(s->current_el, dst);

> >> +

> >>          if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {

> >>              gen_io_start();

> >>          }

> >> diff --git a/target/arm/trace-events b/target/arm/trace-events

> >> index 41c63d7570..2d4fca16a1 100644

> >> --- a/target/arm/trace-events

> >> +++ b/target/arm/trace-events

> >> @@ -1,5 +1,10 @@

> >>  # See docs/devel/tracing.txt for syntax documentation.

> >>  

> >> +# translate-a64.c

> >> +# Mode: softmmu

> >> +# Targets: TCG(aarch64-softmmu)

> >> +tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", "exec_eret: EL%d to EL%"PRId64

> >

> > If I read correctly, the name should be eret_tcg()

> > And I'm not sure TCGv will be accepted as a parameter type, use

> > uint64_t instead (and %PRIu64)

> 

> This was my confusion. I thought the trace-events file was prefixed with

> tcg like guest_mem_before:

> 

>   vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", "vaddr=0x%016"PRIx64" info=%d"

> 

> and that signalled the tools to generate _trans, _exec and _tcg hooks in

> the generated files. The trace code (see other patch) also has logic to

> translate natural TCG types into the natives types as well signalling

> which values are only visible for the _exec portion.

> 

> Maybe I'm over thinking this. Perhaps all the TCG tracing use cases are

> just as easily supported with TCG plugins now and we should deprecate

> this unused bit of complexity. I certainly understand the plugin

> interactions better ;-)


Lluís: are you happy to deprecate tcg trace events in favor of TCG
plugins?

My question is whether TCG plugins are really equivalent here. Will TCG
plugin users have to write their own log file output code to extract
this information from the QEMU process (i.e. reinventing tracing)? Is
the performance at least as good as tracing?

Stefan
Alex Bennée April 12, 2021, 7:06 p.m. | #5
Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Apr 09, 2021 at 05:29:08PM +0100, Alex Bennée wrote:

>> 

>> Laurent Vivier <laurent@vivier.eu> writes:

>> 

>> > Le 06/04/2021 à 18:00, Alex Bennée a écrit :

>> >> Hi,

>> >> 

>> >> It's been awhile since I last played with this but I think we are

>> >> suffering from not having some test cases for tracing code

>> >> generation/execution in the tree. I tried adding a simple trace point to

>> >> see if I could track ERET calls:

>> >> 

>> >> --8<---------------cut here---------------start------------->8---

>> >> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

>> >> index 0b42e53500..0d643f78fe 100644

>> >> --- a/target/arm/translate-a64.c

>> >> +++ b/target/arm/translate-a64.c

>> >> @@ -36,6 +36,7 @@

>> >>  #include "exec/log.h"

>> >>  

>> >>  #include "trace-tcg.h"

>> >> +#include "trace.h"

>> >>  #include "translate-a64.h"

>> >>  #include "qemu/atomic128.h"

>> >>  

>> >> @@ -2302,6 +2303,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)

>> >>          default:

>> >>              goto do_unallocated;

>> >>          }

>> >> +

>> >> +        trace_eret_tcg(s->current_el, dst);

>> >> +

>> >>          if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {

>> >>              gen_io_start();

>> >>          }

>> >> diff --git a/target/arm/trace-events b/target/arm/trace-events

>> >> index 41c63d7570..2d4fca16a1 100644

>> >> --- a/target/arm/trace-events

>> >> +++ b/target/arm/trace-events

>> >> @@ -1,5 +1,10 @@

>> >>  # See docs/devel/tracing.txt for syntax documentation.

>> >>  

>> >> +# translate-a64.c

>> >> +# Mode: softmmu

>> >> +# Targets: TCG(aarch64-softmmu)

>> >> +tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", "exec_eret: EL%d to EL%"PRId64

>> >

>> > If I read correctly, the name should be eret_tcg()

>> > And I'm not sure TCGv will be accepted as a parameter type, use

>> > uint64_t instead (and %PRIu64)

>> 

>> This was my confusion. I thought the trace-events file was prefixed with

>> tcg like guest_mem_before:

>> 

>>   vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", "vaddr=0x%016"PRIx64" info=%d"

>> 

>> and that signalled the tools to generate _trans, _exec and _tcg hooks in

>> the generated files. The trace code (see other patch) also has logic to

>> translate natural TCG types into the natives types as well signalling

>> which values are only visible for the _exec portion.

>> 

>> Maybe I'm over thinking this. Perhaps all the TCG tracing use cases are

>> just as easily supported with TCG plugins now and we should deprecate

>> this unused bit of complexity. I certainly understand the plugin

>> interactions better ;-)

>

> Lluís: are you happy to deprecate tcg trace events in favor of TCG

> plugins?

>

> My question is whether TCG plugins are really equivalent here. Will TCG

> plugin users have to write their own log file output code to extract

> this information from the QEMU process (i.e. reinventing tracing)?


Yes - although there is no reason we couldn't expose the trace output as
a helper. Currently there is:

  /**
   * qemu_plugin_outs() - output string via QEMU's logging system
   * @string: a string
   */
  void qemu_plugin_outs(const char *string);

which allows the user to echo to the log in conjunction with -d plugin
on the command line. Plugins are of course free to do there own thing.

> Is

> the performance at least as good as tracing?


Well like all things that depends ;-)

Generally on the sort of events you tend to trace (like the example
memory access) you usually either want to aggregate or filter your
results. With trace output their is no way to do this and you end up
post processing potentially very large files (smaller if you use the
non-default binary format). I don't know if a similar thing is possible
with uprobes and ebpf - I've only ever used the simple logging output in
anger. The example plugins generally do things like count total
accesses:

  https://gitlab.com/qemu-project/qemu/-/blob/master/tests/plugin/mem.c

(pretty fast in comparison to writing out reams of data)

or aggregate the results:

  https://gitlab.com/qemu-project/qemu/-/blob/master/contrib/plugins/hotpages.c

(probably slower while running QEMU, but faster overall because no post
processing of log files required.)

Of course plugins are a non-default build option because although light
it does have a small performance impact on code generation even when no
instrumentation is occurring. As a result you can't use it without
building a version first.

If we had test code in the build that used the TCG tracing abilities I
wouldn't worry because at least we are defending the feature and we
wouldn't run into problems like the above. At least if plugins get
broken we'll know about it due to the fairly thorough workout on CI, e.g.:

  https://gitlab.com/qemu-project/qemu/-/jobs/1172484753#L3458

-- 
Alex Bennée
Stefan Hajnoczi April 13, 2021, 8:33 a.m. | #6
On Mon, Apr 12, 2021 at 08:06:57PM +0100, Alex Bennée wrote:
> 

> Stefan Hajnoczi <stefanha@redhat.com> writes:

> 

> > On Fri, Apr 09, 2021 at 05:29:08PM +0100, Alex Bennée wrote:

> >> 

> >> Laurent Vivier <laurent@vivier.eu> writes:

> >> 

> >> > Le 06/04/2021 à 18:00, Alex Bennée a écrit :

> >> >> Hi,

> >> >> 

> >> >> It's been awhile since I last played with this but I think we are

> >> >> suffering from not having some test cases for tracing code

> >> >> generation/execution in the tree. I tried adding a simple trace point to

> >> >> see if I could track ERET calls:

> >> >> 

> >> >> --8<---------------cut here---------------start------------->8---

> >> >> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

> >> >> index 0b42e53500..0d643f78fe 100644

> >> >> --- a/target/arm/translate-a64.c

> >> >> +++ b/target/arm/translate-a64.c

> >> >> @@ -36,6 +36,7 @@

> >> >>  #include "exec/log.h"

> >> >>  

> >> >>  #include "trace-tcg.h"

> >> >> +#include "trace.h"

> >> >>  #include "translate-a64.h"

> >> >>  #include "qemu/atomic128.h"

> >> >>  

> >> >> @@ -2302,6 +2303,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)

> >> >>          default:

> >> >>              goto do_unallocated;

> >> >>          }

> >> >> +

> >> >> +        trace_eret_tcg(s->current_el, dst);

> >> >> +

> >> >>          if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {

> >> >>              gen_io_start();

> >> >>          }

> >> >> diff --git a/target/arm/trace-events b/target/arm/trace-events

> >> >> index 41c63d7570..2d4fca16a1 100644

> >> >> --- a/target/arm/trace-events

> >> >> +++ b/target/arm/trace-events

> >> >> @@ -1,5 +1,10 @@

> >> >>  # See docs/devel/tracing.txt for syntax documentation.

> >> >>  

> >> >> +# translate-a64.c

> >> >> +# Mode: softmmu

> >> >> +# Targets: TCG(aarch64-softmmu)

> >> >> +tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", "exec_eret: EL%d to EL%"PRId64

> >> >

> >> > If I read correctly, the name should be eret_tcg()

> >> > And I'm not sure TCGv will be accepted as a parameter type, use

> >> > uint64_t instead (and %PRIu64)

> >> 

> >> This was my confusion. I thought the trace-events file was prefixed with

> >> tcg like guest_mem_before:

> >> 

> >>   vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", "vaddr=0x%016"PRIx64" info=%d"

> >> 

> >> and that signalled the tools to generate _trans, _exec and _tcg hooks in

> >> the generated files. The trace code (see other patch) also has logic to

> >> translate natural TCG types into the natives types as well signalling

> >> which values are only visible for the _exec portion.

> >> 

> >> Maybe I'm over thinking this. Perhaps all the TCG tracing use cases are

> >> just as easily supported with TCG plugins now and we should deprecate

> >> this unused bit of complexity. I certainly understand the plugin

> >> interactions better ;-)

> >

> > Lluís: are you happy to deprecate tcg trace events in favor of TCG

> > plugins?

> >

> > My question is whether TCG plugins are really equivalent here. Will TCG

> > plugin users have to write their own log file output code to extract

> > this information from the QEMU process (i.e. reinventing tracing)?

> 

> Yes - although there is no reason we couldn't expose the trace output as

> a helper. Currently there is:

> 

>   /**

>    * qemu_plugin_outs() - output string via QEMU's logging system

>    * @string: a string

>    */

>   void qemu_plugin_outs(const char *string);

> 

> which allows the user to echo to the log in conjunction with -d plugin

> on the command line. Plugins are of course free to do there own thing.

> 

> > Is

> > the performance at least as good as tracing?

> 

> Well like all things that depends ;-)

> 

> Generally on the sort of events you tend to trace (like the example

> memory access) you usually either want to aggregate or filter your

> results. With trace output their is no way to do this and you end up

> post processing potentially very large files (smaller if you use the

> non-default binary format). I don't know if a similar thing is possible

> with uprobes and ebpf - I've only ever used the simple logging output in

> anger. The example plugins generally do things like count total

> accesses:

> 

>   https://gitlab.com/qemu-project/qemu/-/blob/master/tests/plugin/mem.c

> 

> (pretty fast in comparison to writing out reams of data)

> 

> or aggregate the results:

> 

>   https://gitlab.com/qemu-project/qemu/-/blob/master/contrib/plugins/hotpages.c

> 

> (probably slower while running QEMU, but faster overall because no post

> processing of log files required.)

> 

> Of course plugins are a non-default build option because although light

> it does have a small performance impact on code generation even when no

> instrumentation is occurring. As a result you can't use it without

> building a version first.

> 

> If we had test code in the build that used the TCG tracing abilities I

> wouldn't worry because at least we are defending the feature and we

> wouldn't run into problems like the above. At least if plugins get

> broken we'll know about it due to the fairly thorough workout on CI, e.g.:

> 

>   https://gitlab.com/qemu-project/qemu/-/jobs/1172484753#L3458


I can see the trade-offs being acceptable for TCG instrumentation use
cases. There is a lot of overlap between the two approaches and if Lluís
agrees we could remove the TCG trace event functionality in favor of TCG
plugins. If any documentation is missing to explain how to solve these
types of problems using TCG plugins then that should be added.

That said, I haven't used the TCG trace event functionality and maybe
I'm missing something obvious that Lluís will point out :).

Stefan
Alex Bennée April 13, 2021, 9:25 a.m. | #7
Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Mon, Apr 12, 2021 at 08:06:57PM +0100, Alex Bennée wrote:

>> 

>> Stefan Hajnoczi <stefanha@redhat.com> writes:

>> 

>> > On Fri, Apr 09, 2021 at 05:29:08PM +0100, Alex Bennée wrote:

>> >> 

>> >> Laurent Vivier <laurent@vivier.eu> writes:

>> >> 

>> >> > Le 06/04/2021 à 18:00, Alex Bennée a écrit :

>> >> >> Hi,

>> >> >> 

>> >> >> It's been awhile since I last played with this but I think we are

>> >> >> suffering from not having some test cases for tracing code

>> >> >> generation/execution in the tree. I tried adding a simple trace point to

>> >> >> see if I could track ERET calls:

<snip>
>> >

>> > Lluís: are you happy to deprecate tcg trace events in favor of TCG

>> > plugins?

<snip>
>

> That said, I haven't used the TCG trace event functionality and maybe

> I'm missing something obvious that Lluís will point out :).


I've updated the Cc to what I think is Lluís current email as I was
getting bounces from the old academic address.

-- 
Alex Bennée
Vilanova, Lluis April 23, 2021, 10:58 a.m. | #8
El dt. 13 de 04 de 2021 a les 10:25 +0100, en/na Alex Bennée va
escriure:
> Stefan Hajnoczi <stefanha@redhat.com> writes:

> 

> > On Mon, Apr 12, 2021 at 08:06:57PM +0100, Alex Bennée wrote:

> > > 

> > > Stefan Hajnoczi <stefanha@redhat.com> writes:

> > > 

> > > > On Fri, Apr 09, 2021 at 05:29:08PM +0100, Alex Bennée wrote:

> > > > > 

> > > > > Laurent Vivier <laurent@vivier.eu> writes:

> > > > > 

> > > > > > Le 06/04/2021 à 18:00, Alex Bennée a écrit :

> > > > > > > Hi,

> > > > > > > 

> > > > > > > It's been awhile since I last played with this but I

> > > > > > > think we are

> > > > > > > suffering from not having some test cases for tracing

> > > > > > > code

> > > > > > > generation/execution in the tree. I tried adding a simple

> > > > > > > trace point to

> > > > > > > see if I could track ERET calls:

> <snip>

> > > > 

> > > > Lluís: are you happy to deprecate tcg trace events in favor of

> > > > TCG

> > > > plugins?

> <snip>

> > 

> > That said, I haven't used the TCG trace event functionality and

> > maybe

> > I'm missing something obvious that Lluís will point out :).

> 

> I've updated the Cc to what I think is Lluís current email as I was

> getting bounces from the old academic address.


Hi folks, long time no write; thanks for tracking me down :)

I think it'd be great to deprecate TCG tracing in favour of the plugin
interface, pushing some of the burden out of qemu.

I haven't measured the performance of the plugin interface, but AFAIR
TCG trace prints also use TCG helpers, so there should not be a lot of
difference.

As Stefan pointed out, this means plugin writers will have to write
their own TCG tracing code. Hopefully, the plugin API can be extended
to integrate with qemu's logging backend (it seems qemu_plugin_outs
goes somewhere else), and common plugins can be kept on a separate repo
under the qemu umbrella for easier reuse (e.g., to provide the same
functionality as TCG tracing).


Cheers,
Lluis
Alex Bennée April 23, 2021, 3:14 p.m. | #9
"Vilanova, Lluis" <vilanova@imperial.ac.uk> writes:

> El dt. 13 de 04 de 2021 a les 10:25 +0100, en/na Alex Bennée va

> escriure:

>> Stefan Hajnoczi <stefanha@redhat.com> writes:

>> 

>> > On Mon, Apr 12, 2021 at 08:06:57PM +0100, Alex Bennée wrote:

>> > > 

>> > > Stefan Hajnoczi <stefanha@redhat.com> writes:

>> > > 

>> > > > On Fri, Apr 09, 2021 at 05:29:08PM +0100, Alex Bennée wrote:

>> > > > > 

>> > > > > Laurent Vivier <laurent@vivier.eu> writes:

>> > > > > 

>> > > > > > Le 06/04/2021 à 18:00, Alex Bennée a écrit :

>> > > > > > > Hi,

>> > > > > > > 

>> > > > > > > It's been awhile since I last played with this but I

>> > > > > > > think we are

>> > > > > > > suffering from not having some test cases for tracing

>> > > > > > > code

>> > > > > > > generation/execution in the tree. I tried adding a simple

>> > > > > > > trace point to

>> > > > > > > see if I could track ERET calls:

>> <snip>

>> > > > 

>> > > > Lluís: are you happy to deprecate tcg trace events in favor of

>> > > > TCG

>> > > > plugins?

>> <snip>

>> > 

>> > That said, I haven't used the TCG trace event functionality and

>> > maybe

>> > I'm missing something obvious that Lluís will point out :).

>> 

>> I've updated the Cc to what I think is Lluís current email as I was

>> getting bounces from the old academic address.

>

> Hi folks, long time no write; thanks for tracking me down :)

>

> I think it'd be great to deprecate TCG tracing in favour of the plugin

> interface, pushing some of the burden out of qemu.

>

> I haven't measured the performance of the plugin interface, but AFAIR

> TCG trace prints also use TCG helpers, so there should not be a lot of

> difference.


Pretty much. The only real difference is we don't have plugins enabled
by default as there is a small impact when no plugins are used do to the
insertion of dummy callbacks during code generation. We do this to avoid
a double-pass later.

> As Stefan pointed out, this means plugin writers will have to write

> their own TCG tracing code. Hopefully, the plugin API can be extended

> to integrate with qemu's logging backend (it seems qemu_plugin_outs

> goes somewhere else),


qemu_plugin_outs goes out via the logging backend - but not the tracing
subsystem. Do you think being able to emit tracepoints to simpletrace or
the dtrace/ftrace would be a useful extension to the API.

> and common plugins can be kept on a separate repo

> under the qemu umbrella for easier reuse (e.g., to provide the same

> functionality as TCG tracing).


Currently we have two areas in QEMU, tests/plugins which are very basic
and used in check-tcg to exercise the code and contrib/plugins where the
meatier plugins are defined. I'm happy (and encourage!) more to be added
there.

Do you have any documented uses of the trace subsystem that I could
re-implement in TCG plugins by way of example? I have the feeling it has
been used for academic papers but I haven't seen usage "in the wild".

>

>

> Cheers,

> Lluis



-- 
Alex Bennée
Vilanova, Lluis April 27, 2021, 1 p.m. | #10
El dv. 23 de 04 de 2021 a les 16:14 +0100, en/na Alex Bennée va
escriure:
> > > > > > > > 

> > As Stefan pointed out, this means plugin writers will have to write

> > their own TCG tracing code. Hopefully, the plugin API can be

> > extended

> > to integrate with qemu's logging backend (it seems qemu_plugin_outs

> > goes somewhere else),

> 

> qemu_plugin_outs goes out via the logging backend - but not the

> tracing

> subsystem. Do you think being able to emit tracepoints to simpletrace

> or

> the dtrace/ftrace would be a useful extension to the API.


It seems dtrace would be hard to "automatically" support unless all
plugin callbacks are kept in tracetool (since there is all the compile-
time generation). So maybe it's better to keep it simple and let plugin
writers decide if they want to support a specific backend by defining
whatever necessary on their own plugins.

Would plugins be able to hook into QEMU's backend to inform it of the
plugin-defined events when the plugin is loaded? (e.g., let dtrace know
about dtrace events in the plugin). I'm not sure how the external
dependencies work for all the various backends.


> 

> Do you have any documented uses of the trace subsystem that I could

> re-implement in TCG plugins by way of example? I have the feeling it

> has

> been used for academic papers but I haven't seen usage "in the wild".


I have not kept up with the plugin developments since I participated in
the discussion of this years ago. So unfortunately I cannot provide any
meaningful answers/help here.


Cheers,
Lluis

Patch

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 0b42e53500..0d643f78fe 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -36,6 +36,7 @@ 
 #include "exec/log.h"
 
 #include "trace-tcg.h"
+#include "trace.h"
 #include "translate-a64.h"
 #include "qemu/atomic128.h"
 
@@ -2302,6 +2303,9 @@  static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
         default:
             goto do_unallocated;
         }
+
+        trace_eret_tcg(s->current_el, dst);
+
         if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
             gen_io_start();
         }
diff --git a/target/arm/trace-events b/target/arm/trace-events
index 41c63d7570..2d4fca16a1 100644
--- a/target/arm/trace-events
+++ b/target/arm/trace-events
@@ -1,5 +1,10 @@ 
 # See docs/devel/tracing.txt for syntax documentation.
 
+# translate-a64.c
+# Mode: softmmu
+# Targets: TCG(aarch64-softmmu)
+tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", "exec_eret: EL%d to EL%"PRId64
+
 # helper.c
 arm_gt_recalc(int timer, int irqstate, uint64_t nexttick) "gt recalc: timer %d irqstate %d next tick 0x%" PRIx64
 arm_gt_recalc_disabled(int timer) "gt recalc: timer %d irqstate 0 timer disabled"