Message ID | 20121204031145.GG4293@truffula.fritz.box |
---|---|
State | New |
Headers | show |
On 04.12.2012, at 04:11, David Gibson wrote: > On Tue, Dec 04, 2012 at 01:42:17PM +1100, David Gibson wrote: >> From: Mike Qiu <qiudayu@linux.vnet.ibm.com> > > Oops, messed this one up, build error for user only builds. Fixed > version below. > > From d2cc6bcee4737647c4bbbf65d3c0129d4a119dd3 Mon Sep 17 00:00:00 2001 > From: Mike Qiu <qiudayu@linux.vnet.ibm.com> > Date: Tue, 4 Dec 2012 10:52:14 +1100 > Subject: [PATCH] target-ppc: Give a meaningful error if too many threads are > specified > > Currently the target-ppc tcg code only supports a single thread. You can > specify more, but they're treated identically to multiple cores. On KVM > we obviously can't support more threads than the hardware; if more are > specified it will cause strange and cryptic errors. > > This patch clarifies the situation by giving a simple meaningful error if > more threads are specified than we can support. > > Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > target-ppc/translate_init.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index dba572f..bb054a7 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -28,6 +28,7 @@ > #include <kvm.h> > #include "kvm_ppc.h" > #include "arch_init.h" > +#include "cpus.h" > > //#define PPC_DUMP_CPU > //#define PPC_DEBUG_SPR > @@ -10037,6 +10038,10 @@ static int ppc_fixup_cpu(CPUPPCState *env) > > int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def) > { > +#if !defined(CONFIG_USER_ONLY) > + int max_smt = kvmppc_smt_threads(); What if I am using TCG on PPC? Alex > +#endif > + > env->msr_mask = def->msr_mask; > env->mmu_model = def->mmu_model; > env->excp_model = def->excp_model; > @@ -10046,6 +10051,13 @@ int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def) > env->flags = def->flags; > env->bfd_mach = def->bfd_mach; > env->check_pow = def->check_pow; > +#if !defined(CONFIG_USER_ONLY) > + if (smp_threads > max_smt) { > + fprintf(stderr, "Cannot support more than %d threads on PPC with %s\n", > + max_smt, kvm_enabled() ? "KVM" : "TCG"); > + exit(1); > + } > +#endif > > #if defined(TARGET_PPC64) > if (def->sps) > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson >
On Thu, Dec 13, 2012 at 01:57:16PM +0100, Alexander Graf wrote: > > On 04.12.2012, at 04:11, David Gibson wrote: > > > On Tue, Dec 04, 2012 at 01:42:17PM +1100, David Gibson wrote: > >> From: Mike Qiu <qiudayu@linux.vnet.ibm.com> > > > > Oops, messed this one up, build error for user only builds. Fixed > > version below. > > > > From d2cc6bcee4737647c4bbbf65d3c0129d4a119dd3 Mon Sep 17 00:00:00 2001 > > From: Mike Qiu <qiudayu@linux.vnet.ibm.com> > > Date: Tue, 4 Dec 2012 10:52:14 +1100 > > Subject: [PATCH] target-ppc: Give a meaningful error if too many threads are > > specified > > > > Currently the target-ppc tcg code only supports a single thread. You can > > specify more, but they're treated identically to multiple cores. On KVM > > we obviously can't support more threads than the hardware; if more are > > specified it will cause strange and cryptic errors. > > > > This patch clarifies the situation by giving a simple meaningful error if > > more threads are specified than we can support. > > > > Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > target-ppc/translate_init.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index dba572f..bb054a7 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -28,6 +28,7 @@ > > #include <kvm.h> > > #include "kvm_ppc.h" > > #include "arch_init.h" > > +#include "cpus.h" > > > > //#define PPC_DUMP_CPU > > //#define PPC_DEBUG_SPR > > @@ -10037,6 +10038,10 @@ static int ppc_fixup_cpu(CPUPPCState *env) > > > > int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def) > > { > > +#if !defined(CONFIG_USER_ONLY) > > + int max_smt = kvmppc_smt_threads(); > > What if I am using TCG on PPC? Then the stub version of kvmppc_smt_threads() returns 1, and since the PPC TCG doesn't do any SMT emulation, that would seem to be correct...
On 13.12.2012, at 14:17, David Gibson wrote: > On Thu, Dec 13, 2012 at 01:57:16PM +0100, Alexander Graf wrote: >> >> On 04.12.2012, at 04:11, David Gibson wrote: >> >>> On Tue, Dec 04, 2012 at 01:42:17PM +1100, David Gibson wrote: >>>> From: Mike Qiu <qiudayu@linux.vnet.ibm.com> >>> >>> Oops, messed this one up, build error for user only builds. Fixed >>> version below. >>> >>> From d2cc6bcee4737647c4bbbf65d3c0129d4a119dd3 Mon Sep 17 00:00:00 2001 >>> From: Mike Qiu <qiudayu@linux.vnet.ibm.com> >>> Date: Tue, 4 Dec 2012 10:52:14 +1100 >>> Subject: [PATCH] target-ppc: Give a meaningful error if too many threads are >>> specified >>> >>> Currently the target-ppc tcg code only supports a single thread. You can >>> specify more, but they're treated identically to multiple cores. On KVM >>> we obviously can't support more threads than the hardware; if more are >>> specified it will cause strange and cryptic errors. >>> >>> This patch clarifies the situation by giving a simple meaningful error if >>> more threads are specified than we can support. >>> >>> Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>> --- >>> target-ppc/translate_init.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >>> index dba572f..bb054a7 100644 >>> --- a/target-ppc/translate_init.c >>> +++ b/target-ppc/translate_init.c >>> @@ -28,6 +28,7 @@ >>> #include <kvm.h> >>> #include "kvm_ppc.h" >>> #include "arch_init.h" >>> +#include "cpus.h" >>> >>> //#define PPC_DUMP_CPU >>> //#define PPC_DEBUG_SPR >>> @@ -10037,6 +10038,10 @@ static int ppc_fixup_cpu(CPUPPCState *env) >>> >>> int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def) >>> { >>> +#if !defined(CONFIG_USER_ONLY) >>> + int max_smt = kvmppc_smt_threads(); >> >> What if I am using TCG on PPC? > > Then the stub version of kvmppc_smt_threads() returns 1, and since the > PPC TCG doesn't do any SMT emulation, that would seem to be correct... int kvmppc_smt_threads(void) { return cap_ppc_smt ? cap_ppc_smt : 1; } So because we don't call kvm_arch_init, the cap is 0 and thus we return 1? That's quite hard to follow :). Please make this explicit. Only call kvmppc_smt_threads() when kvm_enabled(). Alex
On 13 December 2012 13:19, Alexander Graf <agraf@suse.de> wrote: > > On 13.12.2012, at 14:17, David Gibson wrote: > >> On Thu, Dec 13, 2012 at 01:57:16PM +0100, Alexander Graf wrote: >>> What if I am using TCG on PPC? >> >> Then the stub version of kvmppc_smt_threads() returns 1, and since the >> PPC TCG doesn't do any SMT emulation, that would seem to be correct... > > int kvmppc_smt_threads(void) > { > return cap_ppc_smt ? cap_ppc_smt : 1; > } > > So because we don't call kvm_arch_init, the cap is 0 and thus we return 1? No, if we aren't CONFIG_KVM then that source file and version of the function aren't compiled at all. David means the stub function, which is in kvm_ppc.h: static inline int kvmppc_smt_threads(void) { return 1; } (if you don't like stubs lurking in the header file you could have a target-ppc/kvm-stub.c by analogy with the i386 one I suppose) -- PMM
On 13 December 2012 14:17, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 December 2012 13:19, Alexander Graf <agraf@suse.de> wrote: >> So because we don't call kvm_arch_init, the cap is 0 and thus we return 1? > > No, if we aren't CONFIG_KVM then that source file and version of > the function aren't compiled at all. ...and 5 seconds after sending that I realised what you actually meant. ignore me :-) -- PMM
On Thu, Dec 13, 2012 at 02:18:15PM +0000, Peter Maydell wrote: > On 13 December 2012 14:17, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 13 December 2012 13:19, Alexander Graf <agraf@suse.de> wrote: > >> So because we don't call kvm_arch_init, the cap is 0 and thus we return 1? > > > > No, if we aren't CONFIG_KVM then that source file and version of > > the function aren't compiled at all. > > ...and 5 seconds after sending that I realised what you actually > meant. ignore me :-) And a bit later, so did I. I'll revise.
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index dba572f..bb054a7 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -28,6 +28,7 @@ #include <kvm.h> #include "kvm_ppc.h" #include "arch_init.h" +#include "cpus.h" //#define PPC_DUMP_CPU //#define PPC_DEBUG_SPR @@ -10037,6 +10038,10 @@ static int ppc_fixup_cpu(CPUPPCState *env) int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def) { +#if !defined(CONFIG_USER_ONLY) + int max_smt = kvmppc_smt_threads(); +#endif + env->msr_mask = def->msr_mask; env->mmu_model = def->mmu_model; env->excp_model = def->excp_model; @@ -10046,6 +10051,13 @@ int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def) env->flags = def->flags; env->bfd_mach = def->bfd_mach; env->check_pow = def->check_pow; +#if !defined(CONFIG_USER_ONLY) + if (smp_threads > max_smt) { + fprintf(stderr, "Cannot support more than %d threads on PPC with %s\n", + max_smt, kvm_enabled() ? "KVM" : "TCG"); + exit(1); + } +#endif #if defined(TARGET_PPC64) if (def->sps)