From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0WXZ-0001Lg-VZ for qemu-devel@nongnu.org; Wed, 11 May 2016 11:59:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0WXU-0006Gd-Mh for qemu-devel@nongnu.org; Wed, 11 May 2016 11:59:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42219) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0WXU-0006GY-El for qemu-devel@nongnu.org; Wed, 11 May 2016 11:59:08 -0400 Date: Wed, 11 May 2016 17:59:03 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Message-ID: <20160511155902.GC1859@potion> References: <1462826940-2422-1-git-send-email-rkrcmar@redhat.com> <20160510195336.GA4457@thinpad.lan.raisama.net> <20160511123733.GG12472@potion> <20160511152624.GH4457@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160511152624.GH4457@thinpad.lan.raisama.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] target-i386: implement CPUID[0xB] (Extended Topology Enumeration) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Paolo Bonzini , Richard Henderson 2016-05-11 12:26-0300, Eduardo Habkost: > On Wed, May 11, 2016 at 02:37:38PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99 w= rote: >> 2016-05-10 16:53-0300, Eduardo Habkost: >> > On Mon, May 09, 2016 at 10:49:00PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: >> >> I looked at a dozen Intel CPU that have this CPUID and all of them >> >> always had Core offset as 1 (a wasted bit when hyperthreading is >> >> disabled) and Package offset at least 4 (wasted bits at <=3D 4 core= s). >> >>=20 >> >> QEMU uses more compact IDs and it doesn't make much sense to change= it >> >> now. I keep the SMT and Core sub-leaves even if there is just one >> >> thread/core; it makes the code simpler and there should be no harm= . >> >=20 >> > If in the future we really want to make the APIC ID offsets match >> > the CPUs you looked at, we can add extra X86CPU properties to >> > allow configuration of APIC ID bit offsets larger than the ones >> > calculated from smp_cores and smp_threads. >>=20 >> Sounds good. No hurry with that as virt has no problem with routing a >> x2APIC cluster that covers two packages and I'm not sure what the >> reasoning for HT is. >>=20 >> > What about compatiblity on migration? With this patch, guests >> > will see CPUID data change when upgrading QEMU. >>=20 >> Ah, thanks, I always forget that QEMU doesn't migrate all configuratio= n >> and has machine types ... >=20 > Even if we did migrate CPUID data (something I have been > considering), changing CPUID on non-live migration would still be > a problem. It's hard to know if CPUID changes are going to > surprise some guest software, even if it's after a VM cold > reboot. >=20 > In this case, I won't be surprised if some software uses CPU > topology information from CPUID[0xB] for license validation. And > I wouldn't want to find that out by getting a bug report from > somebody running it in production a few years from now. >=20 >>=20 >> I don't think that we can directly override values from cpu_x86_cpuid(= ) >> with machine type wrappers. What about adding a compatibility flags >> into X86CPUDefinition and assigning one flag to disable CPUID 0xB, or = a >> global flag? >=20 > Adding a new boolean property to X86CPU (defaulting to true) and > setting it to false on PC_COMPAT_2_6 is the preferred way to do > it. Will do. >> >> Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >> >> --- >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> >> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_= t index, uint32_t count, >> >> + switch (*ecx) { >> >> + case 0: >> >> + *eax =3D apicid_core_offset(smp_cores, smp_threads); >> >> + *ebx =3D smp_threads; >> >> + *ecx |=3D CPUID_TOPOLOGY_LEVEL_SMT; >> >> + break; >> >> + case 1: >> >> + *eax =3D apicid_pkg_offset(smp_cores, smp_threads); >> >> + *ebx =3D smp_cores * smp_threads; >> >> + *ecx |=3D CPUID_TOPOLOGY_LEVEL_CORE; >> >> + break; >> >> + default: >> >> + *eax =3D 0; >> >> + *ebx =3D 0; >> >> + *ecx |=3D CPUID_TOPOLOGY_LEVEL_INVALID; >> >> + } >> >> + >> >> + /* Preserve reserved bits. Extremely unlikely to make a d= ifference. */ >> >> + *eax &=3D 0x1f; >> >> + *ebx &=3D 0xffff; >> >=20 >> > That means we don't know how to handle apicid_*_offset() >=3D 32, >> > smp_threads > 2^16, or smp_cores * smp_threads > 2^16. If that >> > happen one day, I would prefer to see QEMU abort than silently >> > truncating data in CPUID[0xB]. What about an assert()? >>=20 >> I skipped an assert because the spec says that *ebx cannot be taken >> seriously, so killing the guest didn't seem worth it: >> Software must not use EBX[15:0] to enumerate processor topology of t= he >> system. This value in this field (EBX[15:0]) is only intended for >> display/diagnostic purposes. The actual number of logical processors >> available to BIOS/OS/Applications may be different from the value of >> EBX[15:0], depending on software and platform hardware configuration= s. >>=20 >> I'd warn, but I don't know if 'printf' is ok, so I skipped it too, >> because the overflow really doesn't matter. >=20 > We still have *eax, that is documented as "Software should use > this field to enumerate processor topology of the system". >=20 > But I don't mind if you prefer to not assert(). If in the future > somebody decide to support smp_threads * smp_cores >=3D 2^32 > (that would make apicid_pkg_offset() >=3D 32), we can let them > decide what should be reported in CPUID[0xB] and fix this. eax can store offsets up to 31 and edx cannot store more than 32 bits, so it should trip elsewhere. I'll add an assert for eax, though. Thanks.