| Message ID | 1355082353-322-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote: > Simplifies the upcoming cleanup of cpu_x86_find_by_name(). ...by making cpu_x86_register() more complicated, and having CPU model name lookup spread into different parts of the code. The CPU model lookup is a bit complex because of the "host" exception, but at least the complexity was hidden inside cpu_x86_find_by_name() (making it very easy to replace that logic by a CPU subclass lookup, later). > > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > target-i386/cpu.c | 12 +++++++----- > 1 Datei geändert, 7 Zeilen hinzugefügt(+), 5 Zeilen entfernt(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 7be3ad8..a46faa2 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1217,9 +1217,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > break; > } > } > - if (kvm_enabled() && name && strcmp(name, "host") == 0) { > - kvm_cpu_fill_host(x86_cpu_def); > - } else if (!def) { > + if (!def) { > return -1; > } else { > memcpy(x86_cpu_def, def, sizeof(*def)); > @@ -1505,8 +1503,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > name = model_pieces[0]; > features = model_pieces[1]; > > - if (cpu_x86_find_by_name(def, name) < 0) { > - goto error; > + if (kvm_enabled() && strcmp(name, "host") == 0) { > + kvm_cpu_fill_host(def); > + } else { > + if (cpu_x86_find_by_name(def, name) < 0) { > + goto error; > + } > } > > if (cpu_x86_parse_featurestr(def, features) < 0) { > -- > 1.7.10.4 >
On Mon, 10 Dec 2012 10:46:13 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote: > > Simplifies the upcoming cleanup of cpu_x86_find_by_name(). > > ...by making cpu_x86_register() more complicated, and having CPU model > name lookup spread into different parts of the code. > > The CPU model lookup is a bit complex because of the "host" exception, > but at least the complexity was hidden inside cpu_x86_find_by_name() > (making it very easy to replace that logic by a CPU subclass lookup, > later). +1 [...] > > -- > Eduardo >
Am 10.12.2012 19:55, schrieb Igor Mammedov: > On Mon, 10 Dec 2012 10:46:13 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote: >>> Simplifies the upcoming cleanup of cpu_x86_find_by_name(). >> >> ...by making cpu_x86_register() more complicated, and having CPU model >> name lookup spread into different parts of the code. >> >> The CPU model lookup is a bit complex because of the "host" exception, >> but at least the complexity was hidden inside cpu_x86_find_by_name() >> (making it very easy to replace that logic by a CPU subclass lookup, >> later). (Somehow I didn't get this message ... yet) > +1 Could you guys re-review this in light of the subclasses patch? The issue I was facing is that I did not see a reliable way to register the host class depending on kvm_enabled(). Therefore ..._find_by_name() becomes the class lookup whereas the host check remains a special check in cpu_x86_init() [a faulty one I now see, ignoring "host-x86_64-cpu"]. cpu_x86_register() is problematic in that it gets the X86CPU served on a silver plate, so it's too late to choose subclasses in that function. Andreas > > [...] >> >> -- >> Eduardo >> > >
On Tue, Dec 11, 2012 at 12:21:10AM +0100, Andreas Färber wrote: > Am 10.12.2012 19:55, schrieb Igor Mammedov: > > On Mon, 10 Dec 2012 10:46:13 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > >> On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote: > >>> Simplifies the upcoming cleanup of cpu_x86_find_by_name(). > >> > >> ...by making cpu_x86_register() more complicated, and having CPU model > >> name lookup spread into different parts of the code. > >> > >> The CPU model lookup is a bit complex because of the "host" exception, > >> but at least the complexity was hidden inside cpu_x86_find_by_name() > >> (making it very easy to replace that logic by a CPU subclass lookup, > >> later). > > (Somehow I didn't get this message ... yet) > > > +1 > > Could you guys re-review this in light of the subclasses patch? The > issue I was facing is that I did not see a reliable way to register the > host class depending on kvm_enabled(). Therefore ..._find_by_name() > becomes the class lookup whereas the host check remains a special check > in cpu_x86_init() [a faulty one I now see, ignoring "host-x86_64-cpu"]. > cpu_x86_register() is problematic in that it gets the X86CPU served on a > silver plate, so it's too late to choose subclasses in that function. I have a patch[1] to change that in my APIC-ID-topology work in progress branch[2]. I can submit this (and maybe a few other code movements) as PATCH tomorrow. I have a branch for CPU subclasses using an approach similar to the one you used, but that includes "host" as well[3]. But I was waiting until the properties series is resubmitted, so I could use it as base and make the subclass code simpler. I will take a deeper look at your RFC tomorrow, and probably send additional comments. [1] https://github.com/ehabkost/qemu-hacks/commit/850c5531c84839919d650b2dac6e98fa4c2529c4 [2] https://github.com/ehabkost/qemu-hacks/commits/work/apicid [3] https://github.com/ehabkost/qemu-hacks/commits/work/cpu-model-classes
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 7be3ad8..a46faa2 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1217,9 +1217,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) break; } } - if (kvm_enabled() && name && strcmp(name, "host") == 0) { - kvm_cpu_fill_host(x86_cpu_def); - } else if (!def) { + if (!def) { return -1; } else { memcpy(x86_cpu_def, def, sizeof(*def)); @@ -1505,8 +1503,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) name = model_pieces[0]; features = model_pieces[1]; - if (cpu_x86_find_by_name(def, name) < 0) { - goto error; + if (kvm_enabled() && strcmp(name, "host") == 0) { + kvm_cpu_fill_host(def); + } else { + if (cpu_x86_find_by_name(def, name) < 0) { + goto error; + } } if (cpu_x86_parse_featurestr(def, features) < 0) {
Simplifies the upcoming cleanup of cpu_x86_find_by_name(). Signed-off-by: Andreas Färber <afaerber@suse.de> --- target-i386/cpu.c | 12 +++++++----- 1 Datei geändert, 7 Zeilen hinzugefügt(+), 5 Zeilen entfernt(-)