| Message ID | 20231129205037.16849-1-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel: Do not set CPUState::can_do_io in non-TCG accels | expand |
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);
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
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 >
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);
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!
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.
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);
'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(-)