Re: [Qemu-devel] [PATCH 06/22] cpu: introduce CPUClass.resume() method

[Andreas Färber]

Am 09.04.2013 12:26, schrieb Igor Mammedov:
> On Mon, 8 Apr 2013 17:13:11 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
>> On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote:
>>> ... and call it if defined from CPUClass.realize() if CPU was hotplugged
>>>
>>> by default leave .resume() unset (i.e. NULL) and override it for softmmu
>>> in qemu_init_vcpu() if it's still unset.
>>>
>>> Signed-off-by: Igor Mammedov <address@hidden>
>> [...]
>>> diff --git a/cpus.c b/cpus.c
>>> index 9b9a32f..6b793c5 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
>> [...]
>>> @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env)
>>>  {
>>>      CPUArchState *env = _env;
>>>      CPUState *cpu = ENV_GET_CPU(env);
>>> +    CPUClass *klass = CPU_GET_CLASS(cpu);
>>>  
>>> +    if (klass->resume == NULL) {
>>> +        klass->resume = resume_vcpu;
>>> +    }
>>
>> So you are initializing a field of CPUClass struct inside a CPU object
>> initialization function. And that's a function that is not even
>> converted to QOM yet, and buried inside a non-trivial function call tree
>> (hence easy to be called at the wrong time if one day we reorder the
>> initialization steps).
> init step are not likely to change this drastically but main reason why
> it's here see below.
> 
>>
>> Can't we do this on class_init(), where it belongs? If we need different
>> implementations for softmmu/user, we can add a stub for *-user. I think
>> even an explicit #ifdef inside resume_vcpu() would be preferable to
>> this.
> Generally I agree with you that class_init() would be more correct, but
> ifdefs in qom/cpu.c defeat purpose to build qom/cpu.c only once for all
> targets, that Andreas are working towards.

Indeed. Anthony had suggested to introduce a cpu-softmmu.c or something
like that it we ever need to distinguish. Not sure how they would
interface though. ;)

Andreas

> 
> It asks surely asks for resume_vcpu() stub in libqemustubs, and I'd do it
> there weren't objections to it.
> 
> Paolo,
>  it looks like a good candidate for libqemustubs, would it be ok to add stub
>  there?
> 
>>
>>
>>>      cpu->nr_cores = smp_cores;
>>>      cpu->nr_threads = smp_threads;
>>>      cpu->stopped = true;
>> [...]
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg