From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dp0qX-0003QB-M1 for qemu-devel@nongnu.org; Mon, 04 Sep 2017 19:32:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dp0qS-0006dJ-Pa for qemu-devel@nongnu.org; Mon, 04 Sep 2017 19:32:01 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:43313 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dp0qS-0006cf-IX for qemu-devel@nongnu.org; Mon, 04 Sep 2017 19:31:56 -0400 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v84NT9n9021674 for ; Mon, 4 Sep 2017 19:31:55 -0400 Received: from e23smtp06.au.ibm.com (e23smtp06.au.ibm.com [202.81.31.148]) by mx0a-001b2d01.pphosted.com with ESMTP id 2cs8rt3v1j-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 04 Sep 2017 19:31:55 -0400 Received: from localhost by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 5 Sep 2017 09:31:52 +1000 Date: Tue, 5 Sep 2017 09:31:47 +1000 From: Sam Bobroff References: <747daf1a700a5340eeeedd9d98188bf89f75bb85.1504161284.git.sam.bobroff@au1.ibm.com> <20170904101945.A6B6EAC041@b01ledav006.gho.pok.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170904101945.A6B6EAC041@b01ledav006.gho.pok.ibm.com> Message-Id: <20170904233147.g4froekilptpud5h@tungsten.ozlabs.ibm.com> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/1] ppc: spapr: Move VCPU ID calculation into sPAPR List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, david@gibson.dropbear.id.au On Mon, Sep 04, 2017 at 12:18:57PM +0200, Greg Kurz wrote: > On Thu, 31 Aug 2017 16:38:46 +1000 > Sam Bobroff wrote: > > > Move the calculation of a CPU's VCPU ID out of the generic PPC code > > (ppc_cpu_realizefn()) and into sPAPR specific code > > (spapr_cpu_core_realize()) where it belongs. > > > > Unfortunately, due to the way things are ordered, we still need to > > default the VCPU ID in ppc_cpu_realizfn() but at least doing that > > doesn't require any interaction with sPAPR. > > > > Signed-off-by: Sam Bobroff > > --- > > This is follow up work arising from my work to clean up the way CPU VCPU IDs are > > handled on PowerPC. It had looked like it would be difficult to move the actual > > VCPU ID calculation out of generic code but it turned out to be OK. > > > > It's based on dgibson/ppc-for-2.11. > > > > Cheers, > > Sam. > > > > hw/ppc/spapr_cpu_core.c | 11 +++++++++++ > > target/ppc/translate_init.c | 18 +++--------------- > > 2 files changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 5e319d9bbb..84dcc6e264 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -211,6 +211,7 @@ error: > > > > static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > { > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > > CPUCore *cc = CPU_CORE(OBJECT(dev)); > > @@ -237,6 +238,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > cs = CPU(obj); > > cpu = POWERPC_CPU(cs); > > cs->cpu_index = cc->core_id + i; > > + cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i; > > According to what we currently have in the generic code, this should be: > > ((cc->core_id + i) / smp_threads) * spapr->vsmt + ((cc->core_id + i) % smp_threads) > > but since cc->core_id is a multiple of smp_threads and i < cc->nr_threads <= smp_threads, > then we can indeed simplify the computation as you did. :) > > \o/ > > Reviewed-by: Greg Kurz Thanks for the review, and the nice simplification was David's idea :-) > > + if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) { > > + error_setg(&local_err, "Can't create CPU with id %d in KVM", > > + cpu->vcpu_id); > > + error_append_hint(&local_err, "Adjust the number of cpus to %d " > > + "or try to raise the number of threads per core\n", > > + cpu->vcpu_id * smp_threads / spapr->vsmt); > > + goto err; > > + } > > + > > > > /* Set NUMA node for the threads belonged to core */ > > cpu->node_id = sc->node_id; > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > index 7f6a349e43..1f7286c893 100644 > > --- a/target/ppc/translate_init.c > > +++ b/target/ppc/translate_init.c > > @@ -9903,28 +9903,15 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) > > PowerPCCPU *cpu = POWERPC_CPU(dev); > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > > Error *local_err = NULL; > > -#if !defined(CONFIG_USER_ONLY) > > - int max_smt = kvmppc_smt_threads(); > > -#endif > > > > cpu_exec_realizefn(cs, &local_err); > > if (local_err != NULL) { > > error_propagate(errp, local_err); > > return; > > } > > - > > -#if !defined(CONFIG_USER_ONLY) > > - cpu->vcpu_id = (cs->cpu_index / smp_threads) * max_smt > > - + (cs->cpu_index % smp_threads); > > - > > - if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) { > > - error_setg(errp, "Can't create CPU with id %d in KVM", cpu->vcpu_id); > > - error_append_hint(errp, "Adjust the number of cpus to %d " > > - "or try to raise the number of threads per core\n", > > - cpu->vcpu_id * smp_threads / max_smt); > > - goto unrealize; > > + if (cpu->vcpu_id == UNASSIGNED_CPU_INDEX) { > > + cpu->vcpu_id = cs->cpu_index; > > } > > -#endif > > > > if (tcg_enabled()) { > > if (ppc_fixup_cpu(cpu) != 0) { > > @@ -10625,6 +10612,7 @@ static void ppc_cpu_initfn(Object *obj) > > CPUPPCState *env = &cpu->env; > > > > cs->env_ptr = env; > > + cpu->vcpu_id = UNASSIGNED_CPU_INDEX; > > > > env->msr_mask = pcc->msr_mask; > > env->mmu_model = pcc->mmu_model; >