Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Infrastructure for Cpu Devices

[Andreas Färber]

Hi,

Am 07.06.2013 19:28, schrieb Jason J. Herne:
> From: "Jason J. Herne" <address@hidden>
> 
> Add infrastructure for treating cpus as devices. This patch allows cpus to be
> specified using a combination of '-smp' and '-device cpu'.  This approach
> forces a change in the way cpus are counted via smp_cpus.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> ---
>  include/hw/boards.h |    3 ++
>  vl.c                |   95 
> +++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 84 insertions(+), 14 deletions(-)

This feels like an ugly hack. And nack to CPUS_ARE_DEVICES(), since all
CPUs are devices already.

Could you please give a bit more rationale for each change? What's the
intended use case here? This is not just an s390 change but a design
question, so again please CC me as CPU maintainer.

There's a number of questions that your series touches on but doesn't
discuss the concept in the cover letter or in the commit messages:

1) Mixing -smp N and -device s390-cpu

I would expect to get N+1 CPUs. Do you agree or disagree?

-smp 0 is probably not well tested, if at all, so why specify -device
s390-cpu on the command line at all? Of course if there's bugs I'll be
happy to accept fixes, but I'm seeing device_add as more relevant than
-device honestly.

2) QEMUMachine::max_cpus

I believe -device s390-cpu should honor the limit, do you agree?
If so, then there's no need to iterate over -device options, because
that'll miss hot-added devices, but instead move any checks to the CPU's
realize function. If a user creates more CPUs than
qom/cpu.c:cpu_common_realizefn() should be made to fail. Note that my
upcoming CPUState series moves qemu_init_vcpu() there, so we will be
able to bail out before creating vCPU threads etc. for that CPU.

3) vCPU initialization hooks

We already have a QEMUMachine::hot_add_cpu hook. If you need additional
hooks, I'd like to first understand why that cannot go into S390CPU's
realize function, and then we can think about a more generic solution
like adding a Notifier that anyone can use.

Regards,
Andreas

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