Re: [libvirt] [qom-cpu PATCH 7/7 v9] target-i386: CPU model subclasses
On Mon, Feb 10, 2014 at 11:39:51PM +0100, Andreas Färber wrote:
Am 10.02.2014 11:21, schrieb Eduardo Habkost:
> +static const TypeInfo x86_cpu_host_type_info = {
> + .name = CPU_CLASS_NAME("host"),
> + .parent = TYPE_X86_CPU,
> + .instance_size = sizeof(X86CPU),
> + .instance_init = x86_cpu_instance_init_host,
> + .abstract = false,
> + .class_size = sizeof(X86CPUClass),
> + .class_init = x86_cpu_class_init_host,
> +};
This looks broken: .class_data is not set but the loading of the cpudef
happens in the TYPE_X86_CPU initfn. My preferred solution would be to
move the cpudef-loading from TYPE_X86_CPU's instance_init to a separate
one specified for the models only, allowing non-cpudef-based models. Not
finished investigating yet.
class_data is not set because x86_cpu_class_init_host doesn't use it.
x86_cpu_class_init_host() simply points fills host_cpu_def and makes
xcc->cpu_def point to it. Why would we need a separate instance_init
only for the other models? x86_cpu_initfn() already works for everything
except the ->features field (that is then handled by
x86_cpu_instance_init_host()).
For now I've prepended a patch implementing my generalized
CPUClass::class_by_name instead of a custom x86_cpu_class_by_name().
Sounds good to me.
Other style nits that I'm working on cleaning up are declarations in the
middle of blocks, keeping _class_init naming convention (pretty sure my
patches always had the most-specific-to-least-specific naming), strictly
distinguishing between type and class, adding to my gtk-style
documentation rather than new custom comments, placing struct
documentation in the header and keeping the diff nicely readable AFAP.
I'd further like to keep some other conventions from previous CPU
subclasses, like pulling the model for loop out of the model
registration function.
My patches had always tried to turn what is now x86_cpu_load_def() into
an instance_init function rather than calling it from one - did you have
reasons not to?
I like to keep the "simply move stuff from X86CPUDefinition to X86CPU"
logic in a separate place. I think it makes the code more readable (and
it also made the code movements more obvious because I didn't have to
move all the code that's inside load_def(), just the load_def() call.
But if you want to inline it inside instance_init, I wouldn't mind.
Did you consider converting the host model in a first step to make the
patch smaller?
Converting the host model would require moving some logic to
instance_init but keeping the strcmp(name, "host") hack. I thought it
would involve more complex intermediate steps so I chose not to try it.
--
Eduardo