Re: [Qemu-devel] [PATCH 1/2] CPUArchState: introduce per-cpu lock

[liu ping fan]

On Fri, Jun 22, 2012 at 8:55 PM, Andreas Färber <address@hidden> wrote:
> Am 21.06.2012 17:06, schrieb Liu Ping Fan:
>> introduce a lock for per-cpu to protect agaist accesing from
>> other vcpu thread.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  cpu-defs.h  |    2 ++
>>  cpus.c      |   17 +++++++++++++++++
>>  main-loop.h |    3 +++
>>  3 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/cpu-defs.h b/cpu-defs.h
>> index f49e950..7305822 100644
>> --- a/cpu-defs.h
>> +++ b/cpu-defs.h
>> @@ -30,6 +30,7 @@
>>  #include "osdep.h"
>>  #include "qemu-queue.h"
>>  #include "targphys.h"
>> +#include "qemu-thread-posix.h"
>>
>>  #ifndef TARGET_LONG_BITS
>>  #error TARGET_LONG_BITS must be defined before including this header
>> @@ -220,6 +221,7 @@ typedef struct CPUWatchpoint {
>>      CPU_COMMON_THREAD                                                   \
>>      struct QemuCond *halt_cond;                                         \
>>      int thread_kicked;                                                  \
>> +    struct QemuMutex *cpu_lock;                                         \
>>      struct qemu_work_item *queued_work_first, *queued_work_last;        \
>>      const char *cpu_model_str;                                          \
>>      struct KVMState *kvm_state;                                         \
>
> Please don't add stuff to CPU_COMMON. Instead add to CPUState in
> qom/cpu.c. The QOM CPUState part 4 series moves many of those fields there.
>
Ok, thanks.
>> diff --git a/cpus.c b/cpus.c
>> index b182b3d..554f7bc 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -735,6 +735,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>      env->thread_id = qemu_get_thread_id();
>>      cpu_single_env = env;
>>
>> +
>
> Stray whitespace addition.
>
>>      r = kvm_init_vcpu(env);
>>      if (r < 0) {
>>          fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
>> @@ -891,6 +892,20 @@ int qemu_cpu_is_self(void *_env)
>>      return qemu_thread_is_self(env->thread);
>>  }
>>
>> +void qemu_mutex_lock_cpu(void *_env)
>> +{
>> +    CPUArchState *env = _env;
>> +
>> +    qemu_mutex_lock(env->cpu_lock);
>> +}
>> +
>> +void qemu_mutex_unlock_cpu(void *_env)
>> +{
>> +    CPUArchState *env = _env;
>> +
>> +    qemu_mutex_unlock(env->cpu_lock);
>> +}
>> +
>
> I don't like these helpers. For one, you are using void * arguments and
> casting them, for another you are using CPUArchState at all. With my
> suggestion above these can be CPUState *cpu.
>
For using it in apic.c, we need to hide the  CPUArchState structure
>>  void qemu_mutex_lock_iothread(void)
>>  {
>>      if (!tcg_enabled()) {
>> @@ -1027,6 +1042,8 @@ void qemu_init_vcpu(void *_env)
>>      env->nr_cores = smp_cores;
>>      env->nr_threads = smp_threads;
>>      env->stopped = 1;
>> +    env->cpu_lock = g_malloc0(sizeof(QemuMutex));
>> +    qemu_mutex_init(env->cpu_lock);
>
> Are you sure this is not needed for linux-user/bsd-user? If not needed,
> then the field should be #ifdef'ed in the struct to assure that.
> Otherwise this function is never called and you need to move the
> initialization to the initfn in qom/cpu.c and then should also clean it
> up in a finalizer.
>
It is not needed for linux-user/bsd-user. I will use the macro,

Thanks and regards,
pingfan

> Andreas
>
>>      if (kvm_enabled()) {
>>          qemu_kvm_start_vcpu(env);
>>      } else if (tcg_enabled()) {
>> diff --git a/main-loop.h b/main-loop.h
>> index dce1cd9..d8d44a4 100644
>> --- a/main-loop.h
>> +++ b/main-loop.h
>> @@ -323,6 +323,9 @@ void qemu_bh_delete(QEMUBH *bh);
>>  int qemu_add_child_watch(pid_t pid);
>>  #endif
>>
>> +void qemu_mutex_lock_cpu(void *_env);
>> +void qemu_mutex_unlock_cpu(void *_env);
>> +
>>  /**
>>   * qemu_mutex_lock_iothread: Lock the main loop mutex.
>>   *
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg