Re: [Qemu-devel] [PATCH 0/3] Move CPU model definitions to C

[Igor Mammedov <imammedo@redhat.com>]

On 08/01/2012 11:53 PM, Andreas Färber wrote:
> Am 01.08.2012 22:27, schrieb Eduardo Habkost:
>> On Wed, Aug 01, 2012 at 10:04:50PM +0200, Andreas Färber wrote:
>>> Am 01.08.2012 20:45, schrieb Eduardo Habkost:
>>>> This makes the change we discussed on the latest KVM conf call[1], moving the
>>>> existing cpudefs from cpus-x86_64.conf to the C code.
>>>>
>>>> The config file data was converted to C using a script, available at:
>>>> https://gist.github.com/3229602
>>>>
>>>> Except by the extra square brackets around the CPU model names (indicating they
>>>> are built-in models), the output of "-cpu ?dump" is exactly the same before and
>>>> after applying this series.
>>>>
>>>> [1] http://article.gmane.org/gmane.comp.emulators.kvm.devel/95328
>>>>
>>>> Eduardo Habkost (3):
>>>>    i386: add missing CPUID_* constants
>>>>    move CPU models from cpus-x86_64.conf to C
>>>>    eliminate cpus-x86_64.conf file
>>>
>>> If we do this, we will need to refactor the C code again for CPU
>>> subclasses. Can't we (you) do that in one go then? :-)
>>
>> Why again? The refactor for classes would be a one-time mechanical
>> process, won't it?
>
> Because of the resulting second movement sketched below. Like I told you
> some time ago I am seriously scared of loosing some tiny feature bit or
> mixing up some TLAs and breaking things if I had to do that
> touch-all-CPU-definitions refactoring myself. So I'd rather either avoid
> it or have someone who knows that code and/or CPU better do it. So if
> you touch it here that would've been a perfect fit. ;)
I could do this conversion later after as follow up series for 1.3

>
>> Anyway, I wouldn't do it in a single step. I prefer doing things one
>> small step at a time.
>
> Just raising awareness. If that's what people want to do and there's
> volunteers for refactoring and reviewing then fine with me. :-)
>
>>> I thought there was a broad consensus not to go my declarative
>>> X86CPUInfo way but to initialize CPUs imperatively as done for ARM.
>>> That would mean transforming your
>>>
>>> +    {
>>> +        .family = 6,
>>> +        .model = 2,
>>> ...
>>>
>>> into an initfn doing
>>>
>>> -    {
>>> +static void conroe_initfn(Object *obj)
>>> +{
>>> +    X86CPU *cpu = X86_CPU(obj);
>>> +    CPUX86State *env = &cpu->env;
>>> +
>>> -        .family = 6,
>>> +    env->family = 6;
>>> -        .model = 2,
>>> +    env->model = 2;
>>> ...
>>>
>>> or so (not all being as nicely aligned).
>>
>> Really? I am surprised that this is the consensus. Why would one want to
>> transform machine-friendly data into a large set of opaque functions
>> that look all the same and contains exactly the same data inside it, but
>> in a too verbose, machine-unfriendly and refactor-unfriendly way? It
>> doesn't make sense to me.
>>
>> I will look for previous discussions to try to understand the reason for
>> that (was this discussed on qemu-devel?). Do you have any pointers
>> handy?
>
> http://patchwork.ozlabs.org/patch/146704/ ;-)
> and the surrounding couple of series back then (Blue for sparc and Paul
> for arm come to mind, cc'ed).
>
> The key isssue is that class_init gets the class_data pointer (e.g.,
> x86_def_t aka X86CPUInfo) but the initfn doesn't. Thus all data initfn
> needs must either be stored into X86CPUClass (then there's two copies of
> the data) or hardcoded per type in an initfn. For the -cpudef we get
> arbitrary data so we'd need the former solution.
>
> (To add to the confusion, for -cpu ...,x=y we will need to set QOM
> properties on the QOM instance in the future, that's what my setters are
> for that you have helped to review. Disappointing how little progress
> we've made since then...)
That is thing I am working on lately, a 3 days stale state you could see 
at https://github.com/imammedo/qemu/tree/x86-cpu-properties.WIP
I'm going to post completed series next week. I guess it's not candidate 
for 1.3 so no rush.

BTW: Are you going to host qom-next like last time while qemu is in 
freeze for 1.2?
>
> Andreas
>