Re: [Qemu-devel] [PATCH v2] i386: Use #defines instead of magic numbers for CPUID cache information

[Andreas Färber]

Am 30.08.2013 21:49, schrieb Eduardo Habkost:
> On Fri, Aug 30, 2013 at 04:51:20PM +0200, Andreas Färber 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.
>>>
>>> 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).
>>>
>>> This even keeps the code bug-compatible with the previous version: today
>>> the cache information returned on AMD cache information leaves (CPUID
>>> 0x80000005 & 0x80000006) do not match the information returned on CPUID
>>> 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.
>>>
>>> Signed-off-by: Eduardo Habkost <address@hidden>
>>> Reviewed-by: liguang <address@hidden>
>>> ---
>>> Changes v1 -> v2:
>>>  * s/leafs/leaves/ on code comments
>>> ---
>>>  target-i386/cpu.c | 184 
>>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 162 insertions(+), 22 deletions(-)
>>
>> 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?
>>
>> 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 do
>> so myself, the former being appreciated. :)
> 
> In case anybody wants to verify it: compile it before/after applying the
> patch, with:
>   make CFLAGS='-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.

So the only difference I see is movl lines changing slightly before
object_dynamic_cast_assert() calls, so I'm applying it to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

(Since for backporting commits I consider it more important to have
consistent prefixes, I have shortened the end of the subject instead.)

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg