* [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
@ 2023-11-29 20:50 Philippe Mathieu-Daudé
2023-11-30 12:48 ` Claudio Fontana
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-29 20:50 UTC (permalink / raw)
To: qemu-devel
Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm,
Philippe Mathieu-Daudé
'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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
2023-11-29 20:50 [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels Philippe Mathieu-Daudé
@ 2023-11-30 12:48 ` Claudio Fontana
2023-11-30 13:31 ` Philippe Mathieu-Daudé
2023-11-30 13:51 ` Richard Henderson
2024-01-17 9:39 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 7+ messages in thread
From: Claudio Fontana @ 2023-11-30 12:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
2023-11-30 12:48 ` Claudio Fontana
@ 2023-11-30 13:31 ` Philippe Mathieu-Daudé
2023-11-30 13:37 ` Claudio Fontana
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-30 13:31 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel
Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
2023-11-30 13:31 ` Philippe Mathieu-Daudé
@ 2023-11-30 13:37 ` Claudio Fontana
0 siblings, 0 replies; 7+ messages in thread
From: Claudio Fontana @ 2023-11-30 13:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
2023-11-29 20:50 [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels Philippe Mathieu-Daudé
2023-11-30 12:48 ` Claudio Fontana
@ 2023-11-30 13:51 ` Richard Henderson
2024-01-16 21:24 ` Philippe Mathieu-Daudé
2024-01-17 9:39 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2023-11-30 13:51 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
2023-11-30 13:51 ` Richard Henderson
@ 2024-01-16 21:24 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-16 21:24 UTC (permalink / raw)
To: Richard Henderson, qemu-devel, Pavel Dovgalyuk
Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels
2023-11-29 20:50 [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels Philippe Mathieu-Daudé
2023-11-30 12:48 ` Claudio Fontana
2023-11-30 13:51 ` Richard Henderson
@ 2024-01-17 9:39 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-17 9:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Cameron Esfahani, Paolo Bonzini, Roman Bolshakov, kvm
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-01-17 9:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-29 20:50 [PATCH] accel: Do not set CPUState::can_do_io in non-TCG accels Philippe Mathieu-Daudé
2023-11-30 12:48 ` Claudio Fontana
2023-11-30 13:31 ` Philippe Mathieu-Daudé
2023-11-30 13:37 ` Claudio Fontana
2023-11-30 13:51 ` Richard Henderson
2024-01-16 21:24 ` Philippe Mathieu-Daudé
2024-01-17 9:39 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.