| Message ID | 1472797976-24210-3-git-send-email-nikunj@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 2 Sep 2016 12:02:54 +0530 Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- Shouldn't this patch be the last one, when all other issues have been addressed ? > target-ppc/kvm.c | 2 +- > target-ppc/kvm_ppc.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index dcb68b9..20eb450 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) > > int kvmppc_smt_threads(void) > { > - return cap_ppc_smt ? cap_ppc_smt : 1; > + return cap_ppc_smt ? cap_ppc_smt : 8; If KVM is there but does not support SMT processor modes, it looks wrong to return anything but 1. This check needs kvm_enabled(). Also, why 8 ? This depends on the CPU model. Also, since real HW allows to choose the SMT mode, maybe this should be configurable from the command line as well. > } > > #ifdef TARGET_PPC64 > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > index 5461d10..053db0a 100644 > --- a/target-ppc/kvm_ppc.h > +++ b/target-ppc/kvm_ppc.h > @@ -128,7 +128,7 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) > > static inline int kvmppc_smt_threads(void) > { > - return 1; > + return 8; Same remark. > } > > static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits) Cheers. -- Greg
Greg Kurz <groug@kaod.org> writes: > On Fri, 2 Sep 2016 12:02:54 +0530 > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> --- > > Shouldn't this patch be the last one, when all other issues have been addressed ? > >> target-ppc/kvm.c | 2 +- >> target-ppc/kvm_ppc.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> index dcb68b9..20eb450 100644 >> --- a/target-ppc/kvm.c >> +++ b/target-ppc/kvm.c >> @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) >> >> int kvmppc_smt_threads(void) >> { >> - return cap_ppc_smt ? cap_ppc_smt : 1; >> + return cap_ppc_smt ? cap_ppc_smt : 8; > > If KVM is there but does not support SMT processor modes, it looks > wrong to return anything but 1. This check needs kvm_enabled(). This also gets called when emulating PPC on PPC. > Also, why 8 ? This depends on the CPU model. Not sure if I need to emulate according to the host cpu model. I had selected 8, as that was the highest number of threads possible for POWER. > Also, since real HW allows to choose the SMT mode, maybe this should > be configurable from the command line as well. > >> } >> >> #ifdef TARGET_PPC64 >> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h >> index 5461d10..053db0a 100644 >> --- a/target-ppc/kvm_ppc.h >> +++ b/target-ppc/kvm_ppc.h >> @@ -128,7 +128,7 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) >> >> static inline int kvmppc_smt_threads(void) >> { >> - return 1; >> + return 8; > > Same remark. > >> } >> >> static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits) Regards Nikunj
On Fri, 02 Sep 2016 15:04:47 +0530 Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > Greg Kurz <groug@kaod.org> writes: > > > On Fri, 2 Sep 2016 12:02:54 +0530 > > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > > > >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > >> --- > > > > Shouldn't this patch be the last one, when all other issues have been addressed ? > > > >> target-ppc/kvm.c | 2 +- > >> target-ppc/kvm_ppc.h | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > >> index dcb68b9..20eb450 100644 > >> --- a/target-ppc/kvm.c > >> +++ b/target-ppc/kvm.c > >> @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) > >> > >> int kvmppc_smt_threads(void) > >> { > >> - return cap_ppc_smt ? cap_ppc_smt : 1; > >> + return cap_ppc_smt ? cap_ppc_smt : 8; > > > > If KVM is there but does not support SMT processor modes, it looks > > wrong to return anything but 1. This check needs kvm_enabled(). > > This also gets called when emulating PPC on PPC. > Yes and the current value of 1 is the default for non HV KVM and TCG. If you want to change the default for MTTCG, then you need separate paths. > > Also, why 8 ? This depends on the CPU model. > > Not sure if I need to emulate according to the host cpu model. I had > selected 8, as that was the highest number of threads possible for POWER. > If running in full emulation and cpu type is POWER7, this shouldn't be higher than 4. In the end, something like: int kvmppc_smt_threads(void) { if (kvm_enabled()) { return cap_ppc_smt ? cap_ppc_smt : 1; } else { return 8_or_4_or_2_or_1_depending_on_the_cpu_model; } } Cheers. -- Greg > > Also, since real HW allows to choose the SMT mode, maybe this should > > be configurable from the command line as well. > > > >> } > >> > >> #ifdef TARGET_PPC64 > >> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > >> index 5461d10..053db0a 100644 > >> --- a/target-ppc/kvm_ppc.h > >> +++ b/target-ppc/kvm_ppc.h > >> @@ -128,7 +128,7 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) > >> > >> static inline int kvmppc_smt_threads(void) > >> { > >> - return 1; > >> + return 8; > > > > Same remark. > > > >> } > >> > >> static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits) > > Regards > Nikunj >
Greg Kurz <groug@kaod.org> writes: > On Fri, 02 Sep 2016 15:04:47 +0530 > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > >> Greg Kurz <groug@kaod.org> writes: >> >> > On Fri, 2 Sep 2016 12:02:54 +0530 >> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: >> > >> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> >> --- >> > >> > Shouldn't this patch be the last one, when all other issues have been addressed ? >> > >> >> target-ppc/kvm.c | 2 +- >> >> target-ppc/kvm_ppc.h | 2 +- >> >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> >> index dcb68b9..20eb450 100644 >> >> --- a/target-ppc/kvm.c >> >> +++ b/target-ppc/kvm.c >> >> @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) >> >> >> >> int kvmppc_smt_threads(void) >> >> { >> >> - return cap_ppc_smt ? cap_ppc_smt : 1; >> >> + return cap_ppc_smt ? cap_ppc_smt : 8; >> > >> > If KVM is there but does not support SMT processor modes, it looks >> > wrong to return anything but 1. This check needs kvm_enabled(). >> >> This also gets called when emulating PPC on PPC. >> > > Yes and the current value of 1 is the default for non HV KVM and TCG. If > you want to change the default for MTTCG, then you need separate paths. > >> > Also, why 8 ? This depends on the CPU model. >> >> Not sure if I need to emulate according to the host cpu model. I had >> selected 8, as that was the highest number of threads possible for POWER. >> > > If running in full emulation and cpu type is POWER7, this shouldn't be > higher than 4. > > In the end, something like: > > int kvmppc_smt_threads(void) > { > if (kvm_enabled()) { > return cap_ppc_smt ? cap_ppc_smt : 1; > } else { > return 8_or_4_or_2_or_1_depending_on_the_cpu_model; > } > } Sure, will need something like this. Regards, Nikunj
On Fri, Sep 02, 2016 at 12:02:54PM +0530, Nikunj A Dadhania wrote: > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > target-ppc/kvm.c | 2 +- > target-ppc/kvm_ppc.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index dcb68b9..20eb450 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) > > int kvmppc_smt_threads(void) > { > - return cap_ppc_smt ? cap_ppc_smt : 1; > + return cap_ppc_smt ? cap_ppc_smt : 8; > } > > #ifdef TARGET_PPC64 > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > index 5461d10..053db0a 100644 > --- a/target-ppc/kvm_ppc.h > +++ b/target-ppc/kvm_ppc.h > @@ -128,7 +128,7 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) > > static inline int kvmppc_smt_threads(void) > { > - return 1; > + return 8; > } > > static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits) This doesn't make sense to me. Allowing multiple hardware threads to be emulated in TCG (which I think will require a bunch of fixes) seems like a separate question to MTTCG - i.e. allowing separate vcpus to be TCG emulated in separate host threads.
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Fri, Sep 02, 2016 at 12:02:54PM +0530, Nikunj A Dadhania wrote: >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> --- >> target-ppc/kvm.c | 2 +- >> target-ppc/kvm_ppc.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> index dcb68b9..20eb450 100644 >> --- a/target-ppc/kvm.c >> +++ b/target-ppc/kvm.c >> @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) >> >> int kvmppc_smt_threads(void) >> { >> - return cap_ppc_smt ? cap_ppc_smt : 1; >> + return cap_ppc_smt ? cap_ppc_smt : 8; >> } >> >> #ifdef TARGET_PPC64 >> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h >> index 5461d10..053db0a 100644 >> --- a/target-ppc/kvm_ppc.h >> +++ b/target-ppc/kvm_ppc.h >> @@ -128,7 +128,7 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) >> >> static inline int kvmppc_smt_threads(void) >> { >> - return 1; >> + return 8; >> } >> >> static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits) > > This doesn't make sense to me. Allowing multiple hardware threads to > be emulated in TCG (which I think will require a bunch of fixes) seems > like a separate question to MTTCG - i.e. allowing separate vcpus to be > TCG emulated in separate host threads. Right, I was planning to drop this for now and introduce this later. As noted in the other thread, i did not consider the multi-core case. Regards, Nikunj
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index dcb68b9..20eb450 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -2090,7 +2090,7 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) int kvmppc_smt_threads(void) { - return cap_ppc_smt ? cap_ppc_smt : 1; + return cap_ppc_smt ? cap_ppc_smt : 8; } #ifdef TARGET_PPC64 diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h index 5461d10..053db0a 100644 --- a/target-ppc/kvm_ppc.h +++ b/target-ppc/kvm_ppc.h @@ -128,7 +128,7 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy) static inline int kvmppc_smt_threads(void) { - return 1; + return 8; } static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits)
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- target-ppc/kvm.c | 2 +- target-ppc/kvm_ppc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)