Re: [Qemu-devel] [uq/master PATCH 7/7 v8] target-i386: CPU model subclasses

Andreas Färber
2014-02-10T00:23:43Z

Am 31.01.2014 19:13, schrieb Eduardo Habkost:
> Register separate QOM classes for each x86 CPU model.
>
> This will allow management code to more easily probe what each CPU model
> provides, by simply creating objects using the appropriate class name,
> without having to restart QEMU.
>
> This also allows us to eliminate the qdev_prop_set_globals_for_type()
> hack to set CPU-model-specific global properties.
>
> Instead of creating separate class_init functions for each class, I just
> used class_data to store a pointer to the X86CPUDefinition struct for
> each CPU model. This should make the patch shorter and easier to review.
> Later we can gradually convert each X86CPUDefinition field to lists of
> per-class property defaults.
>
> Written based on the ideas from the patch "[RFC v5] target-i386: Slim
> conversion to X86CPU subclasses + KVM subclasses" written by Andreas
> Färber <afaerber@suse.de>, Igor Mammedov <imammedo@redhat.com>.
>
> The "host" CPU model is special, as the feature flags depend on KVM
> being initialized. So it has its own class_init and instance_init
> function, and feature flags are set on instance_init instead of
> class_init.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> This patch is similar to the one sent by Andrea and then later
> resubmitted by Igor as "[RFC v5] target-i386: Slim conversion to X86CPU
> subclasses + KVM subclasses", as it doesn't create one new class_init
> function for each subclass.
>
> Main differences v5 -> v6 are:
> * Code was written from scratch (instead of using the previous patches
> as base)
> * I didn't mean to rewrite it entirely, but when doing additional
> simplification of the CPU init logic on other patches, I ended up
> rewriting it.
> * I chose to keep the Signed-off-by lines because I built upon
> Andreas's and Igor's ideas. Is that OK?

Yes, your From and our Sobs in order is the expected way in this case.
If Igor agrees I would propose to drop the textual repetition of this.

I am ~1/3 through reviewing this and it looks pretty promising so far!
Thanks a lot for your efforts. Meanwhile one cleanup idea inline...

> * No KVM-specific subclasses, to keep things simpler.
> * No embedding of X86CPUDefinition (x86_def_t) inside the class struct,
> instead keeping a pointer to the existing X86CPUDefinition struct.
> * The "host" class is registered on cpu.c, but the CPUID data
> is filled on instance_init instead of class_init (because KVM has to
> be initialized already).
> * kvm_required field introduced to make sure the "host" class can't
> be used without KVM.
>
> Changes v6 -> v7:
> * Rebase
>
> Changes v7 -> v8:
> * Removed CPU listing code (will be sent as a separate patch)
> * Kept x86_cpudef_setup() (will be addressed in a separate patch)
> ---
> target-i386/cpu-qom.h | 13 ++++
> target-i386/cpu.c | 197 ++++++++++++++++++++++++++++++++------------------
> 2 files changed, 138 insertions(+), 72 deletions(-)
>
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 722f11a..60c5c32 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -37,6 +37,9 @@
> #define X86_CPU_GET_CLASS(obj) \
> OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
>
> +
> +typedef struct X86CPUDefinition X86CPUDefinition;
> +
> /**
> * X86CPUClass:
> * @parent_realize: The parent class' realize handler.
> @@ -49,6 +52,16 @@ typedef struct X86CPUClass {
> CPUClass parent_class;
> /*< public >*/
>
> + /* CPU model definition
> + * Should be eventually replaced by subclass-specific property defaults
> + */
> + X86CPUDefinition *cpu_def;
> + /* CPU model requires KVM to be enabled */
> + bool kvm_required;
> + /* Optional description of CPU model.
> + * If unavailable, cpu_def->model_id is used */
> + const char *model_description;

Here I wondered why you needed this? For PowerPCCPU subclasses we have
reused DeviceClass::desc.

Regards,
Andreas

> +
> DeviceRealize parent_realize;
> void (*parent_reset)(CPUState *cpu);
> } X86CPUClass;
[snip]

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