diff mbox

[PULL,05/11] target-i386: Eliminate cpu_init() function

Message ID 1424894306-26740-6-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost Feb. 25, 2015, 7:58 p.m. UTC 
Instead of putting extra logic inside cpu.h, just do everything inside
cpu_x86_init_user().

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |  6 +++---
 target-i386/cpu.h | 12 +++---------
 2 files changed, 6 insertions(+), 12 deletions(-)

Comments

Andreas Färber Feb. 25, 2015, 10:08 p.m. UTC | #1 
Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> Instead of putting extra logic inside cpu.h, just do everything inside
> cpu_x86_init_user().
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |  6 +++---
>  target-i386/cpu.h | 12 +++---------
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8f18556..aee4d3e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2135,7 +2135,7 @@ out:
>      return cpu;
>  }
>  
> -X86CPU *cpu_x86_init_user(const char *cpu_model)
> +CPUX86State *cpu_x86_init_user(const char *cpu_model)
>  {
>      Error *error = NULL;
>      X86CPU *cpu;
> @@ -2153,10 +2153,10 @@ out:
>          error_free(error);
>          if (cpu != NULL) {
>              object_unref(OBJECT(cpu));
> -            cpu = NULL;
>          }
> +        return NULL;
>      }
> -    return cpu;
> +    return &cpu->env;
>  }
>  
>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 41d7f0a..d5bd74e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -982,7 +982,6 @@ typedef struct CPUX86State {
>  
>  #include "cpu-qom.h"
>  
> -X86CPU *cpu_x86_init_user(const char *cpu_model);
>  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>                         Error **errp);
>  int cpu_x86_exec(CPUX86State *s);
> @@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  # define PHYS_ADDR_MASK 0xfffffffffLL
>  # endif
>  
> -static inline CPUX86State *cpu_init(const char *cpu_model)
> -{
> -    X86CPU *cpu = cpu_x86_init_user(cpu_model);
> -    if (cpu == NULL) {
> -        return NULL;
> -    }
> -    return &cpu->env;
> -}
> +/* CPU creation function for *-user */
> +CPUX86State *cpu_x86_init_user(const char *cpu_model);
> +#define cpu_init cpu_x86_init_user

The very purpose of this lightweight glue code in cpu.h was to let us
use X86CPU in cpu.c. It seems that you are needlessly reverting my change?

Andreas

>  
>  #define cpu_exec cpu_x86_exec
>  #define cpu_gen_code cpu_x86_gen_code
>
Eduardo Habkost Feb. 26, 2015, 4:03 p.m. UTC | #2 
On Wed, Feb 25, 2015 at 11:08:56PM +0100, Andreas Färber wrote:
> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> > Instead of putting extra logic inside cpu.h, just do everything inside
> > cpu_x86_init_user().
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c |  6 +++---
> >  target-i386/cpu.h | 12 +++---------
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 8f18556..aee4d3e 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2135,7 +2135,7 @@ out:
> >      return cpu;
> >  }
> >  
> > -X86CPU *cpu_x86_init_user(const char *cpu_model)
> > +CPUX86State *cpu_x86_init_user(const char *cpu_model)
> >  {
> >      Error *error = NULL;
> >      X86CPU *cpu;
> > @@ -2153,10 +2153,10 @@ out:
> >          error_free(error);
> >          if (cpu != NULL) {
> >              object_unref(OBJECT(cpu));
> > -            cpu = NULL;
> >          }
> > +        return NULL;
> >      }
> > -    return cpu;
> > +    return &cpu->env;
> >  }
> >  
> >  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 41d7f0a..d5bd74e 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -982,7 +982,6 @@ typedef struct CPUX86State {
> >  
> >  #include "cpu-qom.h"
> >  
> > -X86CPU *cpu_x86_init_user(const char *cpu_model);
> >  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
> >                         Error **errp);
> >  int cpu_x86_exec(CPUX86State *s);
> > @@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
> >  # define PHYS_ADDR_MASK 0xfffffffffLL
> >  # endif
> >  
> > -static inline CPUX86State *cpu_init(const char *cpu_model)
> > -{
> > -    X86CPU *cpu = cpu_x86_init_user(cpu_model);
> > -    if (cpu == NULL) {
> > -        return NULL;
> > -    }
> > -    return &cpu->env;
> > -}
> > +/* CPU creation function for *-user */
> > +CPUX86State *cpu_x86_init_user(const char *cpu_model);
> > +#define cpu_init cpu_x86_init_user
> 
> The very purpose of this lightweight glue code in cpu.h was to let us
> use X86CPU in cpu.c. It seems that you are needlessly reverting my change?

I am not sure I understand what you mean by "let us use X86CPU in
cpu.c". But I think the series can be redone without this change,
anyway.

In either case, what's your plan to eliminate this glue code from the
cpu.h files?
Andreas Färber Feb. 27, 2015, 2:19 p.m. UTC | #3 
Am 26.02.2015 um 17:03 schrieb Eduardo Habkost:
> On Wed, Feb 25, 2015 at 11:08:56PM +0100, Andreas Färber wrote:
>> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
>>> Instead of putting extra logic inside cpu.h, just do everything inside
>>> cpu_x86_init_user().
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  target-i386/cpu.c |  6 +++---
>>>  target-i386/cpu.h | 12 +++---------
>>>  2 files changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index 8f18556..aee4d3e 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -2135,7 +2135,7 @@ out:
>>>      return cpu;
>>>  }
>>>  
>>> -X86CPU *cpu_x86_init_user(const char *cpu_model)
>>> +CPUX86State *cpu_x86_init_user(const char *cpu_model)
>>>  {
>>>      Error *error = NULL;
>>>      X86CPU *cpu;
>>> @@ -2153,10 +2153,10 @@ out:
>>>          error_free(error);
>>>          if (cpu != NULL) {
>>>              object_unref(OBJECT(cpu));
>>> -            cpu = NULL;
>>>          }
>>> +        return NULL;
>>>      }
>>> -    return cpu;
>>> +    return &cpu->env;
>>>  }
>>>  
>>>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index 41d7f0a..d5bd74e 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -982,7 +982,6 @@ typedef struct CPUX86State {
>>>  
>>>  #include "cpu-qom.h"
>>>  
>>> -X86CPU *cpu_x86_init_user(const char *cpu_model);
>>>  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>>>                         Error **errp);
>>>  int cpu_x86_exec(CPUX86State *s);
>>> @@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>>>  # define PHYS_ADDR_MASK 0xfffffffffLL
>>>  # endif
>>>  
>>> -static inline CPUX86State *cpu_init(const char *cpu_model)
>>> -{
>>> -    X86CPU *cpu = cpu_x86_init_user(cpu_model);
>>> -    if (cpu == NULL) {
>>> -        return NULL;
>>> -    }
>>> -    return &cpu->env;
>>> -}
>>> +/* CPU creation function for *-user */
>>> +CPUX86State *cpu_x86_init_user(const char *cpu_model);
>>> +#define cpu_init cpu_x86_init_user
>>
>> The very purpose of this lightweight glue code in cpu.h was to let us
>> use X86CPU in cpu.c. It seems that you are needlessly reverting my change?
> 
> I am not sure I understand what you mean by "let us use X86CPU in
> cpu.c".

I mean, more and more code is supposed to use *CPU, not CPU*State. Like
your patch moving the APIC ID is adopting X86CPU, which is the right
direction. Replacing X86CPU with CPUX86State however feels wrong to me.

Since I changed cpu_x86_init() to return X86CPU, there will have been a
non-user caller at the time making use of X86CPU or CPUState. Just as in
some ARM use cases I assume that the current cpu_*_init() limitations
lead to its code getting inlined.

Andreas
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8f18556..aee4d3e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2135,7 +2135,7 @@  out:
     return cpu;
 }
 
-X86CPU *cpu_x86_init_user(const char *cpu_model)
+CPUX86State *cpu_x86_init_user(const char *cpu_model)
 {
     Error *error = NULL;
     X86CPU *cpu;
@@ -2153,10 +2153,10 @@  out:
         error_free(error);
         if (cpu != NULL) {
             object_unref(OBJECT(cpu));
-            cpu = NULL;
         }
+        return NULL;
     }
-    return cpu;
+    return &cpu->env;
 }
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 41d7f0a..d5bd74e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -982,7 +982,6 @@  typedef struct CPUX86State {
 
 #include "cpu-qom.h"
 
-X86CPU *cpu_x86_init_user(const char *cpu_model);
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
                        Error **errp);
 int cpu_x86_exec(CPUX86State *s);
@@ -1171,14 +1170,9 @@  uint64_t cpu_get_tsc(CPUX86State *env);
 # define PHYS_ADDR_MASK 0xfffffffffLL
 # endif
 
-static inline CPUX86State *cpu_init(const char *cpu_model)
-{
-    X86CPU *cpu = cpu_x86_init_user(cpu_model);
-    if (cpu == NULL) {
-        return NULL;
-    }
-    return &cpu->env;
-}
+/* CPU creation function for *-user */
+CPUX86State *cpu_x86_init_user(const char *cpu_model);
+#define cpu_init cpu_x86_init_user
 
 #define cpu_exec cpu_x86_exec
 #define cpu_gen_code cpu_x86_gen_code