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
