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

[Anthony Liguori]

Eduardo Habkost <address@hidden> writes:

> 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?
>
> Anyway, I wouldn't do it in a single step. I prefer doing things one
> small step at a time.

I tend to agree.

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

Andreas is absolutely correct here.  Obviously, it's always better to
represent things as data instead of code.  The basic problem is that
this can't be easily represented as data (particularly when you start
getting into compat properties).

> 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?
>
>
>> 
>> Unfortunately the move of the CPU definitions from config file to C does
>> not eliminate the issue that users can still specify CPU models in their
>> own config files or from the command line, right? Doesn't that mean that
>> either we need to keep all CPU definitions declarative as done here and
>> live with any field duplication between X86CPUInfo and X86CPUClass, or
>> have a special intermediate subclass UserX86CPUClass with such fields
>> for user-specified -cpudef models?
>> Assuming, dropping -cpudef for 1.2 is still not an option.
>
> I would be happy to drop support for cpudef config sections
> completely.

Please add docs to your patch about -cpudef being deprecated.  We'll remove
it for 1.3 provided no one screams.

I'll include something in the 1.2 release notes and ask anyone to
contact qemu-devel if they depend on this feature.

We haven't really done this in the past, but I don't see a reasonable
way to keep supporting -cpudef moving forward.

Regards,

Anthony Liguori

> The only problem is compatibility, but personally I wouldn't mind
> breaking it (I don't think many users have been writing their own
> cpudefs).
>
> Anyway, if we keep supporting it for compatibility (it could be simply
> one separated CPU subclass that uses the config file data), at least we
> don't have the need for versioning/compatibility mechanisms for
> user-provided cpudefs.
>
>> 
>> Regards,
>> Andreas
>> 
>
> -- 
> Eduardo