[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/3] Move CPU model definitions to C
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 0/3] Move CPU model definitions to C |
Date: |
Thu, 2 Aug 2012 09:01:18 -0300 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Aug 01, 2012 at 03:41:56PM -0500, Anthony Liguori wrote:
> 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).
I agree that sometimes we can't represent everything easily as data and
some code is required. I just don't see yet why that would be the case
for the CPU model data. The fact that we're insisting in using global
properties to implement the compat code is a demonstration that it can
be completely data-driven, isn't it?
We also need to expose some of the CPU model information to libvirt, so
we will surely need to keep some of that information available as data.
>
> > 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 do.
>
> 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
--
Eduardo