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



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;