Am 24.06.2012 16:02, schrieb liu ping fan: > On Fri, Jun 22, 2012 at 8:55 PM, Andreas Färber <afaer...@suse.de> 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 <pingf...@linux.vnet.ibm.com> >>> --- >>> 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.
My mistake: The struct to add to is in include/qemu/cpu.h obviously. >> > 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 CPUState serves to hide CPUArchState, with added type safety. :) Regards, Andreas >>> 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg