Re: [Qemu-devel] [PATCH 2/9] cpu/topology: add general support for machine properties

[Like Xu <like.xu@linux.intel.com>]

On 2019/4/4 22:25, Igor Mammedov wrote:
> On Fri, 29 Mar 2019 16:48:38 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 

<snipp>
> 
>> diff --git a/cpus.c b/cpus.c
>> index e83f72b..834a697 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -2067,6 +2067,10 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
>>   
>>   void qemu_init_vcpu(CPUState *cpu)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    unsigned int smp_cores = ms->topo.smp_cores;
>> +    unsigned int smp_threads = ms->topo.smp_threads;
> 
> (***)
> for once it probably will crash *-user builds
> and secondly the purpose of getting rid of smp_foo globals
> is disentangle layer violations and not replace it with another global
> (qdev_get_machine()).

I am happy to follow this rule on cpu-topo refactoring work, but 
sometimes calling qdev_get_machine() is inevitable.

> 
> What should be done is to make a properties of nr_cores/nr_threads and set
> them from the parent object that creates CPUs. The point is CPUs shouldn't
> reach out outside itself to fish out data bits it needs, it's responsibility
> of creator to feed to being create CPU needed properties.
> 
> This kind of refactoring probably deserves its own series and should precede
> -smp refactoring as it doesn't depend on CpuTopology at all.
> 

The division of responsibility for this case (refactoring 
qemu_init_vcpu) seems to be a poisonous apple.

The prerequisite for setting cpu-> nr_cores / nr_threads from the parent 
is that the CPU has been created, so if any process during 
initialization needs this topo information, it will use the default 
values form cpu_common_initfn() instead of user-configured parameters.

We may not want to repeat those assignment operations using the new 
values and what do you think, Igor?

<snipp>