| Message ID | 20181025144644.15464-5-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v4,01/71] cpu: convert queued work to a QSIMPLEQ | expand |
Emilio G. Cota <cota@braap.org> writes: > We don't pass a pointer to qemu_global_mutex anymore. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> As discussed on IRC I don't fundamentally object to this being in cpus-common given we have the other work queue stuff there. However given it now lives there we should assert we are in system emulation mode so if a user-mode instruction attempts to use one of the _run_on_cpu() functions we break. > --- > include/qom/cpu.h | 10 ---------- > cpus-common.c | 2 +- > cpus.c | 5 ----- > 3 files changed, 1 insertion(+), 16 deletions(-) > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 2fad537a4f..11cbf21f00 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -766,16 +766,6 @@ void qemu_cpu_kick(CPUState *cpu); > */ > bool cpu_is_stopped(CPUState *cpu); > > -/** > - * do_run_on_cpu: > - * @cpu: The vCPU to run on. > - * @func: The function to be executed. > - * @data: Data to pass to the function. > - * > - * Used internally in the implementation of run_on_cpu. > - */ > -void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data); > - > /** > * run_on_cpu: > * @cpu: The vCPU to run on. > diff --git a/cpus-common.c b/cpus-common.c > index 71469c85ce..3fccee5585 100644 > --- a/cpus-common.c > +++ b/cpus-common.c > @@ -127,7 +127,7 @@ static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi) > cpu_mutex_unlock(cpu); > } > > -void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data) > +void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data) > { > struct qemu_work_item wi; > bool has_bql = qemu_mutex_iothread_locked(); > diff --git a/cpus.c b/cpus.c > index d0b7f8e02d..913db6a8a4 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1234,11 +1234,6 @@ void qemu_init_cpu_loop(void) > qemu_thread_get_self(&io_thread); > } > > -void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data) > -{ > - do_run_on_cpu(cpu, func, data); > -} > - > static void qemu_kvm_destroy_vcpu(CPUState *cpu) > { > if (kvm_destroy_vcpu(cpu) < 0) { -- Alex Bennée
On Mon, Oct 29, 2018 at 16:34:49 +0000, Alex Bennée wrote: > > Emilio G. Cota <cota@braap.org> writes: > > > We don't pass a pointer to qemu_global_mutex anymore. > > > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Signed-off-by: Emilio G. Cota <cota@braap.org> > > As discussed on IRC I don't fundamentally object to this being in > cpus-common given we have the other work queue stuff there. However > given it now lives there we should assert we are in system emulation > mode so if a user-mode instruction attempts to use one of the > _run_on_cpu() functions we break. Thanks for looking into this. I fixed up the commit to add stubs for cpu_lock/unlock, since the former cpu->work_mutex has to keep working. I'm not convinced about adding an "assert(!user-mode)" to run_on_cpu. Given that now it does not depend on the BQL, it could actually work in user-mode if called. If we really wanted to make sure that no user-mode would call it, then a compile-time check would be better than an assert. But again, I fail to see what we'd gain from that. For context, do_run_on_cpu et al. were moved to cpus-common.c by d148d90ee8 ("cpus-common: move CPU work item management to common code", 2016-09-27). The point was to consolidate the run-on-cpu code in a common (softmmu & user-mode) file, since user-mode needed async_run_on_cpu for exclusive work. Now we can finally make run_on_cpu generic as well. Thanks, Emilio
On 29/10/2018 22:39, Emilio G. Cota wrote: > I'm not convinced about adding an "assert(!user-mode)" to run_on_cpu. > Given that now it does not depend on the BQL, it could actually > work in user-mode if called. If we really wanted to make sure > that no user-mode would call it, then a compile-time check > would be better than an assert. But again, I fail to see what > we'd gain from that. > > For context, do_run_on_cpu et al. were moved to cpus-common.c by > d148d90ee8 ("cpus-common: move CPU work item management to > common code", 2016-09-27). The point was to consolidate the > run-on-cpu code in a common (softmmu & user-mode) file, since > user-mode needed async_run_on_cpu for exclusive work. > > Now we can finally make run_on_cpu generic as well. I agree, the run_on_cpu stuff should not be system-specific at all. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 29/10/2018 22:39, Emilio G. Cota wrote: >> I'm not convinced about adding an "assert(!user-mode)" to run_on_cpu. >> Given that now it does not depend on the BQL, it could actually >> work in user-mode if called. If we really wanted to make sure >> that no user-mode would call it, then a compile-time check >> would be better than an assert. But again, I fail to see what >> we'd gain from that. >> >> For context, do_run_on_cpu et al. were moved to cpus-common.c by >> d148d90ee8 ("cpus-common: move CPU work item management to >> common code", 2016-09-27). The point was to consolidate the >> run-on-cpu code in a common (softmmu & user-mode) file, since >> user-mode needed async_run_on_cpu for exclusive work. >> >> Now we can finally make run_on_cpu generic as well. > > I agree, the run_on_cpu stuff should not be system-specific at all. I'm happy to for it to be generic - just not broken ;-) I'm not sure what sort of use cases it has at the moment given we use start/end_exclusive for both atomics and system call marshalling in linux-user. However have a common toolbox across system and linux-user is a good thing. > > Paolo -- Alex Bennée
diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 2fad537a4f..11cbf21f00 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -766,16 +766,6 @@ void qemu_cpu_kick(CPUState *cpu); */ bool cpu_is_stopped(CPUState *cpu); -/** - * do_run_on_cpu: - * @cpu: The vCPU to run on. - * @func: The function to be executed. - * @data: Data to pass to the function. - * - * Used internally in the implementation of run_on_cpu. - */ -void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data); - /** * run_on_cpu: * @cpu: The vCPU to run on. diff --git a/cpus-common.c b/cpus-common.c index 71469c85ce..3fccee5585 100644 --- a/cpus-common.c +++ b/cpus-common.c @@ -127,7 +127,7 @@ static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi) cpu_mutex_unlock(cpu); } -void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data) +void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data) { struct qemu_work_item wi; bool has_bql = qemu_mutex_iothread_locked(); diff --git a/cpus.c b/cpus.c index d0b7f8e02d..913db6a8a4 100644 --- a/cpus.c +++ b/cpus.c @@ -1234,11 +1234,6 @@ void qemu_init_cpu_loop(void) qemu_thread_get_self(&io_thread); } -void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data) -{ - do_run_on_cpu(cpu, func, data); -} - static void qemu_kvm_destroy_vcpu(CPUState *cpu) { if (kvm_destroy_vcpu(cpu) < 0) {