From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51061) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ek9La-0003c9-L6 for qemu-devel@nongnu.org; Fri, 09 Feb 2018 09:08:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ek9LV-000130-Nz for qemu-devel@nongnu.org; Fri, 09 Feb 2018 09:08:14 -0500 Received: from 19.mo3.mail-out.ovh.net ([178.32.98.231]:58320) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ek9LV-00010r-HL for qemu-devel@nongnu.org; Fri, 09 Feb 2018 09:08:09 -0500 Received: from player758.ha.ovh.net (b6.ovh.net [213.186.33.56]) by mo3.mail-out.ovh.net (Postfix) with ESMTP id D1D99195C8E for ; Fri, 9 Feb 2018 15:08:07 +0100 (CET) Date: Fri, 9 Feb 2018 15:06:49 +0100 From: Greg Kurz Message-ID: <20180209150649.7f557ff9@bahia.lan> In-Reply-To: <20180209081858.27013-1-lvivier@redhat.com> References: <20180209081858.27013-1-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] spapr: set vsmt to MAX(8, smp_threads) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, David Gibson On Fri, 9 Feb 2018 09:18:58 +0100 Laurent Vivier wrote: > We ignore silently the value of smp_threads when we set > the default VSMT value, and if smp_threads is greater than VSMT > kernel is going into trouble later. > Hi Laurent, I've looked a bit more and I'm not sure what kernel troubles you're referring to, but several places in QEMU where we use kvm_ppc_smt() later on do assume that smp_threads > kvm_ppc_smt(). Basically, everywhere we compute a vCPU id: In spapr_init_cpus() when creating DRC connectors: int core_id = i * smp_threads; if (mc->has_hotpluggable_cpus) { spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU, (core_id / smp_threads) * smt); } or in spapr_cpu_core_realize() when creating vCPUs: cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i; It is visible by adding some printfs in the current code base. This is what happens when passing -smp cores=2,threads=16 without your patch: DRC connector to vcpu_id 0 CPU vcpu_id 0 CPU vcpu_id 1 CPU vcpu_id 2 CPU vcpu_id 3 CPU vcpu_id 4 CPU vcpu_id 5 CPU vcpu_id 6 CPU vcpu_id 7 CPU vcpu_id 8 CPU vcpu_id 9 CPU vcpu_id 10 CPU vcpu_id 11 CPU vcpu_id 12 CPU vcpu_id 13 CPU vcpu_id 14 CPU vcpu_id 15 DRC connector to vcpu_id 8 ^^^ should be 16 CPU vcpu_id 8 ^^^ should start numbering at 16 CPU vcpu_id 9 CPU vcpu_id 10 CPU vcpu_id 11 CPU vcpu_id 12 CPU vcpu_id 13 CPU vcpu_id 14 CPU vcpu_id 15 CPU vcpu_id 16 CPU vcpu_id 17 CPU vcpu_id 18 CPU vcpu_id 19 CPU vcpu_id 20 CPU vcpu_id 21 CPU vcpu_id 22 CPU vcpu_id 23 qemu-system-ppc64: kvm_init_vcpu failed: File exists ^^^^ CPU 8 already created by the first core I'm not feeling comfortable with the rest of the code silently depending on the fact that spapr_set_vsmt_mode() terminates QEMU if it cannot enforce smp_threads <= kvm_ppc_smt(). Anyway, with your patch, the same command line as above gives: qemu-system-ppc64: Failed to set KVM's VSMT mode to 16 (errno -22) On PPC, a VM with 16 threads/core on a host with 8 threads/core requires the use of VSMT mode 16. This KVM seems to be too old to support VSMT. This hammer is big enough to fix the vCPU ids miscalculations, so: Reviewed-by: Greg Kurz > Fixes: 8904e5a750 > ("spapr: Adjust default VSMT value for better migration compatibility") > > Signed-off-by: Laurent Vivier > --- > > Notes: > v3: use MAX(8, smp_threads) and let KVM to return an error > if nb_threads is too big > update subject to reflect the change > > v2: display a specific error message when the default VSMT is used > fix subject > > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 32a876be56..c8a1eefa17 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2310,7 +2310,7 @@ static void spapr_set_vsmt_mode(sPAPRMachineState *spapr, Error **errp) > * the value that we'd get with KVM on POWER8, the > * overwhelmingly common case in production systems. > */ > - spapr->vsmt = 8; > + spapr->vsmt = MAX(8, smp_threads); > } > > /* KVM: If necessary, set the SMT mode: */