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

["Andreas Färber" <afaerber@suse.de>]

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