Re: [Qemu-devel] [PATCH qom-cpu-next v5] target-i386: Split command line parsing out of cpu_x86_register()

2013-02-16 Thread Andreas Färber 
Am 15.02.2013 14:06, schrieb Igor Mammedov:
 From: Andreas Färber afaer...@suse.de
 
 In order to instantiate a CPU subtype we will need to know which type,
 so move the cpu_model splitting into cpu_x86_init().
 
 Parameters need to be set on the X86CPU instance, so move
 cpu_x86_parse_featurestr() into cpu_x86_init() as well.
 
 This leaves cpu_x86_register() operating on the model name only.
 
 Signed-off-by: Andreas Färber afaer...@suse.de
 Signed-off-by: Igor Mammedov imamm...@redhat.com
 ---
  v5:
   * get error to report from cpu_x86_register()
  v4:
   * consolidate resource cleanup in when leaving cpu_x86_init(),
 to avoid clean code duplication.
   * remove unnecessary error message from hw/pc.c

This version still has the flaw of printing an x86-specific error
message in the model-not-found NULL return case, leading to duplicate
error messages for qemu-i386 / qemu-x86_64.

But I think the progress towards x86 hotplug outweighs that nit, and
adding #ifdef TARGET_I386 to linux-user and bsd-user seemed
unnecessarily ugly to me. Fixing this (or q35?) can be done as follow-up.

Thanks, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

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

[Qemu-devel] [PATCH qom-cpu-next v5] target-i386: Split command line parsing out of cpu_x86_register()

2013-02-15 Thread Igor Mammedov 
From: Andreas Färber afaer...@suse.de

In order to instantiate a CPU subtype we will need to know which type,
so move the cpu_model splitting into cpu_x86_init().

Parameters need to be set on the X86CPU instance, so move
cpu_x86_parse_featurestr() into cpu_x86_init() as well.

This leaves cpu_x86_register() operating on the model name only.

Signed-off-by: Andreas Färber afaer...@suse.de
Signed-off-by: Igor Mammedov imamm...@redhat.com
---
 v5:
  * get error to report from cpu_x86_register()
 v4:
  * consolidate resource cleanup in when leaving cpu_x86_init(),
to avoid clean code duplication.
  * remove unnecessary error message from hw/pc.c
---
 hw/pc.c   |1 -
 target-i386/cpu.c |   80 ++--
 2 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 53cc173..07caba7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -876,7 +876,6 @@ void pc_cpus_init(const char *cpu_model)
 
 for (i = 0; i  smp_cpus; i++) {
 if (!cpu_x86_init(cpu_model)) {
-fprintf(stderr, Unable to find x86 CPU definition\n);
 exit(1);
 }
 }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index dcbed0d..dadb0f0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1516,27 +1516,16 @@ static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
 CPUX86State *env = cpu-env;
 x86_def_t def1, *def = def1;
-Error *error = NULL;
-char *name, *features;
-gchar **model_pieces;
 
 memset(def, 0, sizeof(*def));
 
-model_pieces = g_strsplit(cpu_model, ,, 2);
-if (!model_pieces[0]) {
-error_setg(error, Invalid/empty CPU model name);
-goto out;
-}
-name = model_pieces[0];
-features = model_pieces[1];
-
 if (cpu_x86_find_by_name(def, name)  0) {
-error_setg(error, Unable to find CPU definition: %s, name);
-goto out;
+error_setg(errp, Unable to find CPU definition: %s, name);
+return;
 }
 
 if (kvm_enabled()) {
@@ -1544,58 +1533,69 @@ static int cpu_x86_register(X86CPU *cpu, const char 
*cpu_model)
 }
 def-ext_features |= CPUID_EXT_HYPERVISOR;
 
-object_property_set_str(OBJECT(cpu), def-vendor, vendor, error);
-object_property_set_int(OBJECT(cpu), def-level, level, error);
-object_property_set_int(OBJECT(cpu), def-family, family, error);
-object_property_set_int(OBJECT(cpu), def-model, model, error);
-object_property_set_int(OBJECT(cpu), def-stepping, stepping, error);
+object_property_set_str(OBJECT(cpu), def-vendor, vendor, errp);
+object_property_set_int(OBJECT(cpu), def-level, level, errp);
+object_property_set_int(OBJECT(cpu), def-family, family, errp);
+object_property_set_int(OBJECT(cpu), def-model, model, errp);
+object_property_set_int(OBJECT(cpu), def-stepping, stepping, errp);
 env-cpuid_features = def-features;
 env-cpuid_ext_features = def-ext_features;
 env-cpuid_ext2_features = def-ext2_features;
 env-cpuid_ext3_features = def-ext3_features;
-object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, error);
+object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, errp);
 env-cpuid_kvm_features = def-kvm_features;
 env-cpuid_svm_features = def-svm_features;
 env-cpuid_ext4_features = def-ext4_features;
 env-cpuid_7_0_ebx_features = def-cpuid_7_0_ebx_features;
 env-cpuid_xlevel2 = def-xlevel2;
 
-object_property_set_str(OBJECT(cpu), def-model_id, model-id, error);
-if (error) {
-goto out;
-}
-
-cpu_x86_parse_featurestr(cpu, features, error);
-out:
-g_strfreev(model_pieces);
-if (error) {
-fprintf(stderr, %s\n, error_get_pretty(error));
-error_free(error);
-return -1;
-}
-return 0;
+object_property_set_str(OBJECT(cpu), def-model_id, model-id, errp);
 }
 
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
-X86CPU *cpu;
+X86CPU *cpu = NULL;
 CPUX86State *env;
+gchar **model_pieces;
+char *name, *features;
 Error *error = NULL;
 
+model_pieces = g_strsplit(cpu_model, ,, 2);
+if (!model_pieces[0]) {
+error_setg(error, Invalid/empty CPU model name);
+goto out;
+}
+name = model_pieces[0];
+features = model_pieces[1];
+
 cpu = X86_CPU(object_new(TYPE_X86_CPU));
 env = cpu-env;
 env-cpu_model_str = cpu_model;
 
-if (cpu_x86_register(cpu, cpu_model)  0) {
-object_unref(OBJECT(cpu));
-return NULL;
+cpu_x86_register(cpu, name, error);
+if (error) {
+goto out;
+}
+
+cpu_x86_parse_featurestr(cpu, features, error);
+if (error) {
+goto out;
 }
 
 object_property_set_bool(OBJECT(cpu), true, realized, error);
 if (error) {
+goto out;
+}
+
+out:
+g_strfreev(model_pieces);

Re: [Qemu-devel] [PATCH qom-cpu-next v5] target-i386: Split command line parsing out of cpu_x86_register()

2013-02-15 Thread Eduardo Habkost 
On Fri, Feb 15, 2013 at 02:06:56PM +0100, Igor Mammedov wrote:
 From: Andreas Färber afaer...@suse.de
 
 In order to instantiate a CPU subtype we will need to know which type,
 so move the cpu_model splitting into cpu_x86_init().
 
 Parameters need to be set on the X86CPU instance, so move
 cpu_x86_parse_featurestr() into cpu_x86_init() as well.
 
 This leaves cpu_x86_register() operating on the model name only.
 
 Signed-off-by: Andreas Färber afaer...@suse.de
 Signed-off-by: Igor Mammedov imamm...@redhat.com

Reviewed-by: Eduardo Habkost ehabk...@redhat.com


 ---
  v5:
   * get error to report from cpu_x86_register()
  v4:
   * consolidate resource cleanup in when leaving cpu_x86_init(),
 to avoid clean code duplication.
   * remove unnecessary error message from hw/pc.c
 ---
  hw/pc.c   |1 -
  target-i386/cpu.c |   80 ++--
  2 files changed, 40 insertions(+), 41 deletions(-)
 
 diff --git a/hw/pc.c b/hw/pc.c
 index 53cc173..07caba7 100644
 --- a/hw/pc.c
 +++ b/hw/pc.c
 @@ -876,7 +876,6 @@ void pc_cpus_init(const char *cpu_model)
  
  for (i = 0; i  smp_cpus; i++) {
  if (!cpu_x86_init(cpu_model)) {
 -fprintf(stderr, Unable to find x86 CPU definition\n);
  exit(1);
  }
  }
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index dcbed0d..dadb0f0 100644
 --- a/target-i386/cpu.c
 +++ b/target-i386/cpu.c
 @@ -1516,27 +1516,16 @@ static void filter_features_for_kvm(X86CPU *cpu)
  }
  #endif
  
 -static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 +static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
  {
  CPUX86State *env = cpu-env;
  x86_def_t def1, *def = def1;
 -Error *error = NULL;
 -char *name, *features;
 -gchar **model_pieces;
  
  memset(def, 0, sizeof(*def));
  
 -model_pieces = g_strsplit(cpu_model, ,, 2);
 -if (!model_pieces[0]) {
 -error_setg(error, Invalid/empty CPU model name);
 -goto out;
 -}
 -name = model_pieces[0];
 -features = model_pieces[1];
 -
  if (cpu_x86_find_by_name(def, name)  0) {
 -error_setg(error, Unable to find CPU definition: %s, name);
 -goto out;
 +error_setg(errp, Unable to find CPU definition: %s, name);
 +return;
  }
  
  if (kvm_enabled()) {
 @@ -1544,58 +1533,69 @@ static int cpu_x86_register(X86CPU *cpu, const char 
 *cpu_model)
  }
  def-ext_features |= CPUID_EXT_HYPERVISOR;
  
 -object_property_set_str(OBJECT(cpu), def-vendor, vendor, error);
 -object_property_set_int(OBJECT(cpu), def-level, level, error);
 -object_property_set_int(OBJECT(cpu), def-family, family, error);
 -object_property_set_int(OBJECT(cpu), def-model, model, error);
 -object_property_set_int(OBJECT(cpu), def-stepping, stepping, error);
 +object_property_set_str(OBJECT(cpu), def-vendor, vendor, errp);
 +object_property_set_int(OBJECT(cpu), def-level, level, errp);
 +object_property_set_int(OBJECT(cpu), def-family, family, errp);
 +object_property_set_int(OBJECT(cpu), def-model, model, errp);
 +object_property_set_int(OBJECT(cpu), def-stepping, stepping, errp);
  env-cpuid_features = def-features;
  env-cpuid_ext_features = def-ext_features;
  env-cpuid_ext2_features = def-ext2_features;
  env-cpuid_ext3_features = def-ext3_features;
 -object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, error);
 +object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, errp);
  env-cpuid_kvm_features = def-kvm_features;
  env-cpuid_svm_features = def-svm_features;
  env-cpuid_ext4_features = def-ext4_features;
  env-cpuid_7_0_ebx_features = def-cpuid_7_0_ebx_features;
  env-cpuid_xlevel2 = def-xlevel2;
  
 -object_property_set_str(OBJECT(cpu), def-model_id, model-id, error);
 -if (error) {
 -goto out;
 -}
 -
 -cpu_x86_parse_featurestr(cpu, features, error);
 -out:
 -g_strfreev(model_pieces);
 -if (error) {
 -fprintf(stderr, %s\n, error_get_pretty(error));
 -error_free(error);
 -return -1;
 -}
 -return 0;
 +object_property_set_str(OBJECT(cpu), def-model_id, model-id, errp);
  }
  
  X86CPU *cpu_x86_init(const char *cpu_model)
  {
 -X86CPU *cpu;
 +X86CPU *cpu = NULL;
  CPUX86State *env;
 +gchar **model_pieces;
 +char *name, *features;
  Error *error = NULL;
  
 +model_pieces = g_strsplit(cpu_model, ,, 2);
 +if (!model_pieces[0]) {
 +error_setg(error, Invalid/empty CPU model name);
 +goto out;
 +}
 +name = model_pieces[0];
 +features = model_pieces[1];
 +
  cpu = X86_CPU(object_new(TYPE_X86_CPU));
  env = cpu-env;
  env-cpu_model_str = cpu_model;
  
 -if (cpu_x86_register(cpu, cpu_model)  0) {
 -object_unref(OBJECT(cpu));
 -return NULL;
 +cpu_x86_register(cpu, name, error);
 +if (error) {
 +goto out;