Re: [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties

Re: [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties

[Like Xu]

On 2019/3/29 18:21, Igor Mammedov wrote:
> On Fri, 29 Mar 2019 16:48:36 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
>> This patch series make existing cores/threads/sockets into machine
>> properties and get rid of global variables they use currently.
> Thanks for looking into it!
> Its long overdue and rather desired conversion (albeit naive one,
> but this series is a good starting point). I'll go over your patches
> next week with comments and concrete suggestions how to implement
> particular things.

Hi Igor, any comments and suggestions on smp machine properties
in this patch considering we may add die topology for PCMachine as an 
extension?

> 
>> Like Xu (9):
>>    cpu/topology: add struct CpuTopology to MachineState
>>    cpu/topology: add general support for machine properties
>>    cpu/topology: add uncommon arch support for smp machine properties
>>    cpu/topology: add ARM support for smp machine properties
>>    cpu/topology: add i386 support for smp machine properties
>>    cpu/topology: add PPC support for smp machine properties
>>    cpu/topology: add riscv support for smp machine properties
>>    cpu/topology: add s390x support for smp machine properties
>>    cpu/topology: replace smp global variables with machine propertie
>>
>>   accel/kvm/kvm-all.c          |  3 +++
>>   backends/hostmem.c           |  4 ++++
>>   cpus.c                       |  4 ++++
>>   exec.c                       |  2 ++
>>   gdbstub.c                    |  7 ++++++-
>>   hw/alpha/dp264.c             |  1 +
>>   hw/arm/fsl-imx6.c            |  5 +++++
>>   hw/arm/fsl-imx6ul.c          |  5 +++++
>>   hw/arm/fsl-imx7.c            |  5 +++++
>>   hw/arm/highbank.c            |  1 +
>>   hw/arm/mcimx6ul-evk.c        |  1 +
>>   hw/arm/mcimx7d-sabre.c       |  3 +++
>>   hw/arm/raspi.c               |  2 ++
>>   hw/arm/realview.c            |  1 +
>>   hw/arm/sabrelite.c           |  1 +
>>   hw/arm/vexpress.c            |  3 +++
>>   hw/arm/virt.c                |  7 +++++++
>>   hw/arm/xlnx-zynqmp.c         |  7 +++++++
>>   hw/cpu/core.c                |  3 +++
>>   hw/hppa/machine.c            |  4 ++++
>>   hw/i386/acpi-build.c         |  3 +++
>>   hw/i386/kvmvapic.c           |  5 +++++
>>   hw/i386/pc.c                 | 12 +++++++++++
>>   hw/mips/boston.c             |  1 +
>>   hw/mips/mips_malta.c         |  9 +++++++++
>>   hw/openrisc/openrisc_sim.c   |  1 +
>>   hw/ppc/e500.c                |  3 +++
>>   hw/ppc/mac_newworld.c        |  2 ++
>>   hw/ppc/mac_oldworld.c        |  2 ++
>>   hw/ppc/pnv.c                 |  3 +++
>>   hw/ppc/prep.c                |  2 ++
>>   hw/ppc/spapr.c               | 29 ++++++++++++++++++++++++++
>>   hw/ppc/spapr_rtas.c          |  3 +++
>>   hw/riscv/sifive_e.c          |  4 ++++
>>   hw/riscv/sifive_plic.c       |  3 +++
>>   hw/riscv/sifive_u.c          |  4 ++++
>>   hw/riscv/spike.c             |  2 ++
>>   hw/riscv/virt.c              |  1 +
>>   hw/s390x/s390-virtio-ccw.c   |  2 ++
>>   hw/s390x/sclp.c              |  1 +
>>   hw/smbios/smbios.c           | 11 ++++++++++
>>   hw/sparc/sun4m.c             |  2 ++
>>   hw/sparc64/sun4u.c           |  2 ++
>>   hw/xtensa/sim.c              |  1 +
>>   hw/xtensa/xtfpga.c           |  1 +
>>   include/hw/arm/virt.h        |  2 +-
>>   include/hw/boards.h          |  8 ++++++++
>>   include/sysemu/sysemu.h      |  2 +-
>>   migration/postcopy-ram.c     |  7 +++++++
>>   numa.c                       |  1 +
>>   target/arm/cpu.c             |  7 +++++++
>>   target/i386/cpu.c            |  4 ++++
>>   target/openrisc/sys_helper.c |  5 +++++
>>   target/s390x/cpu.c           |  3 +++
>>   target/s390x/excp_helper.c   |  6 ++++++
>>   tcg/tcg.c                    | 15 ++++++++++++++
>>   vl.c                         | 48 ++++++++++++++++++++++++--------------------
>>   57 files changed, 261 insertions(+), 25 deletions(-)
>>
> 
> 
> 

Re: [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties

[Igor Mammedov]

On Thu, 4 Apr 2019 11:26:09 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> On 2019/3/29 18:21, Igor Mammedov wrote:
> > On Fri, 29 Mar 2019 16:48:36 +0800
> > Like Xu <like.xu@linux.intel.com> wrote:
> >   
> >> This patch series make existing cores/threads/sockets into machine
> >> properties and get rid of global variables they use currently.  
> > Thanks for looking into it!
> > Its long overdue and rather desired conversion (albeit naive one,
> > but this series is a good starting point). I'll go over your patches
> > next week with comments and concrete suggestions how to implement
> > particular things.  
> 
> Hi Igor, any comments and suggestions on smp machine properties
> in this patch considering we may add die topology for PCMachine as an 
> extension?

I've looked at several patches and that it for this series.
The most comments apply to the patches I've not reviewed as well.

> >   
> >> Like Xu (9):
> >>    cpu/topology: add struct CpuTopology to MachineState
> >>    cpu/topology: add general support for machine properties
> >>    cpu/topology: add uncommon arch support for smp machine properties
> >>    cpu/topology: add ARM support for smp machine properties
> >>    cpu/topology: add i386 support for smp machine properties
> >>    cpu/topology: add PPC support for smp machine properties
> >>    cpu/topology: add riscv support for smp machine properties
> >>    cpu/topology: add s390x support for smp machine properties
> >>    cpu/topology: replace smp global variables with machine propertie
> >>
> >>   accel/kvm/kvm-all.c          |  3 +++
> >>   backends/hostmem.c           |  4 ++++
> >>   cpus.c                       |  4 ++++
> >>   exec.c                       |  2 ++
> >>   gdbstub.c                    |  7 ++++++-
> >>   hw/alpha/dp264.c             |  1 +
> >>   hw/arm/fsl-imx6.c            |  5 +++++
> >>   hw/arm/fsl-imx6ul.c          |  5 +++++
> >>   hw/arm/fsl-imx7.c            |  5 +++++
> >>   hw/arm/highbank.c            |  1 +
> >>   hw/arm/mcimx6ul-evk.c        |  1 +
> >>   hw/arm/mcimx7d-sabre.c       |  3 +++
> >>   hw/arm/raspi.c               |  2 ++
> >>   hw/arm/realview.c            |  1 +
> >>   hw/arm/sabrelite.c           |  1 +
> >>   hw/arm/vexpress.c            |  3 +++
> >>   hw/arm/virt.c                |  7 +++++++
> >>   hw/arm/xlnx-zynqmp.c         |  7 +++++++
> >>   hw/cpu/core.c                |  3 +++
> >>   hw/hppa/machine.c            |  4 ++++
> >>   hw/i386/acpi-build.c         |  3 +++
> >>   hw/i386/kvmvapic.c           |  5 +++++
> >>   hw/i386/pc.c                 | 12 +++++++++++
> >>   hw/mips/boston.c             |  1 +
> >>   hw/mips/mips_malta.c         |  9 +++++++++
> >>   hw/openrisc/openrisc_sim.c   |  1 +
> >>   hw/ppc/e500.c                |  3 +++
> >>   hw/ppc/mac_newworld.c        |  2 ++
> >>   hw/ppc/mac_oldworld.c        |  2 ++
> >>   hw/ppc/pnv.c                 |  3 +++
> >>   hw/ppc/prep.c                |  2 ++
> >>   hw/ppc/spapr.c               | 29 ++++++++++++++++++++++++++
> >>   hw/ppc/spapr_rtas.c          |  3 +++
> >>   hw/riscv/sifive_e.c          |  4 ++++
> >>   hw/riscv/sifive_plic.c       |  3 +++
> >>   hw/riscv/sifive_u.c          |  4 ++++
> >>   hw/riscv/spike.c             |  2 ++
> >>   hw/riscv/virt.c              |  1 +
> >>   hw/s390x/s390-virtio-ccw.c   |  2 ++
> >>   hw/s390x/sclp.c              |  1 +
> >>   hw/smbios/smbios.c           | 11 ++++++++++
> >>   hw/sparc/sun4m.c             |  2 ++
> >>   hw/sparc64/sun4u.c           |  2 ++
> >>   hw/xtensa/sim.c              |  1 +
> >>   hw/xtensa/xtfpga.c           |  1 +
> >>   include/hw/arm/virt.h        |  2 +-
> >>   include/hw/boards.h          |  8 ++++++++
> >>   include/sysemu/sysemu.h      |  2 +-
> >>   migration/postcopy-ram.c     |  7 +++++++
> >>   numa.c                       |  1 +
> >>   target/arm/cpu.c             |  7 +++++++
> >>   target/i386/cpu.c            |  4 ++++
> >>   target/openrisc/sys_helper.c |  5 +++++
> >>   target/s390x/cpu.c           |  3 +++
> >>   target/s390x/excp_helper.c   |  6 ++++++
> >>   tcg/tcg.c                    | 15 ++++++++++++++
> >>   vl.c                         | 48 ++++++++++++++++++++++++--------------------
> >>   57 files changed, 261 insertions(+), 25 deletions(-)
> >>  
> > 
> > 
> >   
> 
> 

Re: [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties

[Igor Mammedov]

On Thu, 4 Apr 2019 11:26:09 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> On 2019/3/29 18:21, Igor Mammedov wrote:
> > On Fri, 29 Mar 2019 16:48:36 +0800
> > Like Xu <like.xu@linux.intel.com> wrote:
> >   
> >> This patch series make existing cores/threads/sockets into machine
> >> properties and get rid of global variables they use currently.  
> > Thanks for looking into it!
> > Its long overdue and rather desired conversion (albeit naive one,
> > but this series is a good starting point). I'll go over your patches
> > next week with comments and concrete suggestions how to implement
> > particular things.  
> 
> Hi Igor, any comments and suggestions on smp machine properties
> in this patch considering we may add die topology for PCMachine as an 
> extension?

I've looked at several patches and that it for this series.
The most comments apply to the patches I've not reviewed as well.

> >   
> >> Like Xu (9):
> >>    cpu/topology: add struct CpuTopology to MachineState
> >>    cpu/topology: add general support for machine properties
> >>    cpu/topology: add uncommon arch support for smp machine properties
> >>    cpu/topology: add ARM support for smp machine properties
> >>    cpu/topology: add i386 support for smp machine properties
> >>    cpu/topology: add PPC support for smp machine properties
> >>    cpu/topology: add riscv support for smp machine properties
> >>    cpu/topology: add s390x support for smp machine properties
> >>    cpu/topology: replace smp global variables with machine propertie
> >>
> >>   accel/kvm/kvm-all.c          |  3 +++
> >>   backends/hostmem.c           |  4 ++++
> >>   cpus.c                       |  4 ++++
> >>   exec.c                       |  2 ++
> >>   gdbstub.c                    |  7 ++++++-
> >>   hw/alpha/dp264.c             |  1 +
> >>   hw/arm/fsl-imx6.c            |  5 +++++
> >>   hw/arm/fsl-imx6ul.c          |  5 +++++
> >>   hw/arm/fsl-imx7.c            |  5 +++++
> >>   hw/arm/highbank.c            |  1 +
> >>   hw/arm/mcimx6ul-evk.c        |  1 +
> >>   hw/arm/mcimx7d-sabre.c       |  3 +++
> >>   hw/arm/raspi.c               |  2 ++
> >>   hw/arm/realview.c            |  1 +
> >>   hw/arm/sabrelite.c           |  1 +
> >>   hw/arm/vexpress.c            |  3 +++
> >>   hw/arm/virt.c                |  7 +++++++
> >>   hw/arm/xlnx-zynqmp.c         |  7 +++++++
> >>   hw/cpu/core.c                |  3 +++
> >>   hw/hppa/machine.c            |  4 ++++
> >>   hw/i386/acpi-build.c         |  3 +++
> >>   hw/i386/kvmvapic.c           |  5 +++++
> >>   hw/i386/pc.c                 | 12 +++++++++++
> >>   hw/mips/boston.c             |  1 +
> >>   hw/mips/mips_malta.c         |  9 +++++++++
> >>   hw/openrisc/openrisc_sim.c   |  1 +
> >>   hw/ppc/e500.c                |  3 +++
> >>   hw/ppc/mac_newworld.c        |  2 ++
> >>   hw/ppc/mac_oldworld.c        |  2 ++
> >>   hw/ppc/pnv.c                 |  3 +++
> >>   hw/ppc/prep.c                |  2 ++
> >>   hw/ppc/spapr.c               | 29 ++++++++++++++++++++++++++
> >>   hw/ppc/spapr_rtas.c          |  3 +++
> >>   hw/riscv/sifive_e.c          |  4 ++++
> >>   hw/riscv/sifive_plic.c       |  3 +++
> >>   hw/riscv/sifive_u.c          |  4 ++++
> >>   hw/riscv/spike.c             |  2 ++
> >>   hw/riscv/virt.c              |  1 +
> >>   hw/s390x/s390-virtio-ccw.c   |  2 ++
> >>   hw/s390x/sclp.c              |  1 +
> >>   hw/smbios/smbios.c           | 11 ++++++++++
> >>   hw/sparc/sun4m.c             |  2 ++
> >>   hw/sparc64/sun4u.c           |  2 ++
> >>   hw/xtensa/sim.c              |  1 +
> >>   hw/xtensa/xtfpga.c           |  1 +
> >>   include/hw/arm/virt.h        |  2 +-
> >>   include/hw/boards.h          |  8 ++++++++
> >>   include/sysemu/sysemu.h      |  2 +-
> >>   migration/postcopy-ram.c     |  7 +++++++
> >>   numa.c                       |  1 +
> >>   target/arm/cpu.c             |  7 +++++++
> >>   target/i386/cpu.c            |  4 ++++
> >>   target/openrisc/sys_helper.c |  5 +++++
> >>   target/s390x/cpu.c           |  3 +++
> >>   target/s390x/excp_helper.c   |  6 ++++++
> >>   tcg/tcg.c                    | 15 ++++++++++++++
> >>   vl.c                         | 48 ++++++++++++++++++++++++--------------------
> >>   57 files changed, 261 insertions(+), 25 deletions(-)
> >>  
> > 
> > 
> >   
> 
> 

Re: [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties

[Like Xu]

On 2019/4/8 21:26, Igor Mammedov wrote:
> On Thu, 4 Apr 2019 11:26:09 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
>> On 2019/3/29 18:21, Igor Mammedov wrote:
>>> On Fri, 29 Mar 2019 16:48:36 +0800
>>> Like Xu <like.xu@linux.intel.com> wrote:
>>>    
>>>> This patch series make existing cores/threads/sockets into machine
>>>> properties and get rid of global variables they use currently.
>>> Thanks for looking into it!
>>> Its long overdue and rather desired conversion (albeit naive one,
>>> but this series is a good starting point). I'll go over your patches
>>> next week with comments and concrete suggestions how to implement
>>> particular things.
>>
>> Hi Igor, any comments and suggestions on smp machine properties
>> in this patch considering we may add die topology for PCMachine as an
>> extension?
> 
> I've looked at several patches and that it for this series.
> The most comments apply to the patches I've not reviewed as well.

Hi Igor, thanks for your comments, time and patience.

I'll try to fix them in next version ASAP.

> 
>>>    
>>>> Like Xu (9):
>>>>     cpu/topology: add struct CpuTopology to MachineState
>>>>     cpu/topology: add general support for machine properties
>>>>     cpu/topology: add uncommon arch support for smp machine properties
>>>>     cpu/topology: add ARM support for smp machine properties
>>>>     cpu/topology: add i386 support for smp machine properties
>>>>     cpu/topology: add PPC support for smp machine properties
>>>>     cpu/topology: add riscv support for smp machine properties
>>>>     cpu/topology: add s390x support for smp machine properties
>>>>     cpu/topology: replace smp global variables with machine propertie
>>>>
>>>>    accel/kvm/kvm-all.c          |  3 +++
>>>>    backends/hostmem.c           |  4 ++++
>>>>    cpus.c                       |  4 ++++
>>>>    exec.c                       |  2 ++
>>>>    gdbstub.c                    |  7 ++++++-
>>>>    hw/alpha/dp264.c             |  1 +
>>>>    hw/arm/fsl-imx6.c            |  5 +++++
>>>>    hw/arm/fsl-imx6ul.c          |  5 +++++
>>>>    hw/arm/fsl-imx7.c            |  5 +++++
>>>>    hw/arm/highbank.c            |  1 +
>>>>    hw/arm/mcimx6ul-evk.c        |  1 +
>>>>    hw/arm/mcimx7d-sabre.c       |  3 +++
>>>>    hw/arm/raspi.c               |  2 ++
>>>>    hw/arm/realview.c            |  1 +
>>>>    hw/arm/sabrelite.c           |  1 +
>>>>    hw/arm/vexpress.c            |  3 +++
>>>>    hw/arm/virt.c                |  7 +++++++
>>>>    hw/arm/xlnx-zynqmp.c         |  7 +++++++
>>>>    hw/cpu/core.c                |  3 +++
>>>>    hw/hppa/machine.c            |  4 ++++
>>>>    hw/i386/acpi-build.c         |  3 +++
>>>>    hw/i386/kvmvapic.c           |  5 +++++
>>>>    hw/i386/pc.c                 | 12 +++++++++++
>>>>    hw/mips/boston.c             |  1 +
>>>>    hw/mips/mips_malta.c         |  9 +++++++++
>>>>    hw/openrisc/openrisc_sim.c   |  1 +
>>>>    hw/ppc/e500.c                |  3 +++
>>>>    hw/ppc/mac_newworld.c        |  2 ++
>>>>    hw/ppc/mac_oldworld.c        |  2 ++
>>>>    hw/ppc/pnv.c                 |  3 +++
>>>>    hw/ppc/prep.c                |  2 ++
>>>>    hw/ppc/spapr.c               | 29 ++++++++++++++++++++++++++
>>>>    hw/ppc/spapr_rtas.c          |  3 +++
>>>>    hw/riscv/sifive_e.c          |  4 ++++
>>>>    hw/riscv/sifive_plic.c       |  3 +++
>>>>    hw/riscv/sifive_u.c          |  4 ++++
>>>>    hw/riscv/spike.c             |  2 ++
>>>>    hw/riscv/virt.c              |  1 +
>>>>    hw/s390x/s390-virtio-ccw.c   |  2 ++
>>>>    hw/s390x/sclp.c              |  1 +
>>>>    hw/smbios/smbios.c           | 11 ++++++++++
>>>>    hw/sparc/sun4m.c             |  2 ++
>>>>    hw/sparc64/sun4u.c           |  2 ++
>>>>    hw/xtensa/sim.c              |  1 +
>>>>    hw/xtensa/xtfpga.c           |  1 +
>>>>    include/hw/arm/virt.h        |  2 +-
>>>>    include/hw/boards.h          |  8 ++++++++
>>>>    include/sysemu/sysemu.h      |  2 +-
>>>>    migration/postcopy-ram.c     |  7 +++++++
>>>>    numa.c                       |  1 +
>>>>    target/arm/cpu.c             |  7 +++++++
>>>>    target/i386/cpu.c            |  4 ++++
>>>>    target/openrisc/sys_helper.c |  5 +++++
>>>>    target/s390x/cpu.c           |  3 +++
>>>>    target/s390x/excp_helper.c   |  6 ++++++
>>>>    tcg/tcg.c                    | 15 ++++++++++++++
>>>>    vl.c                         | 48 ++++++++++++++++++++++++--------------------
>>>>    57 files changed, 261 insertions(+), 25 deletions(-)
>>>>   
>>>
>>>
>>>    
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties

[Like Xu]

On 2019/4/8 21:26, Igor Mammedov wrote:
> On Thu, 4 Apr 2019 11:26:09 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
>> On 2019/3/29 18:21, Igor Mammedov wrote:
>>> On Fri, 29 Mar 2019 16:48:36 +0800
>>> Like Xu <like.xu@linux.intel.com> wrote:
>>>    
>>>> This patch series make existing cores/threads/sockets into machine
>>>> properties and get rid of global variables they use currently.
>>> Thanks for looking into it!
>>> Its long overdue and rather desired conversion (albeit naive one,
>>> but this series is a good starting point). I'll go over your patches
>>> next week with comments and concrete suggestions how to implement
>>> particular things.
>>
>> Hi Igor, any comments and suggestions on smp machine properties
>> in this patch considering we may add die topology for PCMachine as an
>> extension?
> 
> I've looked at several patches and that it for this series.
> The most comments apply to the patches I've not reviewed as well.

Hi Igor, thanks for your comments, time and patience.

I'll try to fix them in next version ASAP.

> 
>>>    
>>>> Like Xu (9):
>>>>     cpu/topology: add struct CpuTopology to MachineState
>>>>     cpu/topology: add general support for machine properties
>>>>     cpu/topology: add uncommon arch support for smp machine properties
>>>>     cpu/topology: add ARM support for smp machine properties
>>>>     cpu/topology: add i386 support for smp machine properties
>>>>     cpu/topology: add PPC support for smp machine properties
>>>>     cpu/topology: add riscv support for smp machine properties
>>>>     cpu/topology: add s390x support for smp machine properties
>>>>     cpu/topology: replace smp global variables with machine propertie
>>>>
>>>>    accel/kvm/kvm-all.c          |  3 +++
>>>>    backends/hostmem.c           |  4 ++++
>>>>    cpus.c                       |  4 ++++
>>>>    exec.c                       |  2 ++
>>>>    gdbstub.c                    |  7 ++++++-
>>>>    hw/alpha/dp264.c             |  1 +
>>>>    hw/arm/fsl-imx6.c            |  5 +++++
>>>>    hw/arm/fsl-imx6ul.c          |  5 +++++
>>>>    hw/arm/fsl-imx7.c            |  5 +++++
>>>>    hw/arm/highbank.c            |  1 +
>>>>    hw/arm/mcimx6ul-evk.c        |  1 +
>>>>    hw/arm/mcimx7d-sabre.c       |  3 +++
>>>>    hw/arm/raspi.c               |  2 ++
>>>>    hw/arm/realview.c            |  1 +
>>>>    hw/arm/sabrelite.c           |  1 +
>>>>    hw/arm/vexpress.c            |  3 +++
>>>>    hw/arm/virt.c                |  7 +++++++
>>>>    hw/arm/xlnx-zynqmp.c         |  7 +++++++
>>>>    hw/cpu/core.c                |  3 +++
>>>>    hw/hppa/machine.c            |  4 ++++
>>>>    hw/i386/acpi-build.c         |  3 +++
>>>>    hw/i386/kvmvapic.c           |  5 +++++
>>>>    hw/i386/pc.c                 | 12 +++++++++++
>>>>    hw/mips/boston.c             |  1 +
>>>>    hw/mips/mips_malta.c         |  9 +++++++++
>>>>    hw/openrisc/openrisc_sim.c   |  1 +
>>>>    hw/ppc/e500.c                |  3 +++
>>>>    hw/ppc/mac_newworld.c        |  2 ++
>>>>    hw/ppc/mac_oldworld.c        |  2 ++
>>>>    hw/ppc/pnv.c                 |  3 +++
>>>>    hw/ppc/prep.c                |  2 ++
>>>>    hw/ppc/spapr.c               | 29 ++++++++++++++++++++++++++
>>>>    hw/ppc/spapr_rtas.c          |  3 +++
>>>>    hw/riscv/sifive_e.c          |  4 ++++
>>>>    hw/riscv/sifive_plic.c       |  3 +++
>>>>    hw/riscv/sifive_u.c          |  4 ++++
>>>>    hw/riscv/spike.c             |  2 ++
>>>>    hw/riscv/virt.c              |  1 +
>>>>    hw/s390x/s390-virtio-ccw.c   |  2 ++
>>>>    hw/s390x/sclp.c              |  1 +
>>>>    hw/smbios/smbios.c           | 11 ++++++++++
>>>>    hw/sparc/sun4m.c             |  2 ++
>>>>    hw/sparc64/sun4u.c           |  2 ++
>>>>    hw/xtensa/sim.c              |  1 +
>>>>    hw/xtensa/xtfpga.c           |  1 +
>>>>    include/hw/arm/virt.h        |  2 +-
>>>>    include/hw/boards.h          |  8 ++++++++
>>>>    include/sysemu/sysemu.h      |  2 +-
>>>>    migration/postcopy-ram.c     |  7 +++++++
>>>>    numa.c                       |  1 +
>>>>    target/arm/cpu.c             |  7 +++++++
>>>>    target/i386/cpu.c            |  4 ++++
>>>>    target/openrisc/sys_helper.c |  5 +++++
>>>>    target/s390x/cpu.c           |  3 +++
>>>>    target/s390x/excp_helper.c   |  6 ++++++
>>>>    tcg/tcg.c                    | 15 ++++++++++++++
>>>>    vl.c                         | 48 ++++++++++++++++++++++++--------------------
>>>>    57 files changed, 261 insertions(+), 25 deletions(-)
>>>>   
>>>
>>>
>>>    
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH 1/9] cpu/topology: add struct CpuTopology to MachineState

[Igor Mammedov]

On Fri, 29 Mar 2019 16:48:37 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> ---
>  include/hw/arm/virt.h   | 2 +-
>  include/hw/boards.h     | 8 ++++++++
>  include/sysemu/sysemu.h | 2 +-
>  vl.c                    | 7 ++++++-
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 507517c..724da0c 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -123,7 +123,7 @@ typedef struct {
>      struct arm_boot_info bootinfo;
>      MemMapEntry *memmap;
>      const int *irqmap;
> -    int smp_cpus;
> +    unsigned int smp_cpus;
As Alex pointed out it's unrelated cleanup
(with other similar hunks in this patch),
it's better to split it out into separate patch

>      void *fdt;
>      int fdt_size;
>      uint32_t clock_phandle;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index e231860..cbde276 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -231,6 +231,13 @@ typedef struct DeviceMemoryState {
>      MemoryRegion mr;
>  } DeviceMemoryState;
>  
> +typedef struct CpuTopology {
> +    unsigned int smp_cpus;
> +    unsigned int smp_cores;
> +    unsigned int smp_threads;
> +    unsigned int max_cpus;
> +} CpuTopology;
> +
>  /**
>   * MachineState:
>   */
> @@ -273,6 +280,7 @@ struct MachineState {
>      const char *cpu_type;
>      AccelState *accelerator;
>      CPUArchIdList *possible_cpus;
> +    CpuTopology topo;
maybe s/topo/smp/ and drop smp_ prefix inside CpuTopology structure

>      struct NVDIMMState *nvdimms_state;
>  };
>  
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6065d9e..c0d7d7c 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -100,7 +100,7 @@ extern const char *keyboard_layout;
>  extern int win2k_install_hack;
>  extern int alt_grab;
>  extern int ctrl_grab;
> -extern int smp_cpus;
> +extern unsigned int smp_cpus;
>  extern unsigned int max_cpus;
>  extern int cursor_hide;
>  extern int graphic_rotate;
> diff --git a/vl.c b/vl.c
> index d61d560..9089253 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -162,7 +162,7 @@ static Chardev **serial_hds;
>  Chardev *parallel_hds[MAX_PARALLEL_PORTS];
>  int win2k_install_hack = 0;
>  int singlestep = 0;
> -int smp_cpus;
> +unsigned int smp_cpus;
>  unsigned int max_cpus;
>  int smp_cores = 1;
>  int smp_threads = 1;
> @@ -4116,6 +4116,11 @@ int main(int argc, char **argv, char **envp)
>  
>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>  
> +    current_machine->topo.smp_cpus = smp_cpus;
> +    current_machine->topo.max_cpus = max_cpus;
> +    current_machine->topo.smp_cores = smp_cores;
> +    current_machine->topo.smp_threads = smp_threads;
> +
>      /* sanity-check smp_cpus and max_cpus against machine_class */
>      if (smp_cpus < machine_class->min_cpus) {
>          error_report("Invalid SMP CPUs %d. The min CPUs "

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

[Igor Mammedov]

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

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> ---
>  accel/kvm/kvm-all.c      |  3 +++
>  backends/hostmem.c       |  4 ++++
>  cpus.c                   |  4 ++++
>  exec.c                   |  2 ++
>  gdbstub.c                |  7 ++++++-
>  hw/cpu/core.c            |  3 +++
>  hw/smbios/smbios.c       | 11 +++++++++++
>  migration/postcopy-ram.c |  7 +++++++
>  numa.c                   |  1 +
>  tcg/tcg.c                | 15 +++++++++++++++
>  10 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 241db49..5385218 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1526,6 +1526,9 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
>  static int kvm_init(MachineState *ms)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +    unsigned int max_cpus = ms->topo.max_cpus;
> +
>      static const char upgrade_note[] =
>          "Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n"
>          "(see http://sourceforge.net/projects/kvm).\n";

> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 04baf47..cecdfd5 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -222,6 +222,8 @@ static void host_memory_backend_set_prealloc(Object *obj, bool value,
>  {
>      Error *local_err = NULL;
>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (backend->force_prealloc) {
>          if (value) {
> @@ -311,6 +313,8 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>  {
>      HostMemoryBackend *backend = MEMORY_BACKEND(uc);
>      HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      Error *local_err = NULL;
>      void *ptr;
>      uint64_t sz;
I don't like us abusing smp_cpus for os_mem_prealloc in here as
it doesn't really have to do anything with machine and just adds
non necessary machine dependency to hostmem. And probably should
be a separate option for backend to specify a number of threads
to use for allocation.

But that's out of scope of this series, so it's fine is for now
and later we should get rid of it (which I suspect would be easier
once initial memoy also allocated by backends).

> 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()).

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.

> +
>      cpu->nr_cores = smp_cores;
>      cpu->nr_threads = smp_threads;
>      cpu->stopped = true;
> diff --git a/exec.c b/exec.c
> index 86a38d3..a3c3db7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1829,6 +1829,8 @@ static void *file_ram_alloc(RAMBlock *block,
>                              bool truncate,
>                              Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      void *area;
>  
>      block->page_size = qemu_fd_getpagesize(fd);
> diff --git a/gdbstub.c b/gdbstub.c
> index d54abd1..35f6bbc 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -30,6 +30,7 @@
>  #include "sysemu/sysemu.h"
>  #include "exec/gdbstub.h"
>  #include "hw/cpu/cluster.h"
> +#include "hw/boards.h"
>  #endif
>  
>  #define MAX_PACKET_LENGTH 4096
> @@ -1154,11 +1155,15 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>      CPUState *cpu;
>      GDBThreadIdKind kind;
>  #ifdef CONFIG_USER_ONLY
> -    int max_cpus = 1; /* global variable max_cpus exists only in system mode */
> +    /* global variable max_cpus exists only in system mode */
> +    unsigned int max_cpus = 1;
not related change

>  
>      CPU_FOREACH(cpu) {
>          max_cpus = max_cpus <= cpu->cpu_index ? cpu->cpu_index + 1 : max_cpus;
>      }
> +#else
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int max_cpus = ms->topo.max_cpus;
>  #endif
>      /* uninitialised CPUs stay 0 */
>      newstates = g_new0(char, max_cpus);
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 7e42e2c..b75ffbb 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -11,6 +11,7 @@
>  #include "qapi/visitor.h"
>  #include "qapi/error.h"
>  #include "sysemu/cpus.h"
> +#include "hw/boards.h"
>  
>  static void core_prop_get_core_id(Object *obj, Visitor *v, const char *name,
>                                    void *opaque, Error **errp)
> @@ -69,6 +70,8 @@ static void core_prop_set_nr_threads(Object *obj, Visitor *v, const char *name,
>  
>  static void cpu_core_instance_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_threads = ms->topo.smp_threads;
the same as [***]

>      CPUCore *core = CPU_CORE(obj);
>  
>      object_property_add(obj, "core-id", "int", core_prop_get_core_id,
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 47be907..a5eabe7 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -28,6 +28,7 @@
>  #include "hw/loader.h"
>  #include "exec/cpu-common.h"
>  #include "smbios_build.h"
> +#include "hw/boards.h"
>  
>  /* legacy structures and constants for <= 2.0 machines */
>  struct smbios_header {
> @@ -342,6 +343,9 @@ opts_init(smbios_register_config);
>  
>  static void smbios_validate_table(void)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +
>      uint32_t expect_t4_count = smbios_legacy ? smp_cpus : smbios_smp_sockets;
>  
>      if (smbios_type4_count && smbios_type4_count != expect_t4_count) {
> @@ -571,6 +575,9 @@ static void smbios_build_type_3_table(void)
>  
>  static void smbios_build_type_4_table(unsigned instance)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_threads = ms->topo.smp_threads;
> +    unsigned int smp_cores = ms->topo.smp_cores;
>      char sock_str[128];
>  
>      SMBIOS_BUILD_TABLE_PRE(4, 0x400 + instance, true); /* required */
> @@ -843,7 +850,11 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>                         uint8_t **tables, size_t *tables_len,
>                         uint8_t **anchor, size_t *anchor_len)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      unsigned i, dimm_cnt;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +    unsigned int smp_cores = ms->topo.smp_cores;
> +    unsigned int smp_threads = ms->topo.smp_threads;
>  
wrt whole smbios changes:
instead of accessing machine directly, I suggest to pass necessary values
as arguments and modify callers to provide them

>      if (smbios_legacy) {
>          *tables = *anchor = NULL;
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index e2aa57a..ae92f6e 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -29,6 +29,7 @@
>  #include "sysemu/balloon.h"
>  #include "qemu/error-report.h"
>  #include "trace.h"
> +#include "hw/boards.h"
>  
>  /* Arbitrary limit on size of each discard command,
>   * keeps them around ~200 bytes
> @@ -128,6 +129,8 @@ static void migration_exit_cb(Notifier *n, void *data)
>  
>  static struct PostcopyBlocktimeContext *blocktime_context_new(void)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      PostcopyBlocktimeContext *ctx = g_new0(PostcopyBlocktimeContext, 1);
>      ctx->page_fault_vcpu_time = g_new0(uint32_t, smp_cpus);
>      ctx->vcpu_addr = g_new0(uintptr_t, smp_cpus);
> @@ -141,6 +144,8 @@ static struct PostcopyBlocktimeContext *blocktime_context_new(void)
>  
>  static uint32List *get_vcpu_blocktime_list(PostcopyBlocktimeContext *ctx)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      uint32List *list = NULL, *entry = NULL;
>      int i;
>  
> @@ -806,8 +811,10 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid,
>  static void mark_postcopy_blocktime_end(uintptr_t addr)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      PostcopyBlocktimeContext *dc = mis->blocktime_ctx;
>      int i, affected_cpu = 0;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      bool vcpu_total_blocktime = false;
>      uint32_t read_vcpu_time, low_time_offset;

I don't know enough about migration to say some thing useful here,
CCin David for comments.

>  
> diff --git a/numa.c b/numa.c
> index 3875e1e..127ddbe 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -64,6 +64,7 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>      uint16_t nodenr;
>      uint16List *cpus = NULL;
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    unsigned int max_cpus = ms->topo.max_cpus;
>  
>      if (node->has_nodeid) {
>          nodenr = node->nodeid;
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 9b2bf7f..d1501eb 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -44,6 +44,10 @@
>  #include "exec/cpu-common.h"
>  #include "exec/exec-all.h"
>  
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/boards.h"
> +#endif
> +
>  #include "tcg-op.h"
>  
>  #if UINTPTR_MAX == UINT32_MAX
> @@ -602,6 +606,10 @@ static size_t tcg_n_regions(void)
>      size_t i;
>  
>      /* Use a single region if all we have is one vCPU thread */
> +#if !defined(CONFIG_USER_ONLY)
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int max_cpus = ms->topo.max_cpus;
> +#endif
>      if (max_cpus == 1 || !qemu_tcg_mttcg_enabled()) {
>          return 1;
>      }
> @@ -751,7 +759,12 @@ void tcg_register_thread(void)
>  
>      /* Claim an entry in tcg_ctxs */
>      n = atomic_fetch_inc(&n_tcg_ctxs);
> +#if !defined(CONFIG_USER_ONLY)
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    g_assert(n < ms->topo.max_cpus);
> +#elif
>      g_assert(n < max_cpus);
> +#endif
>      atomic_set(&tcg_ctxs[n], s);
>  
>      tcg_ctx = s;
> @@ -961,6 +974,8 @@ void tcg_context_init(TCGContext *s)
>      tcg_ctxs = &tcg_ctx;
>      n_tcg_ctxs = 1;
>  #else
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int max_cpus = ms->topo.max_cpus;
>      tcg_ctxs = g_new(TCGContext *, max_cpus);
>  #endif

not sure what to in case of accelerators (TCG/KVM/...), as their code
eventually endups being used by cpus. Passing smp information down
from CPU would be a pain and might not work in every case. Maybe
we should pass it as part of configure_accelerator()

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

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Fri, 29 Mar 2019 16:48:38 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
> > Signed-off-by: Like Xu <like.xu@linux.intel.com>
> > ---

<snipp>

> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index e2aa57a..ae92f6e 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -29,6 +29,7 @@
> >  #include "sysemu/balloon.h"
> >  #include "qemu/error-report.h"
> >  #include "trace.h"
> > +#include "hw/boards.h"
> >  
> >  /* Arbitrary limit on size of each discard command,
> >   * keeps them around ~200 bytes
> > @@ -128,6 +129,8 @@ static void migration_exit_cb(Notifier *n, void *data)
> >  
> >  static struct PostcopyBlocktimeContext *blocktime_context_new(void)
> >  {
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >      PostcopyBlocktimeContext *ctx = g_new0(PostcopyBlocktimeContext, 1);
> >      ctx->page_fault_vcpu_time = g_new0(uint32_t, smp_cpus);
> >      ctx->vcpu_addr = g_new0(uintptr_t, smp_cpus);
> > @@ -141,6 +144,8 @@ static struct PostcopyBlocktimeContext *blocktime_context_new(void)
> >  
> >  static uint32List *get_vcpu_blocktime_list(PostcopyBlocktimeContext *ctx)
> >  {
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >      uint32List *list = NULL, *entry = NULL;
> >      int i;
> >  
> > @@ -806,8 +811,10 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid,
> >  static void mark_postcopy_blocktime_end(uintptr_t addr)
> >  {
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> >      PostcopyBlocktimeContext *dc = mis->blocktime_ctx;
> >      int i, affected_cpu = 0;
> > +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >      bool vcpu_total_blocktime = false;
> >      uint32_t read_vcpu_time, low_time_offset;
> 
> I don't know enough about migration to say some thing useful here,
> CCin David for comments.

I think that's OK; we just need to know the total number of vcpus; this
thing calculates some stats based on the number of the vCPUs that are
blocked by postcopy page waits, and in particular if everyone is
blocked.

(I'd slightly prefer macs there rather than ms, just because we tend to
use ms in migration for MigrationState sometimes, but not always).

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [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 <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>

Re: [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 <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>

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

[Igor Mammedov]

On Tue, 30 Apr 2019 15:30:31 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> 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:
> > 

[...]

> 
> 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.

can you point to concrete place that needs access to nr_cores / nr_threads
before cpu is 'realized'?

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

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

[Igor Mammedov]

On Tue, 30 Apr 2019 15:30:31 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> 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:
> > 

[...]

> 
> 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.

can you point to concrete place that needs access to nr_cores / nr_threads
before cpu is 'realized'?

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

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

[Eduardo Habkost]

On Thu, May 02, 2019 at 05:09:28PM +0200, Igor Mammedov wrote:
> On Tue, 30 Apr 2019 15:30:31 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
> > 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:
> > > 
> 
> [...]
> 
> > 
> > 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.
> 
> can you point to concrete place that needs access to nr_cores / nr_threads
> before cpu is 'realized'?

We have very few architectures actually using
nr_cores/nr_threads/smp_cores/smp_threads.  I think those
variables are used only on x86, ppc, and mips.

I believe I suggested some time ago that we should get rid of the
nr_cores/nr_threads CPUState fields and move them to
arch-specific types.  This would help us avoid confusion when
different architectures have different semantics for
nr_cores/nr_threads.

See https://www.mail-archive.com/qemu-devel@nongnu.org/msg587105.html
("[Qemu-devel] Meaning of '-smp threads' on mips_malta") for one
example of confusing arch-specific semantics.

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Apr 30, 2019 at 03:30:31PM +0800, Like Xu wrote:
> 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

Will it?  cpus.o is compiled only on CONFIG_SOFTMMU.

> > 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.

I agree with Igor's goal, but let's do it one step at a time.
Replacing the smp_* globals with qdev_get_machine() is a step in
the right direction, IMO.


> 
> > 
> > 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.

I don't see why it should precede -smp refactoring.

We have a very specific user-visible goal here: making one extra
CPU topology option (CPU dies) available on PC only.  Asking the
author to refactor some code while implementing that is
reasonable.  Requiring the author to touch every single place
where a CPU object is created in QEMU just to avoid a
qdev_get_machine() call doesn't seem reasonable.


> > 
> 
> 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?

I believe we need a better CPU creation API that all machines can
use, otherwise all of them will have to duplicate the same code
between object_new() and object_propert_set_bool("realized", true).

But I really don't think designing and implementing this new API
should be a requirement to get useful work done.

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Apr 30, 2019 at 03:30:31PM +0800, Like Xu wrote:
> 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

Will it?  cpus.o is compiled only on CONFIG_SOFTMMU.

> > 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.

I agree with Igor's goal, but let's do it one step at a time.
Replacing the smp_* globals with qdev_get_machine() is a step in
the right direction, IMO.


> 
> > 
> > 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.

I don't see why it should precede -smp refactoring.

We have a very specific user-visible goal here: making one extra
CPU topology option (CPU dies) available on PC only.  Asking the
author to refactor some code while implementing that is
reasonable.  Requiring the author to touch every single place
where a CPU object is created in QEMU just to avoid a
qdev_get_machine() call doesn't seem reasonable.


> > 
> 
> 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?

I believe we need a better CPU creation API that all machines can
use, otherwise all of them will have to duplicate the same code
between object_new() and object_propert_set_bool("realized", true).

But I really don't think designing and implementing this new API
should be a requirement to get useful work done.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties

[Igor Mammedov]

On Fri, 29 Mar 2019 16:48:39 +0800
Like Xu <like.xu@linux.intel.com> wrote:

here should be a commit message explaining what patch does
in more detail.


> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Generic note, try not call qdev_get_machine() every time
you replace smp_cpus or other variables. It's often possible
to pass MachineState via call chain with trivial fixups.

> ---
>  hw/alpha/dp264.c     | 1 +
>  hw/hppa/machine.c    | 4 ++++
>  hw/mips/boston.c     | 1 +
>  hw/mips/mips_malta.c | 9 +++++++++
>  hw/sparc/sun4m.c     | 2 ++
>  hw/sparc64/sun4u.c   | 2 ++
>  hw/xtensa/sim.c      | 1 +
>  hw/xtensa/xtfpga.c   | 1 +
>  8 files changed, 21 insertions(+)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 0347eb8..ee5d432 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine)
>      char *palcode_filename;
>      uint64_t palcode_entry, palcode_low, palcode_high;
>      uint64_t kernel_entry, kernel_low, kernel_high;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      /* Create up to 4 cpus.  */
>      memset(cpus, 0, sizeof(cpus));
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index d1b1d3c..f652891 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -16,6 +16,7 @@
>  #include "hw/ide.h"
>  #include "hw/timer/i8254.h"
>  #include "hw/char/serial.h"
> +#include "hw/boards.h"
>  #include "hppa_sys.h"
>  #include "qemu/units.h"
>  #include "qapi/error.h"
> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine)
>      MemoryRegion *ram_region;
>      MemoryRegion *cpu_region;
>      long i;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
I'd prefer to replace smp_cpus with machine->topo.smp_cpus
directly at places it's used, as it makes affected sites
more visible in the patch.
And use local smp_cpus only in places where using machine->topo.smp_cpus
makes core less readable.
(but it's just personal preference so I don't insist on it)

>  
>      ram_size = machine->ram_size;
>  
> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine)
>  
>  static void hppa_machine_reset(void)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;

***)
It would be better to pass MachineState as argument to
hppa_machine_reset(), a patch to so should go before this one.

Quick look shows only 3 overrides (hppa, pc, pnv) and one caller,
so I'd rather fix it than calling qdev_get_machine() unnecessarily 

>  
>      qemu_devices_reset();
>  
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index e5bab3c..7752c10 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine)
>      DriveInfo *hd[6];
>      Chardev *chr;
>      int fw_size, fit_err;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>      bool is_64b;
>  
>      if ((machine->ram_size % GiB) ||
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 439665a..d595375 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void)
>  
>  static void malta_mips_config(MIPSCPU *cpu)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      CPUMIPSState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
this one also called from reset, so the same [***] applies here too.

>  
> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque)
>  static void create_cpu_without_cps(const char *cpu_type,
>                                     qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
caller has an access to MachineState so pass it down call chain all the way

>      CPUMIPSState *env;
>      MIPSCPU *cpu;
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      for (i = 0; i < smp_cpus; i++) {
>          cpu = MIPS_CPU(cpu_create(cpu_type));
> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type,
>  static void create_cps(MaltaState *s, const char *cpu_type,
>                         qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
ditto

>      Error *err = NULL;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS));
>      qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default());
> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type,
>  static void mips_create_cpu(MaltaState *s, const char *cpu_type,
>                              qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
ditto

> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +
>      if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) {
>          create_cps(s, cpu_type, cbus_irq, i8259_irq);
>      } else {
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index ca1e382..2321cfb 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>      unsigned int num_vsimms;
>      DeviceState *dev;
>      SysBusDevice *s;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> +    unsigned int max_cpus = machine->topo.max_cpus;
>  
>      /* init CPUs */
>      for(i = 0; i < smp_cpus; i++) {
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 399f2d7..d089c4d 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>      NICInfo *nd;
>      MACAddr macaddr;
>      bool onboard_nic;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> +    unsigned int max_cpus = machine->topo.max_cpus;
>  
>      /* init CPUs */
>      cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> index 12c7437..cc0396b 100644
> --- a/hw/xtensa/sim.c
> +++ b/hw/xtensa/sim.c
> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine)
>      ram_addr_t ram_size = machine->ram_size;
>      const char *kernel_filename = machine->kernel_filename;
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      for (n = 0; n < smp_cpus; n++) {
>          cpu = XTENSA_CPU(cpu_create(machine->cpu_type));
> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> index e05ef75..f71a15e 100644
> --- a/hw/xtensa/xtfpga.c
> +++ b/hw/xtensa/xtfpga.c
> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>      const unsigned system_io_size = 224 * MiB;
>      uint32_t freq = 10000000;
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      if (smp_cpus > 1) {
>          mx_pic = xtensa_mx_pic_init(31);

Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties

[Igor Mammedov]

On Fri, 29 Mar 2019 16:48:39 +0800
Like Xu <like.xu@linux.intel.com> wrote:

here should be a commit message explaining what patch does
in more detail.


> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Generic note, try not call qdev_get_machine() every time
you replace smp_cpus or other variables. It's often possible
to pass MachineState via call chain with trivial fixups.

> ---
>  hw/alpha/dp264.c     | 1 +
>  hw/hppa/machine.c    | 4 ++++
>  hw/mips/boston.c     | 1 +
>  hw/mips/mips_malta.c | 9 +++++++++
>  hw/sparc/sun4m.c     | 2 ++
>  hw/sparc64/sun4u.c   | 2 ++
>  hw/xtensa/sim.c      | 1 +
>  hw/xtensa/xtfpga.c   | 1 +
>  8 files changed, 21 insertions(+)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 0347eb8..ee5d432 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine)
>      char *palcode_filename;
>      uint64_t palcode_entry, palcode_low, palcode_high;
>      uint64_t kernel_entry, kernel_low, kernel_high;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      /* Create up to 4 cpus.  */
>      memset(cpus, 0, sizeof(cpus));
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index d1b1d3c..f652891 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -16,6 +16,7 @@
>  #include "hw/ide.h"
>  #include "hw/timer/i8254.h"
>  #include "hw/char/serial.h"
> +#include "hw/boards.h"
>  #include "hppa_sys.h"
>  #include "qemu/units.h"
>  #include "qapi/error.h"
> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine)
>      MemoryRegion *ram_region;
>      MemoryRegion *cpu_region;
>      long i;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
I'd prefer to replace smp_cpus with machine->topo.smp_cpus
directly at places it's used, as it makes affected sites
more visible in the patch.
And use local smp_cpus only in places where using machine->topo.smp_cpus
makes core less readable.
(but it's just personal preference so I don't insist on it)

>  
>      ram_size = machine->ram_size;
>  
> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine)
>  
>  static void hppa_machine_reset(void)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;

***)
It would be better to pass MachineState as argument to
hppa_machine_reset(), a patch to so should go before this one.

Quick look shows only 3 overrides (hppa, pc, pnv) and one caller,
so I'd rather fix it than calling qdev_get_machine() unnecessarily 

>  
>      qemu_devices_reset();
>  
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index e5bab3c..7752c10 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine)
>      DriveInfo *hd[6];
>      Chardev *chr;
>      int fw_size, fit_err;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>      bool is_64b;
>  
>      if ((machine->ram_size % GiB) ||
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 439665a..d595375 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void)
>  
>  static void malta_mips_config(MIPSCPU *cpu)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      CPUMIPSState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
this one also called from reset, so the same [***] applies here too.

>  
> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque)
>  static void create_cpu_without_cps(const char *cpu_type,
>                                     qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
caller has an access to MachineState so pass it down call chain all the way

>      CPUMIPSState *env;
>      MIPSCPU *cpu;
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      for (i = 0; i < smp_cpus; i++) {
>          cpu = MIPS_CPU(cpu_create(cpu_type));
> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type,
>  static void create_cps(MaltaState *s, const char *cpu_type,
>                         qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
ditto

>      Error *err = NULL;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS));
>      qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default());
> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type,
>  static void mips_create_cpu(MaltaState *s, const char *cpu_type,
>                              qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
ditto

> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +
>      if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) {
>          create_cps(s, cpu_type, cbus_irq, i8259_irq);
>      } else {
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index ca1e382..2321cfb 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>      unsigned int num_vsimms;
>      DeviceState *dev;
>      SysBusDevice *s;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> +    unsigned int max_cpus = machine->topo.max_cpus;
>  
>      /* init CPUs */
>      for(i = 0; i < smp_cpus; i++) {
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 399f2d7..d089c4d 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>      NICInfo *nd;
>      MACAddr macaddr;
>      bool onboard_nic;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> +    unsigned int max_cpus = machine->topo.max_cpus;
>  
>      /* init CPUs */
>      cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> index 12c7437..cc0396b 100644
> --- a/hw/xtensa/sim.c
> +++ b/hw/xtensa/sim.c
> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine)
>      ram_addr_t ram_size = machine->ram_size;
>      const char *kernel_filename = machine->kernel_filename;
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      for (n = 0; n < smp_cpus; n++) {
>          cpu = XTENSA_CPU(cpu_create(machine->cpu_type));
> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> index e05ef75..f71a15e 100644
> --- a/hw/xtensa/xtfpga.c
> +++ b/hw/xtensa/xtfpga.c
> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>      const unsigned system_io_size = 224 * MiB;
>      uint32_t freq = 10000000;
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      if (smp_cpus > 1) {
>          mx_pic = xtensa_mx_pic_init(31);

Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties

[Like Xu]

On 2019/4/8 20:54, Igor Mammedov wrote:
> On Fri, 29 Mar 2019 16:48:39 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
> here should be a commit message explaining what patch does
> in more detail.
> 
> 
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> 
> Generic note, try not call qdev_get_machine() every time
> you replace smp_cpus or other variables. It's often possible
> to pass MachineState via call chain with trivial fixups.

Hi Igor,

I have some doubts on this comments after some attempts.

I'm not sure if this idea could apply to all qdev_get_machine()
usages in tree or just for this smp-touch-only patch.

It takes a lot of efforts on hooks overrides when we
undo calls to qdev_get_machine() with modification of incoming parameters.

The implementation of qdev_get_machine() couldn't be simpler more
and it doesn't seem to bring much overhead compared with parameter stack.

> 
>> ---
>>   hw/alpha/dp264.c     | 1 +
>>   hw/hppa/machine.c    | 4 ++++
>>   hw/mips/boston.c     | 1 +
>>   hw/mips/mips_malta.c | 9 +++++++++
>>   hw/sparc/sun4m.c     | 2 ++
>>   hw/sparc64/sun4u.c   | 2 ++
>>   hw/xtensa/sim.c      | 1 +
>>   hw/xtensa/xtfpga.c   | 1 +
>>   8 files changed, 21 insertions(+)
>>
>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> index 0347eb8..ee5d432 100644
>> --- a/hw/alpha/dp264.c
>> +++ b/hw/alpha/dp264.c
>> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine)
>>       char *palcode_filename;
>>       uint64_t palcode_entry, palcode_low, palcode_high;
>>       uint64_t kernel_entry, kernel_low, kernel_high;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>   
>>       /* Create up to 4 cpus.  */
>>       memset(cpus, 0, sizeof(cpus));
>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index d1b1d3c..f652891 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -16,6 +16,7 @@
>>   #include "hw/ide.h"
>>   #include "hw/timer/i8254.h"
>>   #include "hw/char/serial.h"
>> +#include "hw/boards.h"
>>   #include "hppa_sys.h"
>>   #include "qemu/units.h"
>>   #include "qapi/error.h"
>> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine)
>>       MemoryRegion *ram_region;
>>       MemoryRegion *cpu_region;
>>       long i;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> I'd prefer to replace smp_cpus with machine->topo.smp_cpus
> directly at places it's used, as it makes affected sites
> more visible in the patch.
> And use local smp_cpus only in places where using machine->topo.smp_cpus
> makes core less readable.
> (but it's just personal preference so I don't insist on it)
> 
>>   
>>       ram_size = machine->ram_size;
>>   
>> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine)
>>   
>>   static void hppa_machine_reset(void)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>       int i;
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> 
> ***)
> It would be better to pass MachineState as argument to
> hppa_machine_reset(), a patch to so should go before this one.
> 
> Quick look shows only 3 overrides (hppa, pc, pnv) and one caller,
> so I'd rather fix it than calling qdev_get_machine() unnecessarily
> 
>>   
>>       qemu_devices_reset();
>>   
>> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
>> index e5bab3c..7752c10 100644
>> --- a/hw/mips/boston.c
>> +++ b/hw/mips/boston.c
>> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine)
>>       DriveInfo *hd[6];
>>       Chardev *chr;
>>       int fw_size, fit_err;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>       bool is_64b;
>>   
>>       if ((machine->ram_size % GiB) ||
>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>> index 439665a..d595375 100644
>> --- a/hw/mips/mips_malta.c
>> +++ b/hw/mips/mips_malta.c
>> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void)
>>   
>>   static void malta_mips_config(MIPSCPU *cpu)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>>       CPUMIPSState *env = &cpu->env;
>>       CPUState *cs = CPU(cpu);
> this one also called from reset, so the same [***] applies here too.
> 
>>   
>> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque)
>>   static void create_cpu_without_cps(const char *cpu_type,
>>                                      qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> caller has an access to MachineState so pass it down call chain all the way
> 
>>       CPUMIPSState *env;
>>       MIPSCPU *cpu;
>>       int i;
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>>   
>>       for (i = 0; i < smp_cpus; i++) {
>>           cpu = MIPS_CPU(cpu_create(cpu_type));
>> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type,
>>   static void create_cps(MaltaState *s, const char *cpu_type,
>>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> ditto
> 
>>       Error *err = NULL;
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>>   
>>       s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS));
>>       qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default());
>> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type,
>>   static void mips_create_cpu(MaltaState *s, const char *cpu_type,
>>                               qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> ditto
> 
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>> +
>>       if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) {
>>           create_cps(s, cpu_type, cbus_irq, i8259_irq);
>>       } else {
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index ca1e382..2321cfb 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>>       unsigned int num_vsimms;
>>       DeviceState *dev;
>>       SysBusDevice *s;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>> +    unsigned int max_cpus = machine->topo.max_cpus;
>>   
>>       /* init CPUs */
>>       for(i = 0; i < smp_cpus; i++) {
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 399f2d7..d089c4d 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>       NICInfo *nd;
>>       MACAddr macaddr;
>>       bool onboard_nic;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>> +    unsigned int max_cpus = machine->topo.max_cpus;
>>   
>>       /* init CPUs */
>>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
>> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
>> index 12c7437..cc0396b 100644
>> --- a/hw/xtensa/sim.c
>> +++ b/hw/xtensa/sim.c
>> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine)
>>       ram_addr_t ram_size = machine->ram_size;
>>       const char *kernel_filename = machine->kernel_filename;
>>       int n;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>   
>>       for (n = 0; n < smp_cpus; n++) {
>>           cpu = XTENSA_CPU(cpu_create(machine->cpu_type));
>> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
>> index e05ef75..f71a15e 100644
>> --- a/hw/xtensa/xtfpga.c
>> +++ b/hw/xtensa/xtfpga.c
>> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>>       const unsigned system_io_size = 224 * MiB;
>>       uint32_t freq = 10000000;
>>       int n;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>   
>>       if (smp_cpus > 1) {
>>           mx_pic = xtensa_mx_pic_init(31);
> 
> 

Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties

[Like Xu]

On 2019/4/8 20:54, Igor Mammedov wrote:
> On Fri, 29 Mar 2019 16:48:39 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
> 
> here should be a commit message explaining what patch does
> in more detail.
> 
> 
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> 
> Generic note, try not call qdev_get_machine() every time
> you replace smp_cpus or other variables. It's often possible
> to pass MachineState via call chain with trivial fixups.

Hi Igor,

I have some doubts on this comments after some attempts.

I'm not sure if this idea could apply to all qdev_get_machine()
usages in tree or just for this smp-touch-only patch.

It takes a lot of efforts on hooks overrides when we
undo calls to qdev_get_machine() with modification of incoming parameters.

The implementation of qdev_get_machine() couldn't be simpler more
and it doesn't seem to bring much overhead compared with parameter stack.

> 
>> ---
>>   hw/alpha/dp264.c     | 1 +
>>   hw/hppa/machine.c    | 4 ++++
>>   hw/mips/boston.c     | 1 +
>>   hw/mips/mips_malta.c | 9 +++++++++
>>   hw/sparc/sun4m.c     | 2 ++
>>   hw/sparc64/sun4u.c   | 2 ++
>>   hw/xtensa/sim.c      | 1 +
>>   hw/xtensa/xtfpga.c   | 1 +
>>   8 files changed, 21 insertions(+)
>>
>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> index 0347eb8..ee5d432 100644
>> --- a/hw/alpha/dp264.c
>> +++ b/hw/alpha/dp264.c
>> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine)
>>       char *palcode_filename;
>>       uint64_t palcode_entry, palcode_low, palcode_high;
>>       uint64_t kernel_entry, kernel_low, kernel_high;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>   
>>       /* Create up to 4 cpus.  */
>>       memset(cpus, 0, sizeof(cpus));
>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index d1b1d3c..f652891 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -16,6 +16,7 @@
>>   #include "hw/ide.h"
>>   #include "hw/timer/i8254.h"
>>   #include "hw/char/serial.h"
>> +#include "hw/boards.h"
>>   #include "hppa_sys.h"
>>   #include "qemu/units.h"
>>   #include "qapi/error.h"
>> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine)
>>       MemoryRegion *ram_region;
>>       MemoryRegion *cpu_region;
>>       long i;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> I'd prefer to replace smp_cpus with machine->topo.smp_cpus
> directly at places it's used, as it makes affected sites
> more visible in the patch.
> And use local smp_cpus only in places where using machine->topo.smp_cpus
> makes core less readable.
> (but it's just personal preference so I don't insist on it)
> 
>>   
>>       ram_size = machine->ram_size;
>>   
>> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine)
>>   
>>   static void hppa_machine_reset(void)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>       int i;
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> 
> ***)
> It would be better to pass MachineState as argument to
> hppa_machine_reset(), a patch to so should go before this one.
> 
> Quick look shows only 3 overrides (hppa, pc, pnv) and one caller,
> so I'd rather fix it than calling qdev_get_machine() unnecessarily
> 
>>   
>>       qemu_devices_reset();
>>   
>> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
>> index e5bab3c..7752c10 100644
>> --- a/hw/mips/boston.c
>> +++ b/hw/mips/boston.c
>> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine)
>>       DriveInfo *hd[6];
>>       Chardev *chr;
>>       int fw_size, fit_err;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>       bool is_64b;
>>   
>>       if ((machine->ram_size % GiB) ||
>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>> index 439665a..d595375 100644
>> --- a/hw/mips/mips_malta.c
>> +++ b/hw/mips/mips_malta.c
>> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void)
>>   
>>   static void malta_mips_config(MIPSCPU *cpu)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>>       CPUMIPSState *env = &cpu->env;
>>       CPUState *cs = CPU(cpu);
> this one also called from reset, so the same [***] applies here too.
> 
>>   
>> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque)
>>   static void create_cpu_without_cps(const char *cpu_type,
>>                                      qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> caller has an access to MachineState so pass it down call chain all the way
> 
>>       CPUMIPSState *env;
>>       MIPSCPU *cpu;
>>       int i;
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>>   
>>       for (i = 0; i < smp_cpus; i++) {
>>           cpu = MIPS_CPU(cpu_create(cpu_type));
>> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type,
>>   static void create_cps(MaltaState *s, const char *cpu_type,
>>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> ditto
> 
>>       Error *err = NULL;
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>>   
>>       s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS));
>>       qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default());
>> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type,
>>   static void mips_create_cpu(MaltaState *s, const char *cpu_type,
>>                               qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>   {
>> +    MachineState *ms = MACHINE(qdev_get_machine());
> ditto
> 
>> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>> +
>>       if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) {
>>           create_cps(s, cpu_type, cbus_irq, i8259_irq);
>>       } else {
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index ca1e382..2321cfb 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>>       unsigned int num_vsimms;
>>       DeviceState *dev;
>>       SysBusDevice *s;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>> +    unsigned int max_cpus = machine->topo.max_cpus;
>>   
>>       /* init CPUs */
>>       for(i = 0; i < smp_cpus; i++) {
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 399f2d7..d089c4d 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>       NICInfo *nd;
>>       MACAddr macaddr;
>>       bool onboard_nic;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>> +    unsigned int max_cpus = machine->topo.max_cpus;
>>   
>>       /* init CPUs */
>>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
>> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
>> index 12c7437..cc0396b 100644
>> --- a/hw/xtensa/sim.c
>> +++ b/hw/xtensa/sim.c
>> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine)
>>       ram_addr_t ram_size = machine->ram_size;
>>       const char *kernel_filename = machine->kernel_filename;
>>       int n;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>   
>>       for (n = 0; n < smp_cpus; n++) {
>>           cpu = XTENSA_CPU(cpu_create(machine->cpu_type));
>> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
>> index e05ef75..f71a15e 100644
>> --- a/hw/xtensa/xtfpga.c
>> +++ b/hw/xtensa/xtfpga.c
>> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>>       const unsigned system_io_size = 224 * MiB;
>>       uint32_t freq = 10000000;
>>       int n;
>> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>>   
>>       if (smp_cpus > 1) {
>>           mx_pic = xtensa_mx_pic_init(31);
> 
> 

Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties

[Igor Mammedov]

On Tue, 16 Apr 2019 16:47:55 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> On 2019/4/8 20:54, Igor Mammedov wrote:
> > On Fri, 29 Mar 2019 16:48:39 +0800
> > Like Xu <like.xu@linux.intel.com> wrote:
> > 
> > here should be a commit message explaining what patch does
> > in more detail.
> > 
> >   
> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>  
> > 
> > Generic note, try not call qdev_get_machine() every time
> > you replace smp_cpus or other variables. It's often possible
> > to pass MachineState via call chain with trivial fixups.  
> 
> Hi Igor,
> 
> I have some doubts on this comments after some attempts.
> 
> I'm not sure if this idea could apply to all qdev_get_machine()
> usages in tree or just for this smp-touch-only patch.
Ideally it would apply to all users, but realistically it's
probably too much of refactoring for one series.
So whether we use qdev_get_machine() or not depends on
concrete usecase and how complex 'right' refactoring would be.

> It takes a lot of efforts on hooks overrides when we
> undo calls to qdev_get_machine() with modification of incoming parameters.
sometime it's too much of efforts and sometimes efforts are
justified if it makes code more robust and clear.
What I've seen in this series it's mostly trivial replacement
of -smp related globals with qdev_get_machine() without
looking up the call stack to see if Machine is already available
to the caller and just a several call frames away.

In case of this concrete patch wrt reset hook it's logical
to pass Machine as argument to the caller as I commented earlier
and it's not that complex change.

> The implementation of qdev_get_machine() couldn't be simpler more
> and it doesn't seem to bring much overhead compared with parameter stack.
It's not only about overhead, basically with qdev_get_machine()
you are replacing one global variable with another wrapped in
function call. That however introduces implicit dependency on
machine and the order it's initialized, which makes code a bit
more fragile and it's hard to review.

Or putting it in the form of question: what the reason for
replacing one global with another and why we are getting rid of
globals in the first place?

   
> >> ---
> >>   hw/alpha/dp264.c     | 1 +
> >>   hw/hppa/machine.c    | 4 ++++
> >>   hw/mips/boston.c     | 1 +
> >>   hw/mips/mips_malta.c | 9 +++++++++
> >>   hw/sparc/sun4m.c     | 2 ++
> >>   hw/sparc64/sun4u.c   | 2 ++
> >>   hw/xtensa/sim.c      | 1 +
> >>   hw/xtensa/xtfpga.c   | 1 +
> >>   8 files changed, 21 insertions(+)
> >>
> >> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> >> index 0347eb8..ee5d432 100644
> >> --- a/hw/alpha/dp264.c
> >> +++ b/hw/alpha/dp264.c
> >> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine)
> >>       char *palcode_filename;
> >>       uint64_t palcode_entry, palcode_low, palcode_high;
> >>       uint64_t kernel_entry, kernel_low, kernel_high;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>   
> >>       /* Create up to 4 cpus.  */
> >>       memset(cpus, 0, sizeof(cpus));
> >> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> >> index d1b1d3c..f652891 100644
> >> --- a/hw/hppa/machine.c
> >> +++ b/hw/hppa/machine.c
> >> @@ -16,6 +16,7 @@
> >>   #include "hw/ide.h"
> >>   #include "hw/timer/i8254.h"
> >>   #include "hw/char/serial.h"
> >> +#include "hw/boards.h"
> >>   #include "hppa_sys.h"
> >>   #include "qemu/units.h"
> >>   #include "qapi/error.h"
> >> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine)
> >>       MemoryRegion *ram_region;
> >>       MemoryRegion *cpu_region;
> >>       long i;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;  
> > I'd prefer to replace smp_cpus with machine->topo.smp_cpus
> > directly at places it's used, as it makes affected sites
> > more visible in the patch.
> > And use local smp_cpus only in places where using machine->topo.smp_cpus
> > makes core less readable.
> > (but it's just personal preference so I don't insist on it)
> >   
> >>   
> >>       ram_size = machine->ram_size;
> >>   
> >> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine)
> >>   
> >>   static void hppa_machine_reset(void)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());
> >>       int i;
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;  
> > 
> > ***)
> > It would be better to pass MachineState as argument to
> > hppa_machine_reset(), a patch to so should go before this one.
> > 
> > Quick look shows only 3 overrides (hppa, pc, pnv) and one caller,
> > so I'd rather fix it than calling qdev_get_machine() unnecessarily
> >   
> >>   
> >>       qemu_devices_reset();
> >>   
> >> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> >> index e5bab3c..7752c10 100644
> >> --- a/hw/mips/boston.c
> >> +++ b/hw/mips/boston.c
> >> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine)
> >>       DriveInfo *hd[6];
> >>       Chardev *chr;
> >>       int fw_size, fit_err;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>       bool is_64b;
> >>   
> >>       if ((machine->ram_size % GiB) ||
> >> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> >> index 439665a..d595375 100644
> >> --- a/hw/mips/mips_malta.c
> >> +++ b/hw/mips/mips_malta.c
> >> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void)
> >>   
> >>   static void malta_mips_config(MIPSCPU *cpu)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >>       CPUMIPSState *env = &cpu->env;
> >>       CPUState *cs = CPU(cpu);  
> > this one also called from reset, so the same [***] applies here too.
> >   
> >>   
> >> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque)
> >>   static void create_cpu_without_cps(const char *cpu_type,
> >>                                      qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());  
> > caller has an access to MachineState so pass it down call chain all the way
> >   
> >>       CPUMIPSState *env;
> >>       MIPSCPU *cpu;
> >>       int i;
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >>   
> >>       for (i = 0; i < smp_cpus; i++) {
> >>           cpu = MIPS_CPU(cpu_create(cpu_type));
> >> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type,
> >>   static void create_cps(MaltaState *s, const char *cpu_type,
> >>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());  
> > ditto
> >   
> >>       Error *err = NULL;
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >>   
> >>       s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS));
> >>       qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default());
> >> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type,
> >>   static void mips_create_cpu(MaltaState *s, const char *cpu_type,
> >>                               qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());  
> > ditto
> >   
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >> +
> >>       if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) {
> >>           create_cps(s, cpu_type, cbus_irq, i8259_irq);
> >>       } else {
> >> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> >> index ca1e382..2321cfb 100644
> >> --- a/hw/sparc/sun4m.c
> >> +++ b/hw/sparc/sun4m.c
> >> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
> >>       unsigned int num_vsimms;
> >>       DeviceState *dev;
> >>       SysBusDevice *s;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >> +    unsigned int max_cpus = machine->topo.max_cpus;
> >>   
> >>       /* init CPUs */
> >>       for(i = 0; i < smp_cpus; i++) {
> >> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> >> index 399f2d7..d089c4d 100644
> >> --- a/hw/sparc64/sun4u.c
> >> +++ b/hw/sparc64/sun4u.c
> >> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> >>       NICInfo *nd;
> >>       MACAddr macaddr;
> >>       bool onboard_nic;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >> +    unsigned int max_cpus = machine->topo.max_cpus;
> >>   
> >>       /* init CPUs */
> >>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
> >> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> >> index 12c7437..cc0396b 100644
> >> --- a/hw/xtensa/sim.c
> >> +++ b/hw/xtensa/sim.c
> >> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine)
> >>       ram_addr_t ram_size = machine->ram_size;
> >>       const char *kernel_filename = machine->kernel_filename;
> >>       int n;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>   
> >>       for (n = 0; n < smp_cpus; n++) {
> >>           cpu = XTENSA_CPU(cpu_create(machine->cpu_type));
> >> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> >> index e05ef75..f71a15e 100644
> >> --- a/hw/xtensa/xtfpga.c
> >> +++ b/hw/xtensa/xtfpga.c
> >> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
> >>       const unsigned system_io_size = 224 * MiB;
> >>       uint32_t freq = 10000000;
> >>       int n;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>   
> >>       if (smp_cpus > 1) {
> >>           mx_pic = xtensa_mx_pic_init(31);  
> > 
> >   
> 

Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties

[Igor Mammedov]

On Tue, 16 Apr 2019 16:47:55 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> On 2019/4/8 20:54, Igor Mammedov wrote:
> > On Fri, 29 Mar 2019 16:48:39 +0800
> > Like Xu <like.xu@linux.intel.com> wrote:
> > 
> > here should be a commit message explaining what patch does
> > in more detail.
> > 
> >   
> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>  
> > 
> > Generic note, try not call qdev_get_machine() every time
> > you replace smp_cpus or other variables. It's often possible
> > to pass MachineState via call chain with trivial fixups.  
> 
> Hi Igor,
> 
> I have some doubts on this comments after some attempts.
> 
> I'm not sure if this idea could apply to all qdev_get_machine()
> usages in tree or just for this smp-touch-only patch.
Ideally it would apply to all users, but realistically it's
probably too much of refactoring for one series.
So whether we use qdev_get_machine() or not depends on
concrete usecase and how complex 'right' refactoring would be.

> It takes a lot of efforts on hooks overrides when we
> undo calls to qdev_get_machine() with modification of incoming parameters.
sometime it's too much of efforts and sometimes efforts are
justified if it makes code more robust and clear.
What I've seen in this series it's mostly trivial replacement
of -smp related globals with qdev_get_machine() without
looking up the call stack to see if Machine is already available
to the caller and just a several call frames away.

In case of this concrete patch wrt reset hook it's logical
to pass Machine as argument to the caller as I commented earlier
and it's not that complex change.

> The implementation of qdev_get_machine() couldn't be simpler more
> and it doesn't seem to bring much overhead compared with parameter stack.
It's not only about overhead, basically with qdev_get_machine()
you are replacing one global variable with another wrapped in
function call. That however introduces implicit dependency on
machine and the order it's initialized, which makes code a bit
more fragile and it's hard to review.

Or putting it in the form of question: what the reason for
replacing one global with another and why we are getting rid of
globals in the first place?

   
> >> ---
> >>   hw/alpha/dp264.c     | 1 +
> >>   hw/hppa/machine.c    | 4 ++++
> >>   hw/mips/boston.c     | 1 +
> >>   hw/mips/mips_malta.c | 9 +++++++++
> >>   hw/sparc/sun4m.c     | 2 ++
> >>   hw/sparc64/sun4u.c   | 2 ++
> >>   hw/xtensa/sim.c      | 1 +
> >>   hw/xtensa/xtfpga.c   | 1 +
> >>   8 files changed, 21 insertions(+)
> >>
> >> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> >> index 0347eb8..ee5d432 100644
> >> --- a/hw/alpha/dp264.c
> >> +++ b/hw/alpha/dp264.c
> >> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine)
> >>       char *palcode_filename;
> >>       uint64_t palcode_entry, palcode_low, palcode_high;
> >>       uint64_t kernel_entry, kernel_low, kernel_high;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>   
> >>       /* Create up to 4 cpus.  */
> >>       memset(cpus, 0, sizeof(cpus));
> >> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> >> index d1b1d3c..f652891 100644
> >> --- a/hw/hppa/machine.c
> >> +++ b/hw/hppa/machine.c
> >> @@ -16,6 +16,7 @@
> >>   #include "hw/ide.h"
> >>   #include "hw/timer/i8254.h"
> >>   #include "hw/char/serial.h"
> >> +#include "hw/boards.h"
> >>   #include "hppa_sys.h"
> >>   #include "qemu/units.h"
> >>   #include "qapi/error.h"
> >> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine)
> >>       MemoryRegion *ram_region;
> >>       MemoryRegion *cpu_region;
> >>       long i;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;  
> > I'd prefer to replace smp_cpus with machine->topo.smp_cpus
> > directly at places it's used, as it makes affected sites
> > more visible in the patch.
> > And use local smp_cpus only in places where using machine->topo.smp_cpus
> > makes core less readable.
> > (but it's just personal preference so I don't insist on it)
> >   
> >>   
> >>       ram_size = machine->ram_size;
> >>   
> >> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine)
> >>   
> >>   static void hppa_machine_reset(void)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());
> >>       int i;
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;  
> > 
> > ***)
> > It would be better to pass MachineState as argument to
> > hppa_machine_reset(), a patch to so should go before this one.
> > 
> > Quick look shows only 3 overrides (hppa, pc, pnv) and one caller,
> > so I'd rather fix it than calling qdev_get_machine() unnecessarily
> >   
> >>   
> >>       qemu_devices_reset();
> >>   
> >> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> >> index e5bab3c..7752c10 100644
> >> --- a/hw/mips/boston.c
> >> +++ b/hw/mips/boston.c
> >> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine)
> >>       DriveInfo *hd[6];
> >>       Chardev *chr;
> >>       int fw_size, fit_err;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>       bool is_64b;
> >>   
> >>       if ((machine->ram_size % GiB) ||
> >> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> >> index 439665a..d595375 100644
> >> --- a/hw/mips/mips_malta.c
> >> +++ b/hw/mips/mips_malta.c
> >> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void)
> >>   
> >>   static void malta_mips_config(MIPSCPU *cpu)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >>       CPUMIPSState *env = &cpu->env;
> >>       CPUState *cs = CPU(cpu);  
> > this one also called from reset, so the same [***] applies here too.
> >   
> >>   
> >> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque)
> >>   static void create_cpu_without_cps(const char *cpu_type,
> >>                                      qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());  
> > caller has an access to MachineState so pass it down call chain all the way
> >   
> >>       CPUMIPSState *env;
> >>       MIPSCPU *cpu;
> >>       int i;
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >>   
> >>       for (i = 0; i < smp_cpus; i++) {
> >>           cpu = MIPS_CPU(cpu_create(cpu_type));
> >> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type,
> >>   static void create_cps(MaltaState *s, const char *cpu_type,
> >>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());  
> > ditto
> >   
> >>       Error *err = NULL;
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >>   
> >>       s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS));
> >>       qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default());
> >> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type,
> >>   static void mips_create_cpu(MaltaState *s, const char *cpu_type,
> >>                               qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> >>   {
> >> +    MachineState *ms = MACHINE(qdev_get_machine());  
> > ditto
> >   
> >> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> >> +
> >>       if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) {
> >>           create_cps(s, cpu_type, cbus_irq, i8259_irq);
> >>       } else {
> >> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> >> index ca1e382..2321cfb 100644
> >> --- a/hw/sparc/sun4m.c
> >> +++ b/hw/sparc/sun4m.c
> >> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
> >>       unsigned int num_vsimms;
> >>       DeviceState *dev;
> >>       SysBusDevice *s;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >> +    unsigned int max_cpus = machine->topo.max_cpus;
> >>   
> >>       /* init CPUs */
> >>       for(i = 0; i < smp_cpus; i++) {
> >> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> >> index 399f2d7..d089c4d 100644
> >> --- a/hw/sparc64/sun4u.c
> >> +++ b/hw/sparc64/sun4u.c
> >> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> >>       NICInfo *nd;
> >>       MACAddr macaddr;
> >>       bool onboard_nic;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >> +    unsigned int max_cpus = machine->topo.max_cpus;
> >>   
> >>       /* init CPUs */
> >>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
> >> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> >> index 12c7437..cc0396b 100644
> >> --- a/hw/xtensa/sim.c
> >> +++ b/hw/xtensa/sim.c
> >> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine)
> >>       ram_addr_t ram_size = machine->ram_size;
> >>       const char *kernel_filename = machine->kernel_filename;
> >>       int n;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>   
> >>       for (n = 0; n < smp_cpus; n++) {
> >>           cpu = XTENSA_CPU(cpu_create(machine->cpu_type));
> >> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> >> index e05ef75..f71a15e 100644
> >> --- a/hw/xtensa/xtfpga.c
> >> +++ b/hw/xtensa/xtfpga.c
> >> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
> >>       const unsigned system_io_size = 224 * MiB;
> >>       uint32_t freq = 10000000;
> >>       int n;
> >> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> >>   
> >>       if (smp_cpus > 1) {
> >>           mx_pic = xtensa_mx_pic_init(31);  
> > 
> >   
> 

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Eduardo Habkost]

On Mon, Apr 01, 2019 at 10:56:30AM +0800, Like Xu wrote:
> On 2019/3/29 17:27, Alex Bennée wrote:
[...]
> > > @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
> > >   #ifndef CONFIG_USER_ONLY
> > >   static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> > >   {
> > > +    MachineState *ms = MACHINE(qdev_get_machine());
> > 
> > How expensive is qdev_get_machine? This could potentially be a
> > performance issue if this register is read a lot.
> 
> It may not bother us cause the MachineState object would be feteched from
> qemu QOM at its first requested time and save it as static variable for
> incoming requests.

We already have a current_machine variable declared in
include/hw/boards.h.  We normally avoid using it and use
MACHINE(qdev_get_machine()) instead, but I never understood why.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Like Xu]

On 2019/4/2 7:38, Eduardo Habkost wrote:
> On Mon, Apr 01, 2019 at 10:56:30AM +0800, Like Xu wrote:
>> On 2019/3/29 17:27, Alex Bennée wrote:
> [...]
>>>> @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
>>>>    #ifndef CONFIG_USER_ONLY
>>>>    static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>>>    {
>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>
>>> How expensive is qdev_get_machine? This could potentially be a
>>> performance issue if this register is read a lot.
>>
>> It may not bother us cause the MachineState object would be feteched from
>> qemu QOM at its first requested time and save it as static variable for
>> incoming requests.
> 
> We already have a current_machine variable declared in
> include/hw/boards.h.  We normally avoid using it and use
> MACHINE(qdev_get_machine()) instead, but I never understood why.
>
Which one do you prefer to access smp machine properties?

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Peter Maydell]

On Tue, 2 Apr 2019 at 09:46, Like Xu <like.xu@linux.intel.com> wrote:
>
> On 2019/4/2 7:38, Eduardo Habkost wrote:
> > On Mon, Apr 01, 2019 at 10:56:30AM +0800, Like Xu wrote:
> >> On 2019/3/29 17:27, Alex Bennée wrote:
> > [...]
> >>>> @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
> >>>>    #ifndef CONFIG_USER_ONLY
> >>>>    static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> >>>>    {
> >>>> +    MachineState *ms = MACHINE(qdev_get_machine());
> >>>
> >>> How expensive is qdev_get_machine? This could potentially be a
> >>> performance issue if this register is read a lot.
> >>
> >> It may not bother us cause the MachineState object would be feteched from
> >> qemu QOM at its first requested time and save it as static variable for
> >> incoming requests.
> >
> > We already have a current_machine variable declared in
> > include/hw/boards.h.  We normally avoid using it and use
> > MACHINE(qdev_get_machine()) instead, but I never understood why.
> >
> Which one do you prefer to access smp machine properties?

My suggestion would be that we use qdev_get_machine(). I think
it would be nice to make the remaining dozen or so uses of
the global current_machine outside vl.c use qdev_get_machine()
instead, and then make current_machine local to vl.c instead
of global.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Like Xu]

On 2019/4/2 12:45, Peter Maydell wrote:
> On Tue, 2 Apr 2019 at 09:46, Like Xu <like.xu@linux.intel.com> wrote:
>>
>> On 2019/4/2 7:38, Eduardo Habkost wrote:
>>> On Mon, Apr 01, 2019 at 10:56:30AM +0800, Like Xu wrote:
>>>> On 2019/3/29 17:27, Alex Bennée wrote:
>>> [...]
>>>>>> @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
>>>>>>     #ifndef CONFIG_USER_ONLY
>>>>>>     static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>>>>>     {
>>>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>>>
>>>>> How expensive is qdev_get_machine? This could potentially be a
>>>>> performance issue if this register is read a lot.
>>>>
>>>> It may not bother us cause the MachineState object would be feteched from
>>>> qemu QOM at its first requested time and save it as static variable for
>>>> incoming requests.
>>>
>>> We already have a current_machine variable declared in
>>> include/hw/boards.h.  We normally avoid using it and use
>>> MACHINE(qdev_get_machine()) instead, but I never understood why.
>>>
>> Which one do you prefer to access smp machine properties?
> 
> My suggestion would be that we use qdev_get_machine(). I think
> it would be nice to make the remaining dozen or so uses of
> the global current_machine outside vl.c use qdev_get_machine()
> instead, and then make current_machine local to vl.c instead
> of global.
> 
> thanks
> -- PMM
> 
> 
It looks good to me and I am not sure if this refactoring should be 
added in next patch series or as a separate patch.

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Peter Maydell]

On Tue, 2 Apr 2019 at 12:20, Like Xu <like.xu@linux.intel.com> wrote:
>
> On 2019/4/2 12:45, Peter Maydell wrote:
> > My suggestion would be that we use qdev_get_machine(). I think
> > it would be nice to make the remaining dozen or so uses of
> > the global current_machine outside vl.c use qdev_get_machine()
> > instead, and then make current_machine local to vl.c instead
> > of global.

> It looks good to me and I am not sure if this refactoring should be
> added in next patch series or as a separate patch.

Changing the other uses of current_machine is definitely a
refactoring that should be a separate patch series.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Igor Mammedov]

On Fri, 29 Mar 2019 16:48:40 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> ---
>  hw/arm/fsl-imx6.c      | 5 +++++
>  hw/arm/fsl-imx6ul.c    | 5 +++++
>  hw/arm/fsl-imx7.c      | 5 +++++
>  hw/arm/highbank.c      | 1 +
>  hw/arm/mcimx6ul-evk.c  | 1 +
>  hw/arm/mcimx7d-sabre.c | 3 +++
>  hw/arm/raspi.c         | 2 ++
>  hw/arm/realview.c      | 1 +
>  hw/arm/sabrelite.c     | 1 +
>  hw/arm/vexpress.c      | 3 +++
>  hw/arm/virt.c          | 7 +++++++
>  hw/arm/xlnx-zynqmp.c   | 7 +++++++
>  target/arm/cpu.c       | 7 +++++++
>  13 files changed, 48 insertions(+)
> 
> diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
> index 7b7b97f..efed3a4 100644
> --- a/hw/arm/fsl-imx6.c
> +++ b/hw/arm/fsl-imx6.c
> @@ -23,6 +23,7 @@
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "hw/arm/fsl-imx6.h"
> +#include "hw/boards.h"
>  #include "sysemu/sysemu.h"
>  #include "chardev/char.h"
>  #include "qemu/error-report.h"
> @@ -33,9 +34,11 @@
>  
>  static void fsl_imx6_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6State *s = FSL_IMX6(obj);
>      char name[NAME_SIZE];
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      for (i = 0; i < MIN(smp_cpus, FSL_IMX6_NUM_CPUS); i++) {
this is so broken, one creates all cpus here and ...

>          snprintf(name, NAME_SIZE, "cpu%d", i);
> @@ -93,9 +96,11 @@ static void fsl_imx6_init(Object *obj)
>  
>  static void fsl_imx6_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6State *s = FSL_IMX6(dev);
>      uint16_t i;
>      Error *err = NULL;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (smp_cpus > FSL_IMX6_NUM_CPUS) {
>          error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
than checks for validity later.

(***)
Replacing smp_cpus with another global var in the form qdev_get_machine()
ain't much of improvement.
I suggest adding smp_cpus property here/set it from caller
and then move initializing cpus to realize time after this check.

The same same applies to other similar cases below.

> diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
> index 4b56bfa..426bf8e 100644
> --- a/hw/arm/fsl-imx6ul.c
> +++ b/hw/arm/fsl-imx6ul.c
> @@ -23,14 +23,17 @@
>  #include "hw/misc/unimp.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "hw/boards.h"
>  
>  #define NAME_SIZE 20
>  
>  static void fsl_imx6ul_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6ULState *s = FSL_IMX6UL(obj);
>      char name[NAME_SIZE];
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      for (i = 0; i < MIN(smp_cpus, FSL_IMX6UL_NUM_CPUS); i++) {
>          snprintf(name, NAME_SIZE, "cpu%d", i);
> @@ -156,10 +159,12 @@ static void fsl_imx6ul_init(Object *obj)
>  
>  static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6ULState *s = FSL_IMX6UL(dev);
>      int i;
>      qemu_irq irq;
>      char name[NAME_SIZE];
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (smp_cpus > FSL_IMX6UL_NUM_CPUS) {
>          error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> index 7663ad6..b759f7b 100644
> --- a/hw/arm/fsl-imx7.c
> +++ b/hw/arm/fsl-imx7.c
> @@ -25,11 +25,14 @@
>  #include "hw/misc/unimp.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "hw/boards.h"
>  
>  #define NAME_SIZE 20
>  
>  static void fsl_imx7_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      FslIMX7State *s = FSL_IMX7(obj);
>      char name[NAME_SIZE];
>      int i;
> @@ -155,11 +158,13 @@ static void fsl_imx7_init(Object *obj)
>  
>  static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX7State *s = FSL_IMX7(dev);
>      Object *o;
>      int i;
>      qemu_irq irq;
>      char name[NAME_SIZE];
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (smp_cpus > FSL_IMX7_NUM_CPUS) {
>          error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 96ccf18..58c77f6 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -240,6 +240,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>      SysBusDevice *busdev;
>      qemu_irq pic[128];
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>      qemu_irq cpu_irq[4];
>      qemu_irq cpu_fiq[4];
>      qemu_irq cpu_virq[4];
> diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
> index fb2b015..29fb706 100644
> --- a/hw/arm/mcimx6ul-evk.c
> +++ b/hw/arm/mcimx6ul-evk.c
> @@ -29,6 +29,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
>      static struct arm_boot_info boot_info;
>      MCIMX6ULEVK *s = g_new0(MCIMX6ULEVK, 1);
>      int i;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      if (machine->ram_size > FSL_IMX6UL_MMDC_SIZE) {
>          error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
> index 9c5f0e7..c2cdaf9 100644
> --- a/hw/arm/mcimx7d-sabre.c
> +++ b/hw/arm/mcimx7d-sabre.c
> @@ -20,6 +20,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/qtest.h"
> +#include "hw/boards.h"
>  
>  typedef struct {
>      FslIMX7State soc;
> @@ -28,10 +29,12 @@ typedef struct {
>  
>  static void mcimx7d_sabre_init(MachineState *machine)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      static struct arm_boot_info boot_info;
>      MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
>      Object *soc;
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
>          error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 66899c2..8ab8e10 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -113,6 +113,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>  {
>      static struct arm_boot_info binfo;
>      int r;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      binfo.board_id = raspi_boardid[version];
>      binfo.ram_size = ram_size;
> @@ -174,6 +175,7 @@ static void raspi_init(MachineState *machine, int version)
>      BlockBackend *blk;
>      BusState *bus;
>      DeviceState *carddev;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      object_initialize(&s->soc, sizeof(s->soc),
>                        version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 242f5a8..517c275 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -69,6 +69,7 @@ static void realview_init(MachineState *machine,
>      NICInfo *nd;
>      I2CBus *i2c;
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>      int done_nic = 0;
>      qemu_irq cpu_irq[4];
>      int is_mpcore = 0;
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index ee140e5..108faab 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -47,6 +47,7 @@ static void sabrelite_init(MachineState *machine)
>  {
>      IMX6Sabrelite *s = g_new0(IMX6Sabrelite, 1);
>      Error *err = NULL;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      /* Check the amount of memory is compatible with the SOC */
>      if (machine->ram_size > FSL_IMX6_MMDC_SIZE) {
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index f07134c..1927d12 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -206,9 +206,11 @@ struct VEDBoardInfo {
>  static void init_cpus(const char *cpu_type, const char *privdev,
>                        hwaddr periphbase, qemu_irq *pic, bool secure, bool virt)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
pass machine as argument instead.

>      DeviceState *dev;
>      SysBusDevice *busdev;
>      int n;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      /* Create the actual CPUs */
>      for (n = 0; n < smp_cpus; n++) {
> @@ -558,6 +560,7 @@ static void vexpress_common_init(MachineState *machine)
>      MemoryRegion *flash0mem;
>      const hwaddr *map = daughterboard->motherboard_map;
>      int i;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      daughterboard->init(vms, machine->ram_size, machine->cpu_type, pic);
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ce2664a..ff1dff3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -555,11 +555,13 @@ static void create_v2m(VirtMachineState *vms, qemu_irq *pic)
>  
>  static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>  {
> +    MachineState *ms = MACHINE(vms);
>      /* We create a standalone GIC */
>      DeviceState *gicdev;
>      SysBusDevice *gicbusdev;
>      const char *gictype;
>      int type = vms->gic_version, i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      uint32_t nb_redist_regions = 0;
>  
>      gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
> @@ -984,10 +986,12 @@ static void create_flash(const VirtMachineState *vms,
>  
>  static FWCfgState *create_fw_cfg(const VirtMachineState *vms, AddressSpace *as)
>  {
> +    MachineState *ms = MACHINE(vms);
>      hwaddr base = vms->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vms->memmap[VIRT_FW_CFG].size;
>      FWCfgState *fw_cfg;
>      char *nodename;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
> @@ -1423,6 +1427,8 @@ static void machvirt_init(MachineState *machine)
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>      bool aarch64 = true;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> +    unsigned int max_cpus = machine->topo.max_cpus;
>  
>      /*
>       * In accelerated mode, the memory map is computed earlier in kvm_type()
> @@ -1786,6 +1792,7 @@ static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
>  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>  {
>      int n;
> +    unsigned int max_cpus = ms->topo.max_cpus;
>      VirtMachineState *vms = VIRT_MACHINE(ms);
>  
>      if (ms->possible_cpus) {
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 4f8bc41..29b9427 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -19,6 +19,7 @@
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> +#include "hw/boards.h"
>  #include "hw/arm/xlnx-zynqmp.h"
>  #include "hw/intc/arm_gic_common.h"
>  #include "exec/address-spaces.h"
> @@ -174,8 +175,10 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
>  static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>                                     Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      Error *err = NULL;
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
>  
>      if (num_rpus <= 0) {
> @@ -221,8 +224,10 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>  
>  static void xlnx_zynqmp_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      int num_apus = MIN(smp_cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
>  
>      object_initialize_child(obj, "apu-cluster", &s->apu_cluster,
> @@ -290,10 +295,12 @@ static void xlnx_zynqmp_init(Object *obj)
>  
>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
>      MemoryRegion *system_memory = get_system_memory();
>      uint8_t i;
>      uint64_t ram_size;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      int num_apus = MIN(smp_cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
>      const char *boot_cpu = s->boot_cpu ? s->boot_cpu : "apu-cpu[0]";
>      ram_addr_t ddr_low_size, ddr_high_size;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 4155782..e5d7e3b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -30,6 +30,7 @@
>  #include "hw/qdev-properties.h"
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/loader.h"
> +#include "hw/boards.h"
>  #endif
>  #include "hw/arm/arm.h"
>  #include "sysemu/sysemu.h"
> @@ -1183,6 +1184,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      init_cpreg_list(cpu);
>  
>  #ifndef CONFIG_USER_ONLY
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +
>      if (cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY)) {
>          cs->num_ases = 2;
>  
> @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
>  #ifndef CONFIG_USER_ONLY
>  static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
for both hunks the same as [***]

> +
>      /* Linux wants the number of processors from here.
>       * Might as well set the interrupt-controller bit too.
>       */

Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Igor Mammedov]

On Fri, 29 Mar 2019 16:48:40 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> ---
>  hw/arm/fsl-imx6.c      | 5 +++++
>  hw/arm/fsl-imx6ul.c    | 5 +++++
>  hw/arm/fsl-imx7.c      | 5 +++++
>  hw/arm/highbank.c      | 1 +
>  hw/arm/mcimx6ul-evk.c  | 1 +
>  hw/arm/mcimx7d-sabre.c | 3 +++
>  hw/arm/raspi.c         | 2 ++
>  hw/arm/realview.c      | 1 +
>  hw/arm/sabrelite.c     | 1 +
>  hw/arm/vexpress.c      | 3 +++
>  hw/arm/virt.c          | 7 +++++++
>  hw/arm/xlnx-zynqmp.c   | 7 +++++++
>  target/arm/cpu.c       | 7 +++++++
>  13 files changed, 48 insertions(+)
> 
> diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
> index 7b7b97f..efed3a4 100644
> --- a/hw/arm/fsl-imx6.c
> +++ b/hw/arm/fsl-imx6.c
> @@ -23,6 +23,7 @@
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "hw/arm/fsl-imx6.h"
> +#include "hw/boards.h"
>  #include "sysemu/sysemu.h"
>  #include "chardev/char.h"
>  #include "qemu/error-report.h"
> @@ -33,9 +34,11 @@
>  
>  static void fsl_imx6_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6State *s = FSL_IMX6(obj);
>      char name[NAME_SIZE];
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      for (i = 0; i < MIN(smp_cpus, FSL_IMX6_NUM_CPUS); i++) {
this is so broken, one creates all cpus here and ...

>          snprintf(name, NAME_SIZE, "cpu%d", i);
> @@ -93,9 +96,11 @@ static void fsl_imx6_init(Object *obj)
>  
>  static void fsl_imx6_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6State *s = FSL_IMX6(dev);
>      uint16_t i;
>      Error *err = NULL;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (smp_cpus > FSL_IMX6_NUM_CPUS) {
>          error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
than checks for validity later.

(***)
Replacing smp_cpus with another global var in the form qdev_get_machine()
ain't much of improvement.
I suggest adding smp_cpus property here/set it from caller
and then move initializing cpus to realize time after this check.

The same same applies to other similar cases below.

> diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
> index 4b56bfa..426bf8e 100644
> --- a/hw/arm/fsl-imx6ul.c
> +++ b/hw/arm/fsl-imx6ul.c
> @@ -23,14 +23,17 @@
>  #include "hw/misc/unimp.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "hw/boards.h"
>  
>  #define NAME_SIZE 20
>  
>  static void fsl_imx6ul_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6ULState *s = FSL_IMX6UL(obj);
>      char name[NAME_SIZE];
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      for (i = 0; i < MIN(smp_cpus, FSL_IMX6UL_NUM_CPUS); i++) {
>          snprintf(name, NAME_SIZE, "cpu%d", i);
> @@ -156,10 +159,12 @@ static void fsl_imx6ul_init(Object *obj)
>  
>  static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6ULState *s = FSL_IMX6UL(dev);
>      int i;
>      qemu_irq irq;
>      char name[NAME_SIZE];
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (smp_cpus > FSL_IMX6UL_NUM_CPUS) {
>          error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> index 7663ad6..b759f7b 100644
> --- a/hw/arm/fsl-imx7.c
> +++ b/hw/arm/fsl-imx7.c
> @@ -25,11 +25,14 @@
>  #include "hw/misc/unimp.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
> +#include "hw/boards.h"
>  
>  #define NAME_SIZE 20
>  
>  static void fsl_imx7_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      FslIMX7State *s = FSL_IMX7(obj);
>      char name[NAME_SIZE];
>      int i;
> @@ -155,11 +158,13 @@ static void fsl_imx7_init(Object *obj)
>  
>  static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX7State *s = FSL_IMX7(dev);
>      Object *o;
>      int i;
>      qemu_irq irq;
>      char name[NAME_SIZE];
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (smp_cpus > FSL_IMX7_NUM_CPUS) {
>          error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 96ccf18..58c77f6 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -240,6 +240,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>      SysBusDevice *busdev;
>      qemu_irq pic[128];
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>      qemu_irq cpu_irq[4];
>      qemu_irq cpu_fiq[4];
>      qemu_irq cpu_virq[4];
> diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
> index fb2b015..29fb706 100644
> --- a/hw/arm/mcimx6ul-evk.c
> +++ b/hw/arm/mcimx6ul-evk.c
> @@ -29,6 +29,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
>      static struct arm_boot_info boot_info;
>      MCIMX6ULEVK *s = g_new0(MCIMX6ULEVK, 1);
>      int i;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      if (machine->ram_size > FSL_IMX6UL_MMDC_SIZE) {
>          error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
> index 9c5f0e7..c2cdaf9 100644
> --- a/hw/arm/mcimx7d-sabre.c
> +++ b/hw/arm/mcimx7d-sabre.c
> @@ -20,6 +20,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/qtest.h"
> +#include "hw/boards.h"
>  
>  typedef struct {
>      FslIMX7State soc;
> @@ -28,10 +29,12 @@ typedef struct {
>  
>  static void mcimx7d_sabre_init(MachineState *machine)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      static struct arm_boot_info boot_info;
>      MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
>      Object *soc;
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
>          error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 66899c2..8ab8e10 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -113,6 +113,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>  {
>      static struct arm_boot_info binfo;
>      int r;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      binfo.board_id = raspi_boardid[version];
>      binfo.ram_size = ram_size;
> @@ -174,6 +175,7 @@ static void raspi_init(MachineState *machine, int version)
>      BlockBackend *blk;
>      BusState *bus;
>      DeviceState *carddev;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      object_initialize(&s->soc, sizeof(s->soc),
>                        version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 242f5a8..517c275 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -69,6 +69,7 @@ static void realview_init(MachineState *machine,
>      NICInfo *nd;
>      I2CBus *i2c;
>      int n;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>      int done_nic = 0;
>      qemu_irq cpu_irq[4];
>      int is_mpcore = 0;
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index ee140e5..108faab 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -47,6 +47,7 @@ static void sabrelite_init(MachineState *machine)
>  {
>      IMX6Sabrelite *s = g_new0(IMX6Sabrelite, 1);
>      Error *err = NULL;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      /* Check the amount of memory is compatible with the SOC */
>      if (machine->ram_size > FSL_IMX6_MMDC_SIZE) {
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index f07134c..1927d12 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -206,9 +206,11 @@ struct VEDBoardInfo {
>  static void init_cpus(const char *cpu_type, const char *privdev,
>                        hwaddr periphbase, qemu_irq *pic, bool secure, bool virt)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
pass machine as argument instead.

>      DeviceState *dev;
>      SysBusDevice *busdev;
>      int n;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      /* Create the actual CPUs */
>      for (n = 0; n < smp_cpus; n++) {
> @@ -558,6 +560,7 @@ static void vexpress_common_init(MachineState *machine)
>      MemoryRegion *flash0mem;
>      const hwaddr *map = daughterboard->motherboard_map;
>      int i;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
>  
>      daughterboard->init(vms, machine->ram_size, machine->cpu_type, pic);
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ce2664a..ff1dff3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -555,11 +555,13 @@ static void create_v2m(VirtMachineState *vms, qemu_irq *pic)
>  
>  static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>  {
> +    MachineState *ms = MACHINE(vms);
>      /* We create a standalone GIC */
>      DeviceState *gicdev;
>      SysBusDevice *gicbusdev;
>      const char *gictype;
>      int type = vms->gic_version, i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      uint32_t nb_redist_regions = 0;
>  
>      gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
> @@ -984,10 +986,12 @@ static void create_flash(const VirtMachineState *vms,
>  
>  static FWCfgState *create_fw_cfg(const VirtMachineState *vms, AddressSpace *as)
>  {
> +    MachineState *ms = MACHINE(vms);
>      hwaddr base = vms->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vms->memmap[VIRT_FW_CFG].size;
>      FWCfgState *fw_cfg;
>      char *nodename;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>  
>      fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
> @@ -1423,6 +1427,8 @@ static void machvirt_init(MachineState *machine)
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>      bool aarch64 = true;
> +    unsigned int smp_cpus = machine->topo.smp_cpus;
> +    unsigned int max_cpus = machine->topo.max_cpus;
>  
>      /*
>       * In accelerated mode, the memory map is computed earlier in kvm_type()
> @@ -1786,6 +1792,7 @@ static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
>  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>  {
>      int n;
> +    unsigned int max_cpus = ms->topo.max_cpus;
>      VirtMachineState *vms = VIRT_MACHINE(ms);
>  
>      if (ms->possible_cpus) {
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 4f8bc41..29b9427 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -19,6 +19,7 @@
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> +#include "hw/boards.h"
>  #include "hw/arm/xlnx-zynqmp.h"
>  #include "hw/intc/arm_gic_common.h"
>  #include "exec/address-spaces.h"
> @@ -174,8 +175,10 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
>  static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>                                     Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      Error *err = NULL;
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
>  
>      if (num_rpus <= 0) {
> @@ -221,8 +224,10 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>  
>  static void xlnx_zynqmp_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>      int i;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      int num_apus = MIN(smp_cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
>  
>      object_initialize_child(obj, "apu-cluster", &s->apu_cluster,
> @@ -290,10 +295,12 @@ static void xlnx_zynqmp_init(Object *obj)
>  
>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
>      XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
>      MemoryRegion *system_memory = get_system_memory();
>      uint8_t i;
>      uint64_t ram_size;
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
>      int num_apus = MIN(smp_cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
>      const char *boot_cpu = s->boot_cpu ? s->boot_cpu : "apu-cpu[0]";
>      ram_addr_t ddr_low_size, ddr_high_size;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 4155782..e5d7e3b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -30,6 +30,7 @@
>  #include "hw/qdev-properties.h"
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/loader.h"
> +#include "hw/boards.h"
>  #endif
>  #include "hw/arm/arm.h"
>  #include "sysemu/sysemu.h"
> @@ -1183,6 +1184,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      init_cpreg_list(cpu);
>  
>  #ifndef CONFIG_USER_ONLY
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
> +
>      if (cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY)) {
>          cs->num_ases = 2;
>  
> @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
>  #ifndef CONFIG_USER_ONLY
>  static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->topo.smp_cpus;
for both hunks the same as [***]

> +
>      /* Linux wants the number of processors from here.
>       * Might as well set the interrupt-controller bit too.
>       */