From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58630) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Si3Oh-00077I-Um for qemu-devel@nongnu.org; Fri, 22 Jun 2012 08:55:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Si3Of-0000a8-VM for qemu-devel@nongnu.org; Fri, 22 Jun 2012 08:55:35 -0400 Received: from cantor2.suse.de ([195.135.220.15]:56436 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Si3Of-0000Za-MM for qemu-devel@nongnu.org; Fri, 22 Jun 2012 08:55:33 -0400 Message-ID: <4FE46B3F.8070600@suse.de> Date: Fri, 22 Jun 2012 14:55:27 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1340291218-11669-1-git-send-email-qemulist@gmail.com> <1340291218-11669-2-git-send-email-qemulist@gmail.com> In-Reply-To: <1340291218-11669-2-git-send-email-qemulist@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] CPUArchState: introduce per-cpu lock List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Ping Fan Cc: Jan Kiszka , qemu-devel@nongnu.org, Anthony Liguori Am 21.06.2012 17:06, schrieb Liu Ping Fan: > introduce a lock for per-cpu to protect agaist accesing from > other vcpu thread. >=20 > Signed-off-by: Liu Ping Fan > --- > cpu-defs.h | 2 ++ > cpus.c | 17 +++++++++++++++++ > main-loop.h | 3 +++ > 3 files changed, 22 insertions(+), 0 deletions(-) >=20 > 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" > =20 > #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 ther= e. > 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 =3D qemu_get_thread_id(); > cpu_single_env =3D env; > =20 > + Stray whitespace addition. > r =3D 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); > } > =20 > +void qemu_mutex_lock_cpu(void *_env) > +{ > + CPUArchState *env =3D _env; > + > + qemu_mutex_lock(env->cpu_lock); > +} > + > +void qemu_mutex_unlock_cpu(void *_env) > +{ > + CPUArchState *env =3D _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. > void qemu_mutex_lock_iothread(void) > { > if (!tcg_enabled()) { > @@ -1027,6 +1042,8 @@ void qemu_init_vcpu(void *_env) > env->nr_cores =3D smp_cores; > env->nr_threads =3D smp_threads; > env->stopped =3D 1; > + env->cpu_lock =3D 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. 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 > =20 > +void qemu_mutex_lock_cpu(void *_env); > +void qemu_mutex_unlock_cpu(void *_env); > + > /** > * qemu_mutex_lock_iothread: Lock the main loop mutex. > * --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg