[PATCH 0/2] accel/tcg: Remove qemu_tcg_mttcg_enabled()

[PATCH 0/2] accel/tcg: Remove qemu_tcg_mttcg_enabled()

[Philippe Mathieu-Daudé]

Remove qemu_tcg_mttcg_enabled():
- check for CF_PARALLEL in riscv cpu_init(),
- directly check 'mttcg_enabled' in TCG code.

Philippe Mathieu-Daudé (2):
  target/riscv: Check for CF_PARALLEL instead of qemu_tcg_mttcg_enabled
  accel/tcg: Replace qemu_tcg_mttcg_enabled() by mttcg_enabled

 accel/tcg/tcg-accel-ops-mttcg.h | 8 ++++++++
 include/hw/core/cpu.h           | 9 ---------
 accel/tcg/tcg-accel-ops.c       | 2 +-
 target/riscv/cpu.c              | 2 +-
 tcg/region.c                    | 3 ++-
 5 files changed, 12 insertions(+), 12 deletions(-)

-- 
2.38.1

[PATCH 1/2] target/riscv: Check for CF_PARALLEL instead of qemu_tcg_mttcg_enabled

[Philippe Mathieu-Daudé]

A CPU knows whether MTTCG is enabled or not because it is
reflected in its TCG flags via the CF_PARALLEL bit.

Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4035fe0e62..4dfa64af6a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -473,7 +473,7 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
 
 static void rv128_base_cpu_init(Object *obj)
 {
-    if (qemu_tcg_mttcg_enabled()) {
+    if (CPU(obj)->tcg_cflags & CF_PARALLEL) {
         /* Missing 128-bit aligned atomics */
         error_report("128-bit RISC-V currently does not work with Multi "
                      "Threaded TCG. Please use: -accel tcg,thread=single");
-- 
2.38.1

[PATCH 2/2] accel/tcg: Replace qemu_tcg_mttcg_enabled() by mttcg_enabled

[Philippe Mathieu-Daudé]

Move 'mttcg_enabled' declaration to "tcg-accel-ops-mttcg.h"
which is a TCG-internal header; un-inline and remove the
qemu_tcg_mttcg_enabled() definition.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/tcg-accel-ops-mttcg.h | 8 ++++++++
 include/hw/core/cpu.h           | 9 ---------
 accel/tcg/tcg-accel-ops.c       | 2 +-
 tcg/region.c                    | 3 ++-
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-mttcg.h b/accel/tcg/tcg-accel-ops-mttcg.h
index 8ffa7a9a9f..1ffe8f3ac2 100644
--- a/accel/tcg/tcg-accel-ops-mttcg.h
+++ b/accel/tcg/tcg-accel-ops-mttcg.h
@@ -10,6 +10,14 @@
 #ifndef TCG_ACCEL_OPS_MTTCG_H
 #define TCG_ACCEL_OPS_MTTCG_H
 
+/**
+ * qemu_tcg_mttcg_enabled:
+ * Check whether we are running MultiThread TCG or not.
+ *
+ * Returns: %true if we are in MTTCG mode %false otherwise.
+ */
+extern bool mttcg_enabled;
+
 /* kick MTTCG vCPU thread */
 void mttcg_kick_vcpu_thread(CPUState *cpu);
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b08f8b7079..5d26e6c90c 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -456,15 +456,6 @@ extern CPUTailQ cpus;
 
 extern __thread CPUState *current_cpu;
 
-/**
- * qemu_tcg_mttcg_enabled:
- * Check whether we are running MultiThread TCG or not.
- *
- * Returns: %true if we are in MTTCG mode %false otherwise.
- */
-extern bool mttcg_enabled;
-#define qemu_tcg_mttcg_enabled() (mttcg_enabled)
-
 /**
  * cpu_paging_enabled:
  * @cpu: The CPU whose state is to be inspected.
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 3973591508..5391f078cf 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -188,7 +188,7 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
 
 static void tcg_accel_ops_init(AccelOpsClass *ops)
 {
-    if (qemu_tcg_mttcg_enabled()) {
+    if (mttcg_enabled) {
         ops->create_vcpu_thread = mttcg_start_vcpu_thread;
         ops->kick_vcpu_thread = mttcg_kick_vcpu_thread;
         ops->handle_interrupt = tcg_handle_interrupt;
diff --git a/tcg/region.c b/tcg/region.c
index 2b28ed3556..45f1742271 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -32,6 +32,7 @@
 #include "qapi/error.h"
 #include "tcg/tcg.h"
 #include "exec/translation-block.h"
+#include "accel/tcg/tcg-accel-ops-mttcg.h"
 #include "tcg-internal.h"
 
 
@@ -413,7 +414,7 @@ static size_t tcg_n_regions(size_t tb_size, unsigned max_cpus)
      * dividing the code_gen_buffer among the vCPUs.
      */
     /* Use a single region if all we have is one vCPU thread */
-    if (max_cpus == 1 || !qemu_tcg_mttcg_enabled()) {
+    if (max_cpus == 1 || !mttcg_enabled) {
         return 1;
     }
 
-- 
2.38.1

Re: [PATCH 2/2] accel/tcg: Replace qemu_tcg_mttcg_enabled() by mttcg_enabled

[Philippe Mathieu-Daudé]

On 29/6/23 14:02, Philippe Mathieu-Daudé wrote:
> Move 'mttcg_enabled' declaration to "tcg-accel-ops-mttcg.h"
> which is a TCG-internal header; un-inline and remove the
> qemu_tcg_mttcg_enabled() definition.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops-mttcg.h | 8 ++++++++
>   include/hw/core/cpu.h           | 9 ---------
>   accel/tcg/tcg-accel-ops.c       | 2 +-
>   tcg/region.c                    | 3 ++-
>   4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/accel/tcg/tcg-accel-ops-mttcg.h b/accel/tcg/tcg-accel-ops-mttcg.h
> index 8ffa7a9a9f..1ffe8f3ac2 100644
> --- a/accel/tcg/tcg-accel-ops-mttcg.h
> +++ b/accel/tcg/tcg-accel-ops-mttcg.h
> @@ -10,6 +10,14 @@
>   #ifndef TCG_ACCEL_OPS_MTTCG_H
>   #define TCG_ACCEL_OPS_MTTCG_H
>   
> +/**
> + * qemu_tcg_mttcg_enabled:
> + * Check whether we are running MultiThread TCG or not.
> + *
> + * Returns: %true if we are in MTTCG mode %false otherwise.
> + */

Bah, outdated comment... To be removed.

> +extern bool mttcg_enabled;
> +
>   /* kick MTTCG vCPU thread */
>   void mttcg_kick_vcpu_thread(CPUState *cpu);
>   
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index b08f8b7079..5d26e6c90c 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -456,15 +456,6 @@ extern CPUTailQ cpus;
>   
>   extern __thread CPUState *current_cpu;
>   
> -/**
> - * qemu_tcg_mttcg_enabled:
> - * Check whether we are running MultiThread TCG or not.
> - *
> - * Returns: %true if we are in MTTCG mode %false otherwise.
> - */
> -extern bool mttcg_enabled;
> -#define qemu_tcg_mttcg_enabled() (mttcg_enabled)

Re: [PATCH 1/2] target/riscv: Check for CF_PARALLEL instead of qemu_tcg_mttcg_enabled

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> A CPU knows whether MTTCG is enabled or not because it is
> reflected in its TCG flags via the CF_PARALLEL bit.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4035fe0e62..4dfa64af6a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -473,7 +473,7 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
>  
>  static void rv128_base_cpu_init(Object *obj)
>  {
> -    if (qemu_tcg_mttcg_enabled()) {
> +    if (CPU(obj)->tcg_cflags & CF_PARALLEL) {

Hmm have you checked that tcg_cpu_init_cflags() has executed by this point?

>          /* Missing 128-bit aligned atomics */
>          error_report("128-bit RISC-V currently does not work with Multi "
>                       "Threaded TCG. Please use: -accel tcg,thread=single");

Not that we can do anything about it but in linux-user we start with
CF_PARALLEL unset and only set it at the point we spawn a new thread.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] target/riscv: Check for CF_PARALLEL instead of qemu_tcg_mttcg_enabled

[Philippe Mathieu-Daudé]

On 29/6/23 18:26, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> A CPU knows whether MTTCG is enabled or not because it is
>> reflected in its TCG flags via the CF_PARALLEL bit.
>>
>> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/riscv/cpu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 4035fe0e62..4dfa64af6a 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -473,7 +473,7 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
>>   
>>   static void rv128_base_cpu_init(Object *obj)
>>   {
>> -    if (qemu_tcg_mttcg_enabled()) {
>> +    if (CPU(obj)->tcg_cflags & CF_PARALLEL) {
> 
> Hmm have you checked that tcg_cpu_init_cflags() has executed by this point?

$arch_cpu_realize
  -> qemu_init_vcpu
      -> mttcg_start_vcpu_thread
        -> tcg_cpu_init_cflags

I'll document in the commit description.

>>           /* Missing 128-bit aligned atomics */
>>           error_report("128-bit RISC-V currently does not work with Multi "
>>                        "Threaded TCG. Please use: -accel tcg,thread=single");
> 
> Not that we can do anything about it but in linux-user we start with
> CF_PARALLEL unset and only set it at the point we spawn a new thread.

Hmm I'll give it more thinking then.

Thanks,

Phil.