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

[Alex Bennée]

Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:

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

No tb_flush should keep the CPU interface. In normal usage from the
emulation we always have a CPU to call. However for qemu_set_log you
will need to find a CPU to call or bail out if you can't. Maybe
something like:

  CPUStatus *cpu = current_cpu || first_cpu;
  if (cpu) {
      tb_flush(cpu);
  }

my only worry is if qemu_set_log is called from outside a CPU context
(current_cpu will always be NULL) while first_cpu is in a exclusive
region. We could extend cpu_in_exclusive_context to be:

  cpu == current_cpu && cpu->in_exclusive_context

but that seems a little icky to me. Paolo, any thoughts?

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


-- 
Alex Bennée