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

[liu ping fan]

On Fri, Jun 22, 2012 at 8:36 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Thu, Jun 21, 2012 at 4:06 PM, Liu Ping Fan <address@hidden> wrote:
>> 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"
>
> This breaks Windows, you need qemu-thread.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;                                         \
>
> It would be nicer to declare it QemuMutex cpu_lock (no pointer) so
> that you don't need to worry about malloc/free.
>
Yes :), I have considered about it, and not quite sure.  But I figure
out the lock for per-device to break BQL will be like this for some
reason.
After all, have not decided yet.
>>     struct qemu_work_item *queued_work_first, *queued_work_last;        \
>>     const char *cpu_model_str;                                          \
>>     struct KVMState *kvm_state;                                         \
>> 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;
>>
>> +
>>     r = kvm_init_vcpu(env);
>>     if (r < 0) {
>>         fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
>
> Spurious whitespace change, this should be dropped from the patch.
>
>> 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);
>
> Why void* instead of CPUArchState*?
>
Because we can hide the CPUArchState from apic.c

Thanks and regards,
pingfan
> Stefan