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

[Jason J. Herne]

On 06/08/2013 06:50 PM, Andreas Färber wrote:
> 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.

As per upstream request from the last S390 cpu hotplug submission, the 
end goal here is cpu hotplug capability using the QOM infrastructure.
This means  no new "cpu_add" command. Instead make hotplug work through 
device_add.  All cpu creation/initialization details should be handled 
through QOM object construction routines.

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

I agree. This patch has the desired outcome here.

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

I agree here as well.  but considering the parsing of the command line 
-device and the device_add qmp/hmp command are common I see no easy way 
of enabling hotpluging via device_add bit rejecting the -device command 
line form.  I suppose we could explicitly check for "-device s390-cpu" 
in main() and fail with an error message. But then we need to add a new 
one for each architecture?

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

I agree.  -device should honor max_cpus.  This patch does exactly that. 
 The check during a cpu hotplug happens in qdev_device_add.  I was not 
aware that this could be done in qom/cpu.c:cpu_common_realizefn(). 
Sounds like a good place to put it if we can effectively use that one 
check to remove both the vl.c check and the qdev_device_add.  I'll 
investigate that.

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

You are again talking about the new post_cpu_init() hook I added?  If 
so, I've addressed that concern in my reply to your last set of 
comments.  If not, can you please elaborate?

The hot_add_cpu hook is called by qmp_cpu_add.  I would need to call it 
from qmp_device_add.  In which case, I still have no indication if we're 
truly hot adding or if we're still in pre-boot guest initialization.  Is 
there a way to tell?

I guess we should step back and look at the "end goal" here.  I was 
attempting to make the cpu initialization and hot plug as similar as 
possible.  It seems as though that is required if we're going to be 
using the same basic code paths (through QOM object creation routines) 
for both cases.  Am I way off track here? Should this not be my goal?

> Regards,
> Andreas


--
-- Jason J. Herne (address@hidden)