Patchwork [qom-cpu,1/4] target-i386: Inline -cpu host check into cpu_x86_register()

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

Comments

Andreas Färber - Dec. 9, 2012, 7:45 p.m.
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(-)
Eduardo Habkost - Dec. 10, 2012, 12:46 p.m.
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
>
Igor Mammedov - Dec. 10, 2012, 6:55 p.m.
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
>
Andreas Färber - Dec. 10, 2012, 11:21 p.m.
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
>>
> 
>
Eduardo Habkost - Dec. 10, 2012, 11:33 p.m.
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

Patch

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) {