Patchwork [qom-cpu,3/4] Really finally kill cpudef config section support

login
register
mail settings
Submitter Andreas Färber
Date Dec. 9, 2012, 7:45 p.m.
Message ID <1355082353-322-4-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/204777/
State New
Headers show

Comments

Andreas Färber - Dec. 9, 2012, 7:45 p.m.
Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
config section support) removed the cpudef parsing support but left
cpudef_* hooks behind. Remove those.

Since TYPE_X86_CPU currently is the only CPU class and CPUs are
instantiated exclusively through QOM (i.e., cpu_x86_init()), we can use
its class_init to bootstrap the list of model definitions until we have
these as subclasses. Avoid code movements for x86_cpudef_setup() to not
interfere with the various approaches to drop it in favor of subclasses.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 arch_init.c       |    7 -------
 arch_init.h       |    1 -
 bsd-user/main.c   |    3 ---
 linux-user/main.c |    3 ---
 target-i386/cpu.c |    7 ++++++-
 target-i386/cpu.h |    2 --
 vl.c              |    7 -------
 7 Dateien geändert, 6 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
Eduardo Habkost - Dec. 10, 2012, 6:09 p.m.
On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
> config section support) removed the cpudef parsing support but left
> cpudef_* hooks behind. Remove those.

The cpudef_* functions have nothing to do with the cpudef config section
since QEMU 1.2, it is just about initializing CPU-definition-related
data structures, so the patch subject is a bit misleading.

But I agree with the changes done here, and patch looks good to me, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> 
> Since TYPE_X86_CPU currently is the only CPU class and CPUs are
> instantiated exclusively through QOM (i.e., cpu_x86_init()), we can use
> its class_init to bootstrap the list of model definitions until we have
> these as subclasses. Avoid code movements for x86_cpudef_setup() to not
> interfere with the various approaches to drop it in favor of subclasses.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
>  arch_init.c       |    7 -------
>  arch_init.h       |    1 -
>  bsd-user/main.c   |    3 ---
>  linux-user/main.c |    3 ---
>  target-i386/cpu.c |    7 ++++++-
>  target-i386/cpu.h |    2 --
>  vl.c              |    7 -------
>  7 Dateien geändert, 6 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
> 
[...]
Andreas Färber - Dec. 10, 2012, 11:12 p.m.
Am 10.12.2012 19:09, schrieb Eduardo Habkost:
> On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
>> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
>> config section support) removed the cpudef parsing support but left
>> cpudef_* hooks behind. Remove those.
> 
> The cpudef_* functions have nothing to do with the cpudef config section
> since QEMU 1.2, it is just about initializing CPU-definition-related
> data structures, so the patch subject is a bit misleading.

My memory tells me they were specifically added for the config file
support... git-blame proves me wrong and shows they were added by Johan
Cooper and refactored by Blue, explaining his sudden cpudef patch
involvement. Sorry.

Do you have a proposal for a better text? My reasoning is we should
clean up before we forget about it and things stay behind.

Andreas

> 
> But I agree with the changes done here, and patch looks good to me, so:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
>>
>> Since TYPE_X86_CPU currently is the only CPU class and CPUs are
>> instantiated exclusively through QOM (i.e., cpu_x86_init()), we can use
>> its class_init to bootstrap the list of model definitions until we have
>> these as subclasses. Avoid code movements for x86_cpudef_setup() to not
>> interfere with the various approaches to drop it in favor of subclasses.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  arch_init.c       |    7 -------
>>  arch_init.h       |    1 -
>>  bsd-user/main.c   |    3 ---
>>  linux-user/main.c |    3 ---
>>  target-i386/cpu.c |    7 ++++++-
>>  target-i386/cpu.h |    2 --
>>  vl.c              |    7 -------
>>  7 Dateien geändert, 6 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
>>
> [...]
>
Eduardo Habkost - Dec. 10, 2012, 11:53 p.m.
On Tue, Dec 11, 2012 at 12:12:23AM +0100, Andreas Färber wrote:
> Am 10.12.2012 19:09, schrieb Eduardo Habkost:
> > On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
> >> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
> >> config section support) removed the cpudef parsing support but left
> >> cpudef_* hooks behind. Remove those.
> > 
> > The cpudef_* functions have nothing to do with the cpudef config section
> > since QEMU 1.2, it is just about initializing CPU-definition-related
> > data structures, so the patch subject is a bit misleading.
> 
> My memory tells me they were specifically added for the config file
> support... git-blame proves me wrong and shows they were added by Johan
> Cooper and refactored by Blue, explaining his sudden cpudef patch
> involvement. Sorry.
> 
> Do you have a proposal for a better text? My reasoning is we should
> clean up before we forget about it and things stay behind.

Something like "kill the cpudef_setup() hook that we don't need anymore
[because only x86 uses it, and we can simply use class_init to
initialize data structures]" sounds good enough to me.
Wayne Xia - Dec. 11, 2012, 8:41 a.m.
于 2012-12-11 7:53, Eduardo Habkost 写道:
> On Tue, Dec 11, 2012 at 12:12:23AM +0100, Andreas Färber wrote:
>> Am 10.12.2012 19:09, schrieb Eduardo Habkost:
>>> On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
>>>> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
>>>> config section support) removed the cpudef parsing support but left
>>>> cpudef_* hooks behind. Remove those.
>>>
>>> The cpudef_* functions have nothing to do with the cpudef config section
>>> since QEMU 1.2, it is just about initializing CPU-definition-related
>>> data structures, so the patch subject is a bit misleading.
>>
>> My memory tells me they were specifically added for the config file
>> support... git-blame proves me wrong and shows they were added by Johan
>> Cooper and refactored by Blue, explaining his sudden cpudef patch
>> involvement. Sorry.
>>
>> Do you have a proposal for a better text? My reasoning is we should
>> clean up before we forget about it and things stay behind.
>
> Something like "kill the cpudef_setup() hook that we don't need anymore
> [because only x86 uses it, and we can simply use class_init to
> initialize data structures]" sounds good enough to me.
>
Hi, Eduardo
   I think we need uninstall rules which should delete the def files,
otherwise we met error:
qemu-system-x86_64:/usr/local/etc/qemu/target-x86_64.conf:3: There is no 
option group 'cpudef'

Patch

diff --git a/arch_init.c b/arch_init.c
index e6effe8..bea1b9c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1124,13 +1124,6 @@  void do_smbios_option(const char *optarg)
 #endif
 }
 
-void cpudef_init(void)
-{
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file */
-#endif
-}
-
 int audio_available(void)
 {
 #ifdef HAS_AUDIO
diff --git a/arch_init.h b/arch_init.h
index 5fc780c..84a7f9a 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -27,7 +27,6 @@  extern const uint32_t arch_type;
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const char *optarg);
 void do_smbios_option(const char *optarg);
-void cpudef_init(void);
 int audio_available(void);
 void audio_init(ISABus *isa_bus, PCIBus *pci_bus);
 int tcg_available(void);
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 095ae8e..cf0a345 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -762,9 +762,6 @@  int main(int argc, char **argv)
     }
 
     cpu_model = NULL;
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
-#endif
 
     optind = 1;
     for(;;) {
diff --git a/linux-user/main.c b/linux-user/main.c
index 25e35cd..e881fcf 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3426,9 +3426,6 @@  int main(int argc, char **argv, char **envp)
     }
 
     cpu_model = NULL;
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
-#endif
 
     /* init debug */
     cpu_set_log_filename(log_file);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e265317..13de2e3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1414,6 +1414,9 @@  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     int i;
     char buf[256];
 
+    /* Force creation of CPU class */
+    object_class_by_name(TYPE_X86_CPU);
+
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
         x86_def_t *def = &builtin_x86_defs[i];
         snprintf(buf, sizeof(buf), "%s", def->name);
@@ -1585,7 +1588,7 @@  void cpu_clear_apic_feature(CPUX86State *env)
 
 /* Initialize list of CPU models, filling some non-static fields if necessary
  */
-void x86_cpudef_setup(void)
+static void x86_cpudef_setup(void)
 {
     int i, j;
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
@@ -2138,6 +2141,8 @@  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
+
+    x86_cpudef_setup();
 }
 
 static const TypeInfo x86_cpu_type_info = {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 386c4f6..bd6aec4 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -865,7 +865,6 @@  typedef struct CPUX86State {
 X86CPU *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
-void x86_cpudef_setup(void);
 int cpu_x86_support_mca_broadcast(CPUX86State *env);
 
 int cpu_get_pic_interrupt(CPUX86State *s);
@@ -1047,7 +1046,6 @@  static inline CPUX86State *cpu_init(const char *cpu_model)
 #define cpu_gen_code cpu_x86_gen_code
 #define cpu_signal_handler cpu_x86_signal_handler
 #define cpu_list x86_cpu_list
-#define cpudef_setup	x86_cpudef_setup
 
 #define CPU_SAVE_VERSION 12
 
diff --git a/vl.c b/vl.c
index a3ab384..9d867c2 100644
--- a/vl.c
+++ b/vl.c
@@ -3560,13 +3560,6 @@  int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    /* Init CPU def lists, based on config
-     * - Must be called after all the qemu_read_config_file() calls
-     * - Must be called before list_cpus()
-     * - Must be called before machine->init()
-     */
-    cpudef_init();
-
     if (cpu_model && is_help_option(cpu_model)) {
         list_cpus(stdout, &fprintf, cpu_model);
         exit(0);