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

[Eduardo Habkost]

On Wed, Aug 01, 2012 at 11:53:51PM +0200, 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. ;)
> 
> > 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 volunteer to help on that. Whatever approach we use, we can make it as
mechanically as possible (thus helping to avoid human errors in the
conversaion).

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

Well, we can simply just store everything in the X86CPUClass.

I don't see the existence of two copies of the data as a problem, as
long as the first copy is used only once (while creating the second
copy). But I think we can still avoid that and keep the implementation
data-driven.

Anyway, now we're talking about 1.3, right? Doing this will be much
easier once we eliminate the support for cpudef config sections, and we
won't eliminate it on 1.2 yet.

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

I have been avoiding touching that code by now to avoid conflicts, as we
already have 2 people working at exactly the same code.

-- 
Eduardo