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

[Like Xu]

On 2019/4/4 22:25, Igor Mammedov wrote:
> On Fri, 29 Mar 2019 16:48:38 +0800
> Like Xu <address@hidden> 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>