qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC] target-i386: register a class for each CPU model


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [RFC] target-i386: register a class for each CPU model
Date: Mon, 26 Nov 2012 15:22:34 -0200
User-agent: Mutt/1.5.21 (2010-09-15)

On Sun, Nov 25, 2012 at 08:32:46PM +0100, Andreas Färber wrote:
> Am 12.11.2012 22:48, schrieb Eduardo Habkost:
> > This creates the following class hierarchy:
> > 
> > - TYPE_X86_CPU ("<arch>-cpu")
> >  - TYPE_X86_DEFCPU "<arch>-cpu-predefined": abstract base class for the
> >    predefined CPU models
> >    - "<arch>-cpu-model-<model>": a class for each predefined CPU model
> >  - TYPE_X86_HOST_CPU ("<arch>-cpu-model-host"): class for the "-cpu host" 
> > CPU
> >    model
> 
> Although KVM was invented in Israel, we usually use left-to-right
> naming, i.e. foo-<arch>-cpu. :)

But I am in Brazil, and in portuguese adjectives normally come after
nouns.  ;-)

I will change the naming scheme in the next version (and try to reuse
the existing name->class mapping code from other targets, and make it
reusable by all targets).


> 
> > 
> > On TARGET_X86_64, "<arch>" is "x86_64", on TARGET_I386, "<arch>" is
> > "i386".
> > 
> > The trick here is to replace exactly what's implemented in the
> > cpu_x86_find_cpudef() logic and nothing else. So, the only difference in
> > relation to the previous code is that instead of looking at the CPU model 
> > table
> > on cpu_x86_find_cpudef(), we just use the right CPU class, that will fill 
> > the
> > X86CPUDefinition struct on a ->init_cpudef() method.
> > 
> > The init_cpudef() method was added jut to not require us to kill the
> > X86CPUDefinition array in the same patch. But the X86CPUDefinition
> > struct may eventually go away, and all the default-value initialization
> > for each CPU model can become just different defaults set on
> > instance_init() or class_init().
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > I am using those class names because I don't want the CPU model names to 
> > live
> > in the same namespace as all device names. I want to avoid doing the same
> > mistake that was done in the arm code, that registers a QOM class named 
> > "any".
> > Some CPU model names, like "qemu64", don't make any sense unless you already
> > know it is a CPU model name.
> > 
> > As an alternative to the complex naming above, I was considering simply 
> > using
> > "cpu-<model>" as the class name. I don't know what others think.
> 
> To be honest, this is not what I had in mind, it is still pretty close
> to my original throw at x86 CPU subclasses.
> As you mention there's two more modern alternatives, instance_init per
> model or class_init per model. I thought I heard someone voice a need to
> inspect classes rather than objects, so that would speak for class_init,
> meaning we would need to convert
>     .ext2_features = foo | bar | baz,
> to
>     xcc->ext2_features = foo | bar | baz;
> without loosing any of those stupid flags. :-/

Right. Or, even better: we can use a CPU property initialization string
on each class.

On either case, the advantage is that if the CPU class initialization is
code, we could easily make a new model reuse parts of other models
(instead of copying & pasting the previous model definition, and then
adding the new features).

Basically, the main reason I was keeping x86_def_t (X86CPUDefinition)
around was that I would like to keep the CPU models instrospectable so
we could expose their information to libvirt later. But implementing the
CPU model differences on class_init functions will allow us to keep the
CPU model information introspectable anyway, so the imperative approach
should work.


> The upside is that in a class_init we can call any KVM ioctl we need, as
> long as the class is not accessed before KVM initialization.

Being able to call KVM ioctl()s on class_init would make the code much
simpler. e.g.: instead of adding a init_cpudef() method (that was
introduced just because of -cpu host), we could just initialize the CPU
features inside class_init.

Would it be safe to assume we can do that? I would love to be able to
call KVM ioctl()s on class_init, but then anybody writing any code that
could trigger a CPU class initialization would need to be very careful
to never do that before KVM is initialized.


> The
> previous review comment of not duplicating values between definition and
> class would be resolved by only having them in the class.

True. Igor convinced me that once we finish the CPU properties work, it
will be very easy to implement this by simply using different class_init
functions.

Maybe we will need an easier mechanism to allow subclasses to have
different property defaults from the parent, without the need to
duplicate the list of properties on each class (in other words: parent
class could register the properties and their types, subclasses would
only change the property default value). But probably it's easier to
discuss this implementation detail when we have an actual patch to do
that.


> The QOM properties that I had already prepared for this would then be
> called after instantiating the CPU for handling the extra -cpu arguments
> only. More qdev or whatever CPU properties are then only needed for
> versioning of changing machine defaults and later for
> inspecting/changing things via QMP.
> 
> It would be possible to do something like this patch as an intermediate
> step to get there, but then we would be changing the hierarchy back and
> forth... hmmm.

I think we can have it implemented the right way since the beginning. it
will be much easier after we have the CPU properties work included.

I have done this as an experiment, while we were still wondering if it
would be possible to implement the CPU classes before the CPU
properties, on 1.3.


> Your new header cleanup series looks good, so does part of the init
> cleanups series, I'll review it in-depth the coming week.

Igor said he thought the init cleanup series would force him to redo the
CPU properties work (that already got some review), so maybe we can do
the init cleanup only after including the CPU properties work done by
him.

I sent the init cleanup series because I prefer to clean the code first,
and change behavior later (so the patches that change behavior are
clearer and easier to review). But would be OK with either approach.


> 
> Maybe we can find a way to convert -cpu host before we dive into the big
> mess? And maybe we can generalize my experimental SuperH CPU name->class
> mapping to the CPU base class? (just thinking loud)

I am not sure we would want to convert -cpu host first. Probably it will
be easier to do it only after we are ready to convert everything, so we
don't need to add hacks specific for -cpu host.

About the name->class mapping: I want to make all targets use a common
method for that. I didn't know that SuperH already had some name->class
mapping code (I was only looking at the ARM name->class code). I will
take a look.

> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo



reply via email to

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