From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35670) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFUhs-0002aN-3N for qemu-devel@nongnu.org; Fri, 30 Aug 2013 15:50:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VFUhl-0006sX-H8 for qemu-devel@nongnu.org; Fri, 30 Aug 2013 15:50:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53534) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFUhl-0006s7-9p for qemu-devel@nongnu.org; Fri, 30 Aug 2013 15:50:01 -0400 Date: Fri, 30 Aug 2013 16:49:52 -0300 From: Eduardo Habkost Message-ID: <20130830194952.GC3041@otherpad.lan.raisama.net> References: <1377617077-25174-1-git-send-email-ehabkost@redhat.com> <5220B168.8020703@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5220B168.8020703@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] i386: Use #defines instead of magic numbers for CPUID cache information List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , Igor Mammedov , qemu-devel@nongnu.org, liguang On Fri, Aug 30, 2013 at 04:51:20PM +0200, Andreas F=E4rber wrote: > Am 27.08.2013 17:24, schrieb Eduardo Habkost: > > This is an attempt to make the CPUID cache topology code clearer, by > > replacing the magic numbers in the code with #defines, and moving all > > the cache information to the same place in the file. > >=20 > > I took care of comparing the assembly output of compiling > > target-i386/cpu.c before and after applying this change, to make sure > > not a single bit was changed on cpu_x86_cpuid() before and after > > applying this patch (unfortunately I had to manually check existing > > differences, because of __LINE__ expansions on > > object_class_dynamic_cast_assert() calls). > >=20 > > This even keeps the code bug-compatible with the previous version: to= day > > the cache information returned on AMD cache information leaves (CPUID > > 0x80000005 & 0x80000006) do not match the information returned on CPU= ID > > leaves 2 and 4. The L2 cache information on CPUID leaf 2 also doesn't > > match the information on CPUID leaf 2. The new constants should make = it > > easier to eventually fix those inconsistencies. All inconsistencies I > > have found are documented in code comments. > >=20 > > Signed-off-by: Eduardo Habkost > > Reviewed-by: liguang > > --- > > Changes v1 -> v2: > > * s/leafs/leaves/ on code comments > > --- > > target-i386/cpu.c | 184 ++++++++++++++++++++++++++++++++++++++++++++= +++------- > > 1 file changed, 162 insertions(+), 22 deletions(-) >=20 > I think this is a good idea and the code looked sane, but it is unclear > to me from v1 whether Li Guang has verified as part of his review that > all the bits match the original ones or just that Coding Style and > general idea is okay? >=20 > I'm therefore holding off applying this one for today's pull, waiting > until either someone confirms Eduardo's results or I find the time to d= o > so myself, the former being appreciated. :) In case anybody wants to verify it: compile it before/after applying the patch, with: make CFLAGS=3D'-save-temps -DNDEBUG and save x86_64-softmmu/cpu.s file from each run. You are going to see some differences between both files due to __LINE__ being used as argument to object*_dynamic_cast_assert(), but nothing else. --=20 Eduardo