Re: [Qemu-devel] [PULL 05/11] target-i386: Eliminate cpu_init() function

[Andreas Färber]

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 <address@hidden>
>>> Signed-off-by: Eduardo Habkost <address@hidden>
>>> ---
>>>  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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)