Re: [PATCH] util/log: flush TB cache when log level changes

[Pavel Dovgalyuk]

On 22.01.2021 14:42, Alex Bennée wrote:
> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
>
> > Sometimes we need to collect the translation logs starting
> > from some point of the execution. Some TB listings may
> > be missed in this case, when blocks were translated before.
> > This patch clears TB cache to allow re-translation of such
> > code blocks.
> >
> > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> > ---
> >   accel/tcg/translate-all.c |    8 ++++++++
> >   include/sysemu/tcg.h      |    1 +
> >   stubs/meson.build         |    1 +
> >   stubs/tcg.c               |   12 ++++++++++++
> >   util/log.c                |    3 +++
> >   5 files changed, 25 insertions(+)
> >   create mode 100644 stubs/tcg.c
> >
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index e9de6ff9dd..3acb227c57 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -1461,6 +1461,14 @@ void tb_flush(CPUState *cpu)
> >       }
> >   }
> >   
> > +void tb_flush_all(void)
> > +{
> > +    CPUState *cpu;
> > +    CPU_FOREACH(cpu) {
> > +        tb_flush(cpu);
> > +    }
> > +}
> > +
>
> This isn't needed - tb_flush flushes all translations although it does
> need to be executed in a CPU context to do so.
>
> >   /*
> >    * Formerly ifdef DEBUG_TB_CHECK. These debug functions are user-mode-only,
> >    * so in order to prevent bit rot we compile them unconditionally in 
> > user-mode,
> > diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h
> > index 00349fb18a..7415f11022 100644
> > --- a/include/sysemu/tcg.h
> > +++ b/include/sysemu/tcg.h
> > @@ -9,6 +9,7 @@
> >   #define SYSEMU_TCG_H
> >   
> >   void tcg_exec_init(unsigned long tb_size, int splitwx);
> > +void tb_flush_all(void);
> >   
> >   #ifdef CONFIG_TCG
> >   extern bool tcg_allowed;
> > diff --git a/stubs/meson.build b/stubs/meson.build
> > index 80b1d81a31..95e70f8542 100644
> > --- a/stubs/meson.build
> > +++ b/stubs/meson.build
> > @@ -38,6 +38,7 @@ stub_ss.add(files('set-fd-handler.c'))
> >   stub_ss.add(files('sysbus.c'))
> >   stub_ss.add(files('target-get-monitor-def.c'))
> >   stub_ss.add(files('target-monitor-defs.c'))
> > +stub_ss.add(files('tcg.c'))
> >   stub_ss.add(files('tpm.c'))
> >   stub_ss.add(files('trace-control.c'))
> >   stub_ss.add(files('uuid.c'))
> > diff --git a/stubs/tcg.c b/stubs/tcg.c
> > new file mode 100644
> > index 0000000000..775a748c77
> > --- /dev/null
> > +++ b/stubs/tcg.c
> > @@ -0,0 +1,12 @@
> > +/*
> > + * TCG stubs
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "sysemu/tcg.h"
> > +
> > +void tb_flush_all(void)
> > +{
> > +}
> > diff --git a/util/log.c b/util/log.c
> > index 2ee1500bee..2ff342a91b 100644
> > --- a/util/log.c
> > +++ b/util/log.c
> > @@ -26,6 +26,7 @@
> >   #include "trace/control.h"
> >   #include "qemu/thread.h"
> >   #include "qemu/lockable.h"
> > +#include "sysemu/tcg.h"
> >   
> >   static char *logfilename;
> >   static QemuMutex qemu_logfile_mutex;
> > @@ -84,6 +85,8 @@ void qemu_set_log(int log_flags)
> >   #ifdef CONFIG_TRACE_LOG
> >       qemu_loglevel |= LOG_TRACE;
> >   #endif
> > +    tb_flush_all();
> > +
>
> I would call tb_flush(current_cpu) or first_cpu here. But two things:
>
>   - I'm not sure you have a CPU at all times qemu_set_log is called
>   - It seems overly aggressive to throw away all translations every time
>     the log level is changed. I would define a mask in log.h and have
>     something like:

Do you propose removing the parameter from tb_flush or omitting the loop
from tb_flush_all?

>    if (log_flags & LOG_TRANSLATION) {
>        tb_flush();
>    }
>
> >       /*
> >        * In all cases we only log if qemu_loglevel is set.
> >        * Also: