Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-07-30 Thread Igor Mammedov
On Mon, 29 Jul 2013 15:41:57 -0400
Jason J. Herne jjhe...@linux.vnet.ibm.com wrote:

 On 06/08/2013 09:11 PM, Andreas Färber wrote:
if (tcg_enabled()  !inited) {
inited = true;
s390x_translate_init();
}
  +
  +smp_cpus += 1;
  Won't we need some form of locking?
 
  If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
  not just in s390x code.
 
 
 I've redesigned a lot of this to make it simpler and less intrusive. 
 I'm almost ready to post the next revision but I'm hung up on this one 
 thing.
 
 I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn. 
 However this seems to break the user mode target because smp_cpus does 
 not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY 
 statement but it seems to have no effect.  I think the reason for that 
 is because CONFIG_USER_ONLY is added to config-target.h which is not 
 actually generated until after we compile qom/cpu.c.
 
 ...
CCqom/object.o
CCqom/container.o
CCqom/qom-qobject.o
CCqom/cpu.o
CChw/core/qdev.o
CChw/core/qdev-properties.o
CChw/core/irq.o
GEN   s390x-linux-user/config-target.h
CCs390x-linux-user/exec.o
 ...
 
 Is there another place I should put the increment?

Could you just use current number of cpus instead of smp_cpus increment?

 




Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-07-30 Thread Igor Mammedov
On Sun, 09 Jun 2013 03:11:35 +0200
Andreas Färber afaer...@suse.de wrote:

 Am 07.06.2013 19:28, schrieb Jason J. Herne:
  From: Jason J. Herne jjhe...@us.ibm.com
  
  Modify cpu initialization and QOM routines associated with s390-cpu such 
  that
  all cpus on S390 are now created via the QOM device creation code path.
  
  Signed-off-by: Jason J. Herne jjhe...@us.ibm.com
  ---
   hw/s390x/s390-virtio-ccw.c |   15 ++-
   hw/s390x/s390-virtio.c |   25 +
   hw/s390x/s390-virtio.h |2 +-
   include/qapi/qmp/qerror.h  |3 +++
   qdev-monitor.c |   17 +
   target-s390x/cpu.c |   24 ++--
   6 files changed, 58 insertions(+), 28 deletions(-)
  
  diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
  index 70bd858..141adce 100644
  --- a/hw/s390x/s390-virtio-ccw.c
  +++ b/hw/s390x/s390-virtio-ccw.c
  @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
   /* allocate storage keys */
   s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
   
  -/* init CPUs */
  -s390_init_cpus(args-cpu_model);
  +s390_init_ipi_states();
   
  -if (kvm_enabled()) {
  -kvm_s390_enable_css_support(s390_cpu_addr2state(0));
  -}
   /*
* Create virtual css and set it as default so that non mcss-e
* enabled guests only see virtio devices.
  @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
   s390_create_virtio_net(BUS(css_bus), virtio-net-ccw);
   }
   
  +static void ccw_post_cpu_init(void)
  +{
  +if (kvm_enabled()) {
  +kvm_s390_enable_css_support(s390_cpu_addr2state(0));
  +}
  +}
 
 Am I understanding correctly that all this is about differentiating one
 call between the ccw and legacy machines?
 
 Isn't there a machine-init-done Notifier that the ccw machine init could
 register for?
 
 What if CPU 0 were hot-unplugged? Would the capability need to be
 re-enabled or will this remain a one-time task?
 
  +
   static QEMUMachine ccw_machine = {
   .name = s390-ccw-virtio,
   .alias = s390-ccw,
   .desc = VirtIO-ccw based S390 machine,
  +.cpu_device_str = s390-cpu,
 
 TYPE_S390_CPU would be safer than hardcoding, if we need to do this.
Similar change was rejected before for i386 target and as temporary solution
target specific cpu_add hook was added. But do it needed for s390 at all?

if s390 doesn't support legacy -cpu [+-]foo-feature format then
CPU subclasses + properties should do the job, at least it's where we
heading with i386 target.

 
   .init = ccw_init,
  +.post_cpu_init = ccw_post_cpu_init,
   .block_default_type = IF_VIRTIO,
   .no_cdrom = 1,
   .no_floppy = 1,
  diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
  index 4af2d86..069a187 100644
  --- a/hw/s390x/s390-virtio.c
  +++ b/hw/s390x/s390-virtio.c
  @@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
   qdev_init_nofail(dev);
   }
   
  -void s390_init_cpus(const char *cpu_model)
  +void s390_init_ipi_states(void)
   {
   int i;
   
  -if (cpu_model == NULL) {
  -cpu_model = host;
  -}
  -
  -ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
  -
  -for (i = 0; i  smp_cpus; i++) {
  -S390CPU *cpu;
  -CPUState *cs;
  +ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
   
  -cpu = cpu_s390x_init(cpu_model);
  -cs = CPU(cpu);
  -
  -ipi_states[i] = cpu;
  -cs-halted = 1;
  -cpu-env.exception_index = EXCP_HLT;
  -cpu-env.storage_keys = s390_get_storage_keys_p();
  +for (i = 0; i  max_cpus; i++) {
  +ipi_states[i] = NULL;
   }
   }
   
  -
 
 Whitespace change intentional?
 
   void s390_create_virtio_net(BusState *bus, const char *name)
   {
   int i;
  @@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
   /* allocate storage keys */
   s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
   
  -/* init CPUs */
  -s390_init_cpus(args-cpu_model);
  +s390_init_ipi_states();
   
   /* Create VirtIO network adapters */
   s390_create_virtio_net((BusState *)s390_bus, virtio-net-s390);
 
 So effectively you're ripping out support for -cpu arguments and
 assuming that s390-cpu will stay the only available type - when we were
 actually just waiting for you guys to sort out how you want your models
 to be named, which I believe you wanted to coordinate with Linux?
 
 I still don't understand why you want to deviate from all other
 architectures here. -smp N is supposed to create N times -cpu, not N
 times QEMUMachine::cpu_device_str.
 
  diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
  index c1cb042..7b1ef9f 100644
  --- a/hw/s390x/s390-virtio.h
  +++ b/hw/s390x/s390-virtio.h
  @@ -20,7 +20,7 @@
   typedef int (*s390_virtio_fn)(const uint64_t *args);
   void 

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-07-30 Thread Jason J. Herne

On 07/30/2013 03:24 AM, Igor Mammedov wrote:

On Mon, 29 Jul 2013 15:41:57 -0400
Jason J. Herne jjhe...@linux.vnet.ibm.com wrote:


On 06/08/2013 09:11 PM, Andreas Färber wrote:

   if (tcg_enabled()  !inited) {

  inited = true;
  s390x_translate_init();
  }
+
+smp_cpus += 1;

Won't we need some form of locking?

If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
not just in s390x code.



I've redesigned a lot of this to make it simpler and less intrusive.
I'm almost ready to post the next revision but I'm hung up on this one
thing.

I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn.
However this seems to break the user mode target because smp_cpus does
not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY
statement but it seems to have no effect.  I think the reason for that
is because CONFIG_USER_ONLY is added to config-target.h which is not
actually generated until after we compile qom/cpu.c.

...
CCqom/object.o
CCqom/container.o
CCqom/qom-qobject.o
CCqom/cpu.o
CChw/core/qdev.o
CChw/core/qdev-properties.o
CChw/core/irq.o
GEN   s390x-linux-user/config-target.h
CCs390x-linux-user/exec.o
...

Is there another place I should put the increment?


Could you just use current number of cpus instead of smp_cpus increment?



Is there an easier way of getting the count besides this?

int cpu_count = 0;
for (cpu = first_cpu; cpu != NULL; cpu = cpu-next_cpu) {
cpu_count++;
}




--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)




Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-07-30 Thread Igor Mammedov
On Tue, 30 Jul 2013 10:27:26 -0400
Jason J. Herne jjhe...@linux.vnet.ibm.com wrote:

 On 07/30/2013 03:24 AM, Igor Mammedov wrote:
  On Mon, 29 Jul 2013 15:41:57 -0400
  Jason J. Herne jjhe...@linux.vnet.ibm.com wrote:
 
  On 06/08/2013 09:11 PM, Andreas Färber wrote:
 if (tcg_enabled()  !inited) {
inited = true;
s390x_translate_init();
}
  +
  +smp_cpus += 1;
  Won't we need some form of locking?
 
  If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
  not just in s390x code.
 
 
  I've redesigned a lot of this to make it simpler and less intrusive.
  I'm almost ready to post the next revision but I'm hung up on this one
  thing.
 
  I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn.
  However this seems to break the user mode target because smp_cpus does
  not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY
  statement but it seems to have no effect.  I think the reason for that
  is because CONFIG_USER_ONLY is added to config-target.h which is not
  actually generated until after we compile qom/cpu.c.
 
  ...
  CCqom/object.o
  CCqom/container.o
  CCqom/qom-qobject.o
  CCqom/cpu.o
  CChw/core/qdev.o
  CChw/core/qdev-properties.o
  CChw/core/irq.o
  GEN   s390x-linux-user/config-target.h
  CCs390x-linux-user/exec.o
  ...
 
  Is there another place I should put the increment?
 
  Could you just use current number of cpus instead of smp_cpus increment?
 
 
 Is there an easier way of getting the count besides this?
 
 int cpu_count = 0;
 for (cpu = first_cpu; cpu != NULL; cpu = cpu-next_cpu) {
  cpu_count++;
 }

maybe qemu_for_each_cpu(), direct access to first_cpu  co is not encouraged.
 




Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-07-30 Thread Andreas Färber
Am 30.07.2013 16:50, schrieb Igor Mammedov:
 On Tue, 30 Jul 2013 10:27:26 -0400
 Jason J. Herne jjhe...@linux.vnet.ibm.com wrote:
 On 07/30/2013 03:24 AM, Igor Mammedov wrote:
 Is there an easier way of getting the count besides this?

 int cpu_count = 0;
 for (cpu = first_cpu; cpu != NULL; cpu = cpu-next_cpu) {
  cpu_count++;
 }
 
 maybe qemu_for_each_cpu(), direct access to first_cpu  co is not encouraged.

Negative: qemu_for_each_cpu() is discouraged, first_cpu and next_cpu are
okay. I need to send out my patch introducing CPU_FOREACH() macro based
on QTAILQ_FOREACH(), preview on qom-cpu-12 branch.

Markus wanted to rip out qemu_for_each_cpu() - also on that branch - but
mst wanted both to coexist.

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



Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-07-29 Thread Jason J. Herne

On 06/08/2013 09:11 PM, Andreas Färber wrote:

  if (tcg_enabled()  !inited) {
  inited = true;
  s390x_translate_init();
  }
+
+smp_cpus += 1;

Won't we need some form of locking?

If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
not just in s390x code.



I've redesigned a lot of this to make it simpler and less intrusive. 
I'm almost ready to post the next revision but I'm hung up on this one 
thing.


I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn. 
However this seems to break the user mode target because smp_cpus does 
not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY 
statement but it seems to have no effect.  I think the reason for that 
is because CONFIG_USER_ONLY is added to config-target.h which is not 
actually generated until after we compile qom/cpu.c.


...
  CCqom/object.o
  CCqom/container.o
  CCqom/qom-qobject.o
  CCqom/cpu.o
  CChw/core/qdev.o
  CChw/core/qdev-properties.o
  CChw/core/irq.o
  GEN   s390x-linux-user/config-target.h
  CCs390x-linux-user/exec.o
...

Is there another place I should put the increment?


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)




Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-06-10 Thread Cornelia Huck
On Sun, 09 Jun 2013 03:11:35 +0200
Andreas Färber afaer...@suse.de wrote:

 Am 07.06.2013 19:28, schrieb Jason J. Herne:
  From: Jason J. Herne jjhe...@us.ibm.com
  
  Modify cpu initialization and QOM routines associated with s390-cpu such 
  that
  all cpus on S390 are now created via the QOM device creation code path.
  
  Signed-off-by: Jason J. Herne jjhe...@us.ibm.com
  ---
   hw/s390x/s390-virtio-ccw.c |   15 ++-
   hw/s390x/s390-virtio.c |   25 +
   hw/s390x/s390-virtio.h |2 +-
   include/qapi/qmp/qerror.h  |3 +++
   qdev-monitor.c |   17 +
   target-s390x/cpu.c |   24 ++--
   6 files changed, 58 insertions(+), 28 deletions(-)
  
  diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
  index 70bd858..141adce 100644
  --- a/hw/s390x/s390-virtio-ccw.c
  +++ b/hw/s390x/s390-virtio-ccw.c
  @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
   /* allocate storage keys */
   s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
   
  -/* init CPUs */
  -s390_init_cpus(args-cpu_model);
  +s390_init_ipi_states();
   
  -if (kvm_enabled()) {
  -kvm_s390_enable_css_support(s390_cpu_addr2state(0));
  -}
   /*
* Create virtual css and set it as default so that non mcss-e
* enabled guests only see virtio devices.
  @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
   s390_create_virtio_net(BUS(css_bus), virtio-net-ccw);
   }
   
  +static void ccw_post_cpu_init(void)
  +{
  +if (kvm_enabled()) {
  +kvm_s390_enable_css_support(s390_cpu_addr2state(0));
  +}
  +}
 
 Am I understanding correctly that all this is about differentiating one
 call between the ccw and legacy machines?
 
 Isn't there a machine-init-done Notifier that the ccw machine init could
 register for?

I wasn't aware of that, but it looks worth a try.

 
 What if CPU 0 were hot-unplugged? Would the capability need to be
 re-enabled or will this remain a one-time task?

KVM_ENABLE_CAP is a vcpu ioctl, but we use it to enable a machine-wide
capability (which will stay enabled during the lifetime of the
machine). (It probably should be any cpu instead of cpu 0, but
that's probably not the only place doing that.)




Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-06-10 Thread Jason J. Herne

On 06/08/2013 09:11 PM, Andreas Färber wrote:

Am 07.06.2013 19:28, schrieb Jason J. Herne:

From: Jason J. Herne jjhe...@us.ibm.com

Modify cpu initialization and QOM routines associated with s390-cpu such that
all cpus on S390 are now created via the QOM device creation code path.

Signed-off-by: Jason J. Herne jjhe...@us.ibm.com
---
  hw/s390x/s390-virtio-ccw.c |   15 ++-
  hw/s390x/s390-virtio.c |   25 +
  hw/s390x/s390-virtio.h |2 +-
  include/qapi/qmp/qerror.h  |3 +++
  qdev-monitor.c |   17 +
  target-s390x/cpu.c |   24 ++--
  6 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 70bd858..141adce 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
  /* allocate storage keys */
  s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));

-/* init CPUs */
-s390_init_cpus(args-cpu_model);
+s390_init_ipi_states();

-if (kvm_enabled()) {
-kvm_s390_enable_css_support(s390_cpu_addr2state(0));
-}
  /*
   * Create virtual css and set it as default so that non mcss-e
   * enabled guests only see virtio devices.
@@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
  s390_create_virtio_net(BUS(css_bus), virtio-net-ccw);
  }

+static void ccw_post_cpu_init(void)
+{
+if (kvm_enabled()) {
+kvm_s390_enable_css_support(s390_cpu_addr2state(0));
+}
+}


Am I understanding correctly that all this is about differentiating one
call between the ccw and legacy machines?



If you are referring to the post_cpu_init hook that I've added in a 
previous patch, then yes :). I get into more detail in a reply to the 
relevant patch but essentially kvm_s390_enable_css_support() currently 
depends on cpus being initialized.  My patch series moves cpu 
initialization out of machine-init() and places it in main().  This is 
done to allow cpus to be treated more like QOM devices which are all 
created in main().



Isn't there a machine-init-done Notifier that the ccw machine init could
register for?



If there is I am not aware of it.  I will investigate.


What if CPU 0 were hot-unplugged? Would the capability need to be
re-enabled or will this remain a one-time task?



Several places in S390 code refer to cpu 0 specifically.  When unplug is 
implemented we will need to check for and fail an attempt to unplug cpu 
0.  Else: We'll need to remove all dependencies on cpu-0.



+
  static QEMUMachine ccw_machine = {
  .name = s390-ccw-virtio,
  .alias = s390-ccw,
  .desc = VirtIO-ccw based S390 machine,
+.cpu_device_str = s390-cpu,


TYPE_S390_CPU would be safer than hardcoding, if we need to do this.


  .init = ccw_init,
+.post_cpu_init = ccw_post_cpu_init,
  .block_default_type = IF_VIRTIO,
  .no_cdrom = 1,
  .no_floppy = 1,
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 4af2d86..069a187 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
  qdev_init_nofail(dev);
  }

-void s390_init_cpus(const char *cpu_model)
+void s390_init_ipi_states(void)
  {
  int i;

-if (cpu_model == NULL) {
-cpu_model = host;
-}
-
-ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
-
-for (i = 0; i  smp_cpus; i++) {
-S390CPU *cpu;
-CPUState *cs;
+ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);

-cpu = cpu_s390x_init(cpu_model);
-cs = CPU(cpu);
-
-ipi_states[i] = cpu;
-cs-halted = 1;
-cpu-env.exception_index = EXCP_HLT;
-cpu-env.storage_keys = s390_get_storage_keys_p();
+for (i = 0; i  max_cpus; i++) {
+ipi_states[i] = NULL;
  }
  }

-


Whitespace change intentional?


  void s390_create_virtio_net(BusState *bus, const char *name)
  {
  int i;
@@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
  /* allocate storage keys */
  s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));

-/* init CPUs */
-s390_init_cpus(args-cpu_model);
+s390_init_ipi_states();

  /* Create VirtIO network adapters */
  s390_create_virtio_net((BusState *)s390_bus, virtio-net-s390);


So effectively you're ripping out support for -cpu arguments and
assuming that s390-cpu will stay the only available type - when we were
actually just waiting for you guys to sort out how you want your models
to be named, which I believe you wanted to coordinate with Linux?


Removing model support was not my intention.  I'll discuss this with my 
team and ensure we're all on the same page and headed down the right 
path with respect to models.




I still don't understand why you want to deviate from all other

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-06-08 Thread Andreas Färber
Am 07.06.2013 19:28, schrieb Jason J. Herne:
 From: Jason J. Herne jjhe...@us.ibm.com
 
 Modify cpu initialization and QOM routines associated with s390-cpu such that
 all cpus on S390 are now created via the QOM device creation code path.
 
 Signed-off-by: Jason J. Herne jjhe...@us.ibm.com
 ---
  hw/s390x/s390-virtio-ccw.c |   15 ++-
  hw/s390x/s390-virtio.c |   25 +
  hw/s390x/s390-virtio.h |2 +-
  include/qapi/qmp/qerror.h  |3 +++
  qdev-monitor.c |   17 +
  target-s390x/cpu.c |   24 ++--
  6 files changed, 58 insertions(+), 28 deletions(-)
 
 diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
 index 70bd858..141adce 100644
 --- a/hw/s390x/s390-virtio-ccw.c
 +++ b/hw/s390x/s390-virtio-ccw.c
 @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
  /* allocate storage keys */
  s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
  
 -/* init CPUs */
 -s390_init_cpus(args-cpu_model);
 +s390_init_ipi_states();
  
 -if (kvm_enabled()) {
 -kvm_s390_enable_css_support(s390_cpu_addr2state(0));
 -}
  /*
   * Create virtual css and set it as default so that non mcss-e
   * enabled guests only see virtio devices.
 @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
  s390_create_virtio_net(BUS(css_bus), virtio-net-ccw);
  }
  
 +static void ccw_post_cpu_init(void)
 +{
 +if (kvm_enabled()) {
 +kvm_s390_enable_css_support(s390_cpu_addr2state(0));
 +}
 +}

Am I understanding correctly that all this is about differentiating one
call between the ccw and legacy machines?

Isn't there a machine-init-done Notifier that the ccw machine init could
register for?

What if CPU 0 were hot-unplugged? Would the capability need to be
re-enabled or will this remain a one-time task?

 +
  static QEMUMachine ccw_machine = {
  .name = s390-ccw-virtio,
  .alias = s390-ccw,
  .desc = VirtIO-ccw based S390 machine,
 +.cpu_device_str = s390-cpu,

TYPE_S390_CPU would be safer than hardcoding, if we need to do this.

  .init = ccw_init,
 +.post_cpu_init = ccw_post_cpu_init,
  .block_default_type = IF_VIRTIO,
  .no_cdrom = 1,
  .no_floppy = 1,
 diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
 index 4af2d86..069a187 100644
 --- a/hw/s390x/s390-virtio.c
 +++ b/hw/s390x/s390-virtio.c
 @@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
  qdev_init_nofail(dev);
  }
  
 -void s390_init_cpus(const char *cpu_model)
 +void s390_init_ipi_states(void)
  {
  int i;
  
 -if (cpu_model == NULL) {
 -cpu_model = host;
 -}
 -
 -ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
 -
 -for (i = 0; i  smp_cpus; i++) {
 -S390CPU *cpu;
 -CPUState *cs;
 +ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
  
 -cpu = cpu_s390x_init(cpu_model);
 -cs = CPU(cpu);
 -
 -ipi_states[i] = cpu;
 -cs-halted = 1;
 -cpu-env.exception_index = EXCP_HLT;
 -cpu-env.storage_keys = s390_get_storage_keys_p();
 +for (i = 0; i  max_cpus; i++) {
 +ipi_states[i] = NULL;
  }
  }
  
 -

Whitespace change intentional?

  void s390_create_virtio_net(BusState *bus, const char *name)
  {
  int i;
 @@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
  /* allocate storage keys */
  s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
  
 -/* init CPUs */
 -s390_init_cpus(args-cpu_model);
 +s390_init_ipi_states();
  
  /* Create VirtIO network adapters */
  s390_create_virtio_net((BusState *)s390_bus, virtio-net-s390);

So effectively you're ripping out support for -cpu arguments and
assuming that s390-cpu will stay the only available type - when we were
actually just waiting for you guys to sort out how you want your models
to be named, which I believe you wanted to coordinate with Linux?

I still don't understand why you want to deviate from all other
architectures here. -smp N is supposed to create N times -cpu, not N
times QEMUMachine::cpu_device_str.

 diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
 index c1cb042..7b1ef9f 100644
 --- a/hw/s390x/s390-virtio.h
 +++ b/hw/s390x/s390-virtio.h
 @@ -20,7 +20,7 @@
  typedef int (*s390_virtio_fn)(const uint64_t *args);
  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
  
 -void s390_init_cpus(const char *cpu_model);
 +void s390_init_ipi_states(void);
  void s390_init_ipl_dev(const char *kernel_filename,
 const char *kernel_cmdline,
 const char *initrd_filename,
 diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
 index 6c0a18d..6627dc4 100644
 --- a/include/qapi/qmp/qerror.h
 +++ b/include/qapi/qmp/qerror.h
 @@ -162,6 +162,9 @@ void assert_no_error(Error *err);
  

[Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

2013-06-07 Thread Jason J. Herne
From: Jason J. Herne jjhe...@us.ibm.com

Modify cpu initialization and QOM routines associated with s390-cpu such that
all cpus on S390 are now created via the QOM device creation code path.

Signed-off-by: Jason J. Herne jjhe...@us.ibm.com
---
 hw/s390x/s390-virtio-ccw.c |   15 ++-
 hw/s390x/s390-virtio.c |   25 +
 hw/s390x/s390-virtio.h |2 +-
 include/qapi/qmp/qerror.h  |3 +++
 qdev-monitor.c |   17 +
 target-s390x/cpu.c |   24 ++--
 6 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 70bd858..141adce 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
 /* allocate storage keys */
 s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
 
-/* init CPUs */
-s390_init_cpus(args-cpu_model);
+s390_init_ipi_states();
 
-if (kvm_enabled()) {
-kvm_s390_enable_css_support(s390_cpu_addr2state(0));
-}
 /*
  * Create virtual css and set it as default so that non mcss-e
  * enabled guests only see virtio devices.
@@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
 s390_create_virtio_net(BUS(css_bus), virtio-net-ccw);
 }
 
+static void ccw_post_cpu_init(void)
+{
+if (kvm_enabled()) {
+kvm_s390_enable_css_support(s390_cpu_addr2state(0));
+}
+}
+
 static QEMUMachine ccw_machine = {
 .name = s390-ccw-virtio,
 .alias = s390-ccw,
 .desc = VirtIO-ccw based S390 machine,
+.cpu_device_str = s390-cpu,
 .init = ccw_init,
+.post_cpu_init = ccw_post_cpu_init,
 .block_default_type = IF_VIRTIO,
 .no_cdrom = 1,
 .no_floppy = 1,
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 4af2d86..069a187 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
 qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model)
+void s390_init_ipi_states(void)
 {
 int i;
 
-if (cpu_model == NULL) {
-cpu_model = host;
-}
-
-ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
-
-for (i = 0; i  smp_cpus; i++) {
-S390CPU *cpu;
-CPUState *cs;
+ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
 
-cpu = cpu_s390x_init(cpu_model);
-cs = CPU(cpu);
-
-ipi_states[i] = cpu;
-cs-halted = 1;
-cpu-env.exception_index = EXCP_HLT;
-cpu-env.storage_keys = s390_get_storage_keys_p();
+for (i = 0; i  max_cpus; i++) {
+ipi_states[i] = NULL;
 }
 }
 
-
 void s390_create_virtio_net(BusState *bus, const char *name)
 {
 int i;
@@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
 /* allocate storage keys */
 s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
 
-/* init CPUs */
-s390_init_cpus(args-cpu_model);
+s390_init_ipi_states();
 
 /* Create VirtIO network adapters */
 s390_create_virtio_net((BusState *)s390_bus, virtio-net-s390);
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index c1cb042..7b1ef9f 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -20,7 +20,7 @@
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 
-void s390_init_cpus(const char *cpu_model);
+void s390_init_ipi_states(void);
 void s390_init_ipl_dev(const char *kernel_filename,
const char *kernel_cmdline,
const char *initrd_filename,
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6c0a18d..6627dc4 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -162,6 +162,9 @@ void assert_no_error(Error *err);
 #define QERR_KVM_MISSING_CAP \
 ERROR_CLASS_K_V_M_MISSING_CAP, Using KVM without %s, %s unavailable
 
+#define QERR_MAX_CPUS \
+ERROR_CLASS_GENERIC_ERROR, The maximum number of cpus has already been 
created for this guest
+
 #define QERR_MIGRATION_ACTIVE \
 ERROR_CLASS_GENERIC_ERROR, There's a migration process in progress
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e54dbc2..a4adeb8 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -23,6 +23,9 @@
 #include monitor/qdev.h
 #include qmp-commands.h
 #include sysemu/arch_init.h
+#include sysemu/sysemu.h
+#include hw/boards.h
+#include sysemu/cpus.h
 #include qemu/config-file.h
 
 /*
@@ -442,6 +445,14 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 return NULL;
 }
 
+if (driver  current_machine 
+strcmp(driver, current_machine-cpu_device_str) == 0) {
+if (smp_cpus == max_cpus) {
+qerror_report(QERR_MAX_CPUS);
+return NULL;
+}
+}
+
 k = DEVICE_CLASS(obj);
 
 /* find bus */
@@