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

[Andreas Färber]

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. ;)

> 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...)

Andreas

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