[RFC,v4,05/71] cpu: move run_on_cpu to cpus-common

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>
---
 include/qom/cpu.h | 10 ----------
 cpus-common.c     |  2 +-
 cpus.c            |  5 -----
 3 files changed, 1 insertion(+), 16 deletions(-)

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