accel: Do not set CPUState::can_do_io in non-TCG accels

[PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Philippe Mathieu-Daudé 12 months ago

'can_do_io' is specific to TCG. Having it set in non-TCG
code is confusing, so remove it from QTest / HVF / KVM.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/dummy-cpus.c        | 1 -
 accel/hvf/hvf-accel-ops.c | 1 -
 accel/kvm/kvm-accel-ops.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
index b75c919ac3..1005ec6f56 100644
--- a/accel/dummy-cpus.c
+++ b/accel/dummy-cpus.c
@@ -27,7 +27,6 @@ static void *dummy_cpu_thread_fn(void *arg)
     qemu_mutex_lock_iothread();
     qemu_thread_get_self(cpu->thread);
     cpu->thread_id = qemu_get_thread_id();
-    cpu->neg.can_do_io = true;
     current_cpu = cpu;
 
 #ifndef _WIN32
diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index abe7adf7ee..2bba54cf70 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -428,7 +428,6 @@ static void *hvf_cpu_thread_fn(void *arg)
     qemu_thread_get_self(cpu->thread);
 
     cpu->thread_id = qemu_get_thread_id();
-    cpu->neg.can_do_io = true;
     current_cpu = cpu;
 
     hvf_init_vcpu(cpu);
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index 6195150a0b..f273f415db 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -36,7 +36,6 @@ static void *kvm_vcpu_thread_fn(void *arg)
     qemu_mutex_lock_iothread();
     qemu_thread_get_self(cpu->thread);
     cpu->thread_id = qemu_get_thread_id();
-    cpu->neg.can_do_io = true;
     current_cpu = cpu;
 
     r = kvm_init_vcpu(cpu, &error_fatal);
-- 
2.41.0

Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Philippe Mathieu-Daudé 10 months, 2 weeks ago

On 29/11/23 21:50, Philippe Mathieu-Daudé wrote:
> 'can_do_io' is specific to TCG. Having it set in non-TCG
> code is confusing, so remove it from QTest / HVF / KVM.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/dummy-cpus.c        | 1 -
>   accel/hvf/hvf-accel-ops.c | 1 -
>   accel/kvm/kvm-accel-ops.c | 1 -
>   3 files changed, 3 deletions(-)

Patch queued.

Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Richard Henderson 12 months ago

On 11/29/23 14:50, Philippe Mathieu-Daudé wrote:
> 'can_do_io' is specific to TCG. Having it set in non-TCG
> code is confusing, so remove it from QTest / HVF / KVM.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/dummy-cpus.c        | 1 -
>   accel/hvf/hvf-accel-ops.c | 1 -
>   accel/kvm/kvm-accel-ops.c | 1 -
>   3 files changed, 3 deletions(-)
> 
> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
> index b75c919ac3..1005ec6f56 100644
> --- a/accel/dummy-cpus.c
> +++ b/accel/dummy-cpus.c
> @@ -27,7 +27,6 @@ static void *dummy_cpu_thread_fn(void *arg)
>       qemu_mutex_lock_iothread();
>       qemu_thread_get_self(cpu->thread);
>       cpu->thread_id = qemu_get_thread_id();
> -    cpu->neg.can_do_io = true;
>       current_cpu = cpu;

I expect this is ok...

When I was moving this variable around just recently, 464dacf6, I wondered about these 
other settings, and I wondered if they used to be required for implementing some i/o on 
behalf of hw accel.  Something that has now been factored out to e.g. kvm_handle_io, which 
now uses address_space_rw directly.

I see 3 reads of can_do_io: accel/tcg/{cputlb.c, icount-common.c} and system/watchpoint.c. 
  The final one is nested within replay_running_debug(), which implies icount and tcg.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

>   
>   #ifndef _WIN32
> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> index abe7adf7ee..2bba54cf70 100644
> --- a/accel/hvf/hvf-accel-ops.c
> +++ b/accel/hvf/hvf-accel-ops.c
> @@ -428,7 +428,6 @@ static void *hvf_cpu_thread_fn(void *arg)
>       qemu_thread_get_self(cpu->thread);
>   
>       cpu->thread_id = qemu_get_thread_id();
> -    cpu->neg.can_do_io = true;
>       current_cpu = cpu;
>   
>       hvf_init_vcpu(cpu);
> diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
> index 6195150a0b..f273f415db 100644
> --- a/accel/kvm/kvm-accel-ops.c
> +++ b/accel/kvm/kvm-accel-ops.c
> @@ -36,7 +36,6 @@ static void *kvm_vcpu_thread_fn(void *arg)
>       qemu_mutex_lock_iothread();
>       qemu_thread_get_self(cpu->thread);
>       cpu->thread_id = qemu_get_thread_id();
> -    cpu->neg.can_do_io = true;
>       current_cpu = cpu;
>   
>       r = kvm_init_vcpu(cpu, &error_fatal);

Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Philippe Mathieu-Daudé 10 months, 2 weeks ago

On 30/11/23 14:51, Richard Henderson wrote:
> On 11/29/23 14:50, Philippe Mathieu-Daudé wrote:
>> 'can_do_io' is specific to TCG. Having it set in non-TCG
>> code is confusing, so remove it from QTest / HVF / KVM.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   accel/dummy-cpus.c        | 1 -
>>   accel/hvf/hvf-accel-ops.c | 1 -
>>   accel/kvm/kvm-accel-ops.c | 1 -
>>   3 files changed, 3 deletions(-)
>>
>> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
>> index b75c919ac3..1005ec6f56 100644
>> --- a/accel/dummy-cpus.c
>> +++ b/accel/dummy-cpus.c
>> @@ -27,7 +27,6 @@ static void *dummy_cpu_thread_fn(void *arg)
>>       qemu_mutex_lock_iothread();
>>       qemu_thread_get_self(cpu->thread);
>>       cpu->thread_id = qemu_get_thread_id();
>> -    cpu->neg.can_do_io = true;
>>       current_cpu = cpu;
> 
> I expect this is ok...
> 
> When I was moving this variable around just recently, 464dacf6, I 
> wondered about these other settings, and I wondered if they used to be 
> required for implementing some i/o on behalf of hw accel.  Something 
> that has now been factored out to e.g. kvm_handle_io, which now uses 
> address_space_rw directly.

It was added by mistake in commit 99df7dce8a ("cpu: Move can_do_io
field from CPU_COMMON to CPUState", 2013) to cpu_common_reset() and
commit 626cf8f4c6 ("icount: set can_do_io outside TB execution", 2014),
then moved in commits 57038a92bb ("cpus: extract out kvm-specific code
to accel/kvm"), b86f59c715 ("accel: replace struct CpusAccel with
AccelOpsClass") and the one you mentioned, 464dacf609 ("accel/tcg: Move
can_do_io to CPUNegativeOffsetState").

The culprit is 626cf8f4c6 I guess, so maybe:
Fixes: 626cf8f4c6 ("icount: set can_do_io outside TB execution")

> I see 3 reads of can_do_io: accel/tcg/{cputlb.c, icount-common.c} and 
> system/watchpoint.c.  The final one is nested within 
> replay_running_debug(), which implies icount and tcg.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Claudio Fontana 12 months ago

Hi Philippe,

took a quick look with 

grep -R can_do_io

and this seems to be in include/hw/core/cpu.h as well as cpu-common.c,

maybe there is more meat to address to fully solve this?

Before we had stuff for reset in cpu-common.c under a
if (tcg_enabled()) {
}

but now we have cpu_exec_reset_hold(),
should the implementation for tcg of cpu_exec_reset_hold() do that (and potentially other tcg-specific non-arch-specific cpu variables we might need)?

If can_do_io is TCG-specific, maybe the whole field existence / visibility can be conditioned on TCG actually being at least compiled-in?
This might help find problems of the field being used in the wrong context, by virtue of getting an error when compiling with --disable-tcg for example.

Ciao,

Claudio


On 11/29/23 21:50, Philippe Mathieu-Daudé wrote:
> 'can_do_io' is specific to TCG. Having it set in non-TCG
> code is confusing, so remove it from QTest / HVF / KVM.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  accel/dummy-cpus.c        | 1 -
>  accel/hvf/hvf-accel-ops.c | 1 -
>  accel/kvm/kvm-accel-ops.c | 1 -
>  3 files changed, 3 deletions(-)
> 
> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
> index b75c919ac3..1005ec6f56 100644
> --- a/accel/dummy-cpus.c
> +++ b/accel/dummy-cpus.c
> @@ -27,7 +27,6 @@ static void *dummy_cpu_thread_fn(void *arg)
>      qemu_mutex_lock_iothread();
>      qemu_thread_get_self(cpu->thread);
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu->neg.can_do_io = true;
>      current_cpu = cpu;
>  
>  #ifndef _WIN32
> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> index abe7adf7ee..2bba54cf70 100644
> --- a/accel/hvf/hvf-accel-ops.c
> +++ b/accel/hvf/hvf-accel-ops.c
> @@ -428,7 +428,6 @@ static void *hvf_cpu_thread_fn(void *arg)
>      qemu_thread_get_self(cpu->thread);
>  
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu->neg.can_do_io = true;
>      current_cpu = cpu;
>  
>      hvf_init_vcpu(cpu);
> diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
> index 6195150a0b..f273f415db 100644
> --- a/accel/kvm/kvm-accel-ops.c
> +++ b/accel/kvm/kvm-accel-ops.c
> @@ -36,7 +36,6 @@ static void *kvm_vcpu_thread_fn(void *arg)
>      qemu_mutex_lock_iothread();
>      qemu_thread_get_self(cpu->thread);
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu->neg.can_do_io = true;
>      current_cpu = cpu;
>  
>      r = kvm_init_vcpu(cpu, &error_fatal);

Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Philippe Mathieu-Daudé 12 months ago

Hi Claudio,

On 30/11/23 13:48, Claudio Fontana wrote:
> Hi Philippe,
> 
> took a quick look with
> 
> grep -R can_do_io
> 
> and this seems to be in include/hw/core/cpu.h as well as cpu-common.c,
> 
> maybe there is more meat to address to fully solve this?
> 
> Before we had stuff for reset in cpu-common.c under a
> if (tcg_enabled()) {
> }
> 
> but now we have cpu_exec_reset_hold(),
> should the implementation for tcg of cpu_exec_reset_hold() do that (and potentially other tcg-specific non-arch-specific cpu variables we might need)?

Later we eventually get there:

diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 9b038b1af5..e2c5cf97dc 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -89,6 +89,9 @@ static void tcg_cpu_reset_hold(CPUState *cpu)

      cpu->accel->icount_extra = 0;
      cpu->accel->mem_io_pc = 0;
+
+    qatomic_set(&cpu->neg.icount_decr.u32, 0);
+    cpu->neg.can_do_io = true;
  }

My branch is huge, I'm trying to split it, maybe I shouldn't have
sent this single non-TCG patch out of it. I'll Cc you.

> If can_do_io is TCG-specific, maybe the whole field existence / visibility can be conditioned on TCG actually being at least compiled-in?
> This might help find problems of the field being used in the wrong context, by virtue of getting an error when compiling with --disable-tcg for example.
> 
> Ciao,
> 
> Claudio

Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels

Posted by Claudio Fontana 12 months ago

On 11/30/23 14:31, Philippe Mathieu-Daudé wrote:
> Hi Claudio,
> 
> On 30/11/23 13:48, Claudio Fontana wrote:
>> Hi Philippe,
>>
>> took a quick look with
>>
>> grep -R can_do_io
>>
>> and this seems to be in include/hw/core/cpu.h as well as cpu-common.c,
>>
>> maybe there is more meat to address to fully solve this?
>>
>> Before we had stuff for reset in cpu-common.c under a
>> if (tcg_enabled()) {
>> }
>>
>> but now we have cpu_exec_reset_hold(),
>> should the implementation for tcg of cpu_exec_reset_hold() do that (and potentially other tcg-specific non-arch-specific cpu variables we might need)?
> 
> Later we eventually get there:
> 
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index 9b038b1af5..e2c5cf97dc 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -89,6 +89,9 @@ static void tcg_cpu_reset_hold(CPUState *cpu)
> 
>       cpu->accel->icount_extra = 0;
>       cpu->accel->mem_io_pc = 0;
> +
> +    qatomic_set(&cpu->neg.icount_decr.u32, 0);
> +    cpu->neg.can_do_io = true;
>   }
> 
> My branch is huge, I'm trying to split it, maybe I shouldn't have
> sent this single non-TCG patch out of it. I'll Cc you.

Thanks and congrats for the rework effort there!

Ciao,

Claudio

> 
>> If can_do_io is TCG-specific, maybe the whole field existence / visibility can be conditioned on TCG actually being at least compiled-in?
>> This might help find problems of the field being used in the wrong context, by virtue of getting an error when compiling with --disable-tcg for example.
>>
>> Ciao,
>>
>> Claudio
>