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

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

[Pavel Dovgalyuk]

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);
+    }
+}
+
 /*
  * 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();
+
     /*
      * In all cases we only log if qemu_loglevel is set.
      * Also:

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

[Philippe Mathieu-Daudé]

Hi Pavel,

On 1/22/21 11:03 AM, Pavel Dovgalyuk wrote:
> 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
...

>  /*
>   * 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);

Why not declare in "exec/exec-all.h"?

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

[Pavel Dovgalyuk]

On 22.01.2021 13:32, Philippe Mathieu-Daudé wrote:
> Hi Pavel,
> 
> On 1/22/21 11:03 AM, Pavel Dovgalyuk wrote:
>> 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
> ...
> 
>>   /*
>>    * 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);
> 
> Why not declare in "exec/exec-all.h"?
> 

It includes cpu.h, which is not available for all tools, that use logs.

Pavel Dovgalyuk

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

[Alex Bennée]

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:

  if (log_flags & LOG_TRANSLATION) {
      tb_flush();
  }

>      /*
>       * In all cases we only log if qemu_loglevel is set.
>       * Also:


-- 
Alex Bennée

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

[Philippe Mathieu-Daudé]

On 1/22/21 12:42 PM, 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
...

> 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

Indeed, with QTest accelerator it is NULL. I also remember
some odd cases when running commands from the monitor.

Regards,

Phil.

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

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

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

[Pavel Dovgalyuk]

On 25.01.2021 14:09, Alex Bennée wrote:
> 
> 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

It the following true? We can't get rid of CPU in tb_flush, because 
do_tb_flush must be executed in vCPU thread.
Can one CPU break others execution in case of SMP? Can we move flush to 
BH somehow?

> something like:
> 
>    CPUStatus *cpu = current_cpu || first_cpu;
>    if (cpu) {
>        tb_flush(cpu);
>    }
> 

Then we'll have to expose all this CPU stuff to utils and add stubs for 
them.

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

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

[Alex Bennée]

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

> On 25.01.2021 14:09, Alex Bennée wrote:
>> 
>> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
>> 
>>> On 22.01.2021 14:42, Alex Bennée wrote:
>>>>
>>>> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
<snip>
>>>>>    #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
>
> It the following true? We can't get rid of CPU in tb_flush, because 
> do_tb_flush must be executed in vCPU thread.
> Can one CPU break others execution in case of SMP? Can we move flush to 
> BH somehow?

The flush has to execute in the CPU context because it uses the
exclusive mechanism to ensure all other CPUs are currently quiescent
before doing the flush. Currently the exclusive mechanism is only aware
of CPU threads and extending it would be essentially re-introducing a
BQL.

>
>> something like:
>> 
>>    CPUStatus *cpu = current_cpu || first_cpu;
>>    if (cpu) {
>>        tb_flush(cpu);
>>    }
>> 
>
> Then we'll have to expose all this CPU stuff to utils and add stubs for 
> them.
>
>> 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