| Message ID | 1340291218-11669-2-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 21, 2012 at 4:06 PM, Liu Ping Fan <qemulist@gmail.com> 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. > 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*? Stefan
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 <pingfank@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. > 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. > 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. 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. > *
On Fri, Jun 22, 2012 at 8:55 PM, Andreas Färber <afaerber@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 <pingfank@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. > 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
On Fri, Jun 22, 2012 at 8:36 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Thu, Jun 21, 2012 at 4:06 PM, Liu Ping Fan <qemulist@gmail.com> 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
On Sun, Jun 24, 2012 at 3:05 PM, liu ping fan <qemulist@gmail.com> wrote: > On Fri, Jun 22, 2012 at 8:36 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Thu, Jun 21, 2012 at 4:06 PM, Liu Ping Fan <qemulist@gmail.com> 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. Start simple. If you need it to be a pointer later that's fine but for now I see no reason to do the malloc (especially since there is no free!). >>> 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 There's probably a nicer way of doing that than void*. Stefan
Am 25.06.2012 12:04, schrieb Stefan Hajnoczi: > On Sun, Jun 24, 2012 at 3:05 PM, liu ping fan <qemulist@gmail.com> wrote: >> On Fri, Jun 22, 2012 at 8:36 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> On Thu, Jun 21, 2012 at 4:06 PM, Liu Ping Fan <qemulist@gmail.com> wrote: >>>> 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 > > There's probably a nicer way of doing that than void*. CPUState, as requested. Andreas
Am 24.06.2012 16:02, schrieb liu ping fan: > On Fri, Jun 22, 2012 at 8:55 PM, Andreas Färber <afaerber@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 <pingfank@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
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; \ 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)); @@ -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); +} + 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); 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. *
introduce a lock for per-cpu to protect agaist accesing from other vcpu thread. Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> --- cpu-defs.h | 2 ++ cpus.c | 17 +++++++++++++++++ main-loop.h | 3 +++ 3 files changed, 22 insertions(+), 0 deletions(-)