qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]