Re: [Qemu-devel] [PATCH v5 6/6] s390x/cpu: Allow hotplug of CPUs

From: Igor Mammedov <imammedo@redhat.com>
To: Matthew Rosato <mjrosato@linux.vnet.ibm.com>, afaerber@suse.de
Cc: dahi@linux.vnet.ibm.com, qemu-devel@nongnu.org, agraf@suse.de,
	borntraeger@de.ibm.com, bharata@linux.vnet.ibm.com,
	cornelia.huck@de.ibm.com, pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [Qemu-devel] [PATCH v5 6/6] s390x/cpu: Allow hotplug of CPUs
Date: Fri, 19 Feb 2016 17:18:27 +0100	[thread overview]
Message-ID: <20160219171827.340ff471@nial.brq.redhat.com> (raw)
In-Reply-To: <56C7294D.6090506@linux.vnet.ibm.com>

On Fri, 19 Feb 2016 09:40:13 -0500
Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> On 02/19/2016 09:28 AM, Igor Mammedov wrote:
> > On Thu, 18 Feb 2016 23:03:37 -0500
> > Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> >   
> >> Implement cpu hotplug routine and add the machine hook.
> >>
> >> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> >> ---
> >>  hw/s390x/s390-virtio-ccw.c |  1 +
> >>  target-s390x/cpu.c         | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  target-s390x/cpu.h         |  1 +
> >>  3 files changed, 47 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 31c1082..3be41ee 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -199,6 +199,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
> >>  
> >>      mc->init = ccw_init;
> >>      mc->reset = s390_machine_reset;
> >> +    mc->hot_add_cpu = s390_hot_add_cpu;
> >>      mc->block_default_type = IF_VIRTIO;
> >>      mc->no_cdrom = 1;
> >>      mc->no_floppy = 1;
> >> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> >> index 8dfd063..99394ec 100644
> >> --- a/target-s390x/cpu.c
> >> +++ b/target-s390x/cpu.c
> >> @@ -32,6 +32,12 @@
> >>  #include "trace.h"
> >>  #ifndef CONFIG_USER_ONLY
> >>  #include "sysemu/arch_init.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "hw/boards.h"
> >> +#include "hw/s390x/sclp.h"
> >> +#include "qom/cpu.h"
> >> +
> >> +#define last_cpu QTAILQ_LAST(&cpus, CPUTailQ)  
> > this should be in qom/cpu.h, maybe a separate patch 
> >   
> 
> I did that originally, but trips up due to usage of the phrase
> 'last_cpu' elsewhere in the code.  Looking now, hw/intc/openpic.c is the
> culprit.
> 
> I could split this out into a patch that adds last_cpu to qom/cpu.h +
> changes the variable name in openpic.c.
I'd change variable name in openpic.c.

> 
> Or use a different name for the macro.  (cpu_last for example).
> 
> Preference?
typically macros should be UPPER_CASED, I guess first_cpu is lower
cased due historical reasons to avoid renaming it across a bunch of files.

Andreas,
do you have any preference in which way fix last_cpu issue?

> 
> >>  #endif
> >>  
> >>  #define CR0_RESET       0xE0UL
> >> @@ -211,6 +217,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
> >>  #endif
> >>  
> >>      scc->parent_realize(dev, errp);
> >> +
> >> +#if !defined(CONFIG_USER_ONLY)
> >> +    if (dev->hotplugged) {
> >> +        raise_irq_cpu_hotplug();
> >> +    }
> >> +#endif
> >>  }
> >>  
> >>  static void s390_cpu_initfn(Object *obj)
> >> @@ -254,6 +266,39 @@ static void s390_cpu_finalize(Object *obj)
> >>  }
> >>  
> >>  #if !defined(CONFIG_USER_ONLY)
> >> +void s390_hot_add_cpu(const int64_t id, Error **errp)
> >> +{  
> > this one looks like a func that belongs to board and should
> > be a static in hw/s390x/s390-virtio-ccw.c
> >   
> 
> OK, sure.
> 
> Matt
> 