[Qemu-devel] [PATCH v2 00/19] s390x cleanups and CPU hotplug via device_add

[Qemu-devel] [PATCH v2 00/19] s390x cleanups and CPU hotplug via device_add

[David Hildenbrand]

The first patches are a bunch of cleanups. I decided to go the
extra mile and implement CPU hotplug via "device_add", as well as
"query-hotpluggable-cpus". So the last patches in this series differ
to v1. I tried to stick as much as possible to x86.

On s390x, only complete cores can be plugged. Cores have to be plugged in
the right order (core-id 0, 1, 2 ...). CPU hot unplug is not supported
by the architecture.

In addition, we now also print the CPU model list in sorted order and allow
only 1 CPU for TCG.

v1 -> v2:
- "exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly"
-- added a few instances I missed
- "s390x: get rid of s390-virtio.c"
-- move everything in the first shot and clean the "cpu_states" stuff up
   in later patches, when we have ms->possible_cpus.
- Added "target/s390x: move typedef of S390CPU to its definition"
- Added "s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h"
- Added "s390x: move two function declarations to s390-virtio-ccw.h"
- Added "s390x: move sclp_service_call() to interrupt.c"
- "target/s390x: use program_interrupt() in per_check_exception()"
-- Add a comment describing my concern
- "s390x: allow only 1 CPU with TCG"
-- Add a #define to easily be able to change it (e.g. for development)
- Added "target/s390x: set cpu->id for linux user when realizing"
- "target/s390x: use "core-id" for cpu number/address/id handling"
-- I now use "core-id" as property name, because that will be used by
   device_add
- "target/s390x: rename next_cpu_id to next_core_id"
-- Also change to the term "core_id"
- Added "s390x: print CPU definitions in sorted order"
- Added "s390x: allow cpu hotplug via device_add"
- Added "s390x: CPU hot unplug via device_del cannot work"
- Added "s390x: implement query-hotpluggable-cpu"
- Added "s390x: get rid of cpu_s390x_create()"

David Hildenbrand (19):
  exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly
  cpu: drop old comments describing members
  s390x: get rid of s390-virtio.c
  s390x: rename s390-virtio.h to s390-virtio-hcall.h
  target/s390x: move typedef of S390CPU to its definition
  s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
  s390x: move two function declarations to s390-virtio-ccw.h
  s390x: move sclp_service_call() to interrupt.c
  target/s390x: use trigger_pgm_exception() in
    s390_cpu_handle_mmu_fault()
  target/s390x: use program_interrupt() in per_check_exception()
  s390x: allow only 1 CPU with TCG
  target/s390x: set cpu->id for linux user when realizing
  target/s390x: use "core-id" for cpu number/address/id handling
  target/s390x: rename next_cpu_id to next_core_id
  s390x: print CPU definitions in sorted order
  s390x: allow cpu hotplug via device_add
  s390x: CPU hot unplug via device_del cannot work
  s390x: implement query-hotpluggable-cpus
  s390x: get rid of cpu_s390x_create()

 dump.c                             |   1 -
 exec.c                             |   2 -
 hw/s390x/Makefile.objs             |   1 -
 hw/s390x/s390-virtio-ccw.c         | 259 +++++++++++++++++++++++++++++++++++--
 hw/s390x/s390-virtio-hcall.c       |   2 +-
 hw/s390x/s390-virtio-hcall.h       |  21 +++
 hw/s390x/s390-virtio.c             |  37 ------
 hw/s390x/s390-virtio.h             |  35 -----
 hw/s390x/sclp.c                    |  51 +-------
 include/hw/s390x/s390-virtio-ccw.h |   3 +
 include/hw/s390x/sclp.h            |   1 +
 include/qom/cpu.h                  |   6 +-
 qapi-schema.json                   |  16 +++
 target/i386/arch_dump.c            |   1 -
 target/i386/arch_memory_mapping.c  |   1 -
 target/i386/svm_helper.c           |   1 -
 target/ppc/arch_dump.c             |   1 -
 target/s390x/arch_dump.c           |   1 -
 target/s390x/cpu-qom.h             |   4 +-
 target/s390x/cpu.c                 |  86 ++++--------
 target/s390x/cpu.h                 |  19 +--
 target/s390x/cpu_models.c          |  65 ++++++----
 target/s390x/diag.c                |   1 +
 target/s390x/excp_helper.c         |   5 +-
 target/s390x/helper.c              |  47 +------
 target/s390x/internal.h            |   1 -
 target/s390x/interrupt.c           |  52 ++++++++
 target/s390x/kvm.c                 |   3 +-
 target/s390x/misc_helper.c         |  22 ++--
 target/s390x/translate.c           |   5 +-
 30 files changed, 443 insertions(+), 307 deletions(-)
 create mode 100644 hw/s390x/s390-virtio-hcall.h
 delete mode 100644 hw/s390x/s390-virtio.h

-- 
2.13.5

[Qemu-devel] [PATCH v2 01/19] exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

[David Hildenbrand]

All but a handfull of filles include exec/cpu-all.h via cpu.h only.
As these files already include cpu.h, let's just drop the additional
include.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 dump.c                            | 1 -
 exec.c                            | 2 --
 target/i386/arch_dump.c           | 1 -
 target/i386/arch_memory_mapping.c | 1 -
 target/i386/svm_helper.c          | 1 -
 target/ppc/arch_dump.c            | 1 -
 target/s390x/arch_dump.c          | 1 -
 7 files changed, 8 deletions(-)

diff --git a/dump.c b/dump.c
index a79773d0f7..2ef6a678e8 100644
--- a/dump.c
+++ b/dump.c
@@ -15,7 +15,6 @@
 #include "qemu/cutils.h"
 #include "elf.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/hwaddr.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"
diff --git a/exec.c b/exec.c
index d20c34ca83..d109de2014 100644
--- a/exec.c
+++ b/exec.c
@@ -23,7 +23,6 @@
 
 #include "qemu/cutils.h"
 #include "cpu.h"
-#include "exec/exec-all.h"
 #include "exec/target_page.h"
 #include "tcg.h"
 #include "hw/qdev-core.h"
@@ -56,7 +55,6 @@
 #endif
 
 #endif
-#include "exec/cpu-all.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
 #include "translate-all.h"
diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
index e682904052..35b55fc200 100644
--- a/target/i386/arch_dump.c
+++ b/target/i386/arch_dump.c
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 #include "elf.h"
 #include "sysemu/memory_mapping.h"
diff --git a/target/i386/arch_memory_mapping.c b/target/i386/arch_memory_mapping.c
index 647cff2829..271cb5e41b 100644
--- a/target/i386/arch_memory_mapping.c
+++ b/target/i386/arch_memory_mapping.c
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "sysemu/memory_mapping.h"
 
 /* PAE Paging or IA-32e Paging */
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index 59e8b5091c..f479239875 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -19,7 +19,6 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
index 8e9397aa58..beb7268e57 100644
--- a/target/ppc/arch_dump.c
+++ b/target/ppc/arch_dump.c
@@ -15,7 +15,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 #include "sysemu/kvm.h"
 
diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 9b0bf92698..6f61ff95af 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -15,7 +15,6 @@
 #include "cpu.h"
 #include "internal.h"
 #include "elf.h"
-#include "exec/cpu-all.h"
 #include "sysemu/dump.h"
 
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 01/19] exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

[Cornelia Huck]

On Mon,  4 Sep 2017 17:42:58 +0200
David Hildenbrand <david@redhat.com> wrote:

> All but a handfull of filles include exec/cpu-all.h via cpu.h only.

You had some extra 'l's around? :)

s/handfull/handful/ and s/filles/files/

(I can fix while applying)

> As these files already include cpu.h, let's just drop the additional
> include.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  dump.c                            | 1 -
>  exec.c                            | 2 --
>  target/i386/arch_dump.c           | 1 -
>  target/i386/arch_memory_mapping.c | 1 -
>  target/i386/svm_helper.c          | 1 -
>  target/ppc/arch_dump.c            | 1 -
>  target/s390x/arch_dump.c          | 1 -
>  7 files changed, 8 deletions(-)

This looks obviously correct; would not mind acks, though.

> 
> diff --git a/dump.c b/dump.c
> index a79773d0f7..2ef6a678e8 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -15,7 +15,6 @@
>  #include "qemu/cutils.h"
>  #include "elf.h"
>  #include "cpu.h"
> -#include "exec/cpu-all.h"
>  #include "exec/hwaddr.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/kvm.h"
> diff --git a/exec.c b/exec.c
> index d20c34ca83..d109de2014 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -23,7 +23,6 @@
>  
>  #include "qemu/cutils.h"
>  #include "cpu.h"
> -#include "exec/exec-all.h"
>  #include "exec/target_page.h"
>  #include "tcg.h"
>  #include "hw/qdev-core.h"
> @@ -56,7 +55,6 @@
>  #endif
>  
>  #endif
> -#include "exec/cpu-all.h"
>  #include "qemu/rcu_queue.h"
>  #include "qemu/main-loop.h"
>  #include "translate-all.h"
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index e682904052..35b55fc200 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -13,7 +13,6 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> -#include "exec/cpu-all.h"
>  #include "sysemu/dump.h"
>  #include "elf.h"
>  #include "sysemu/memory_mapping.h"
> diff --git a/target/i386/arch_memory_mapping.c b/target/i386/arch_memory_mapping.c
> index 647cff2829..271cb5e41b 100644
> --- a/target/i386/arch_memory_mapping.c
> +++ b/target/i386/arch_memory_mapping.c
> @@ -13,7 +13,6 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> -#include "exec/cpu-all.h"
>  #include "sysemu/memory_mapping.h"
>  
>  /* PAE Paging or IA-32e Paging */
> diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
> index 59e8b5091c..f479239875 100644
> --- a/target/i386/svm_helper.c
> +++ b/target/i386/svm_helper.c
> @@ -19,7 +19,6 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> -#include "exec/cpu-all.h"
>  #include "exec/helper-proto.h"
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 8e9397aa58..beb7268e57 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -15,7 +15,6 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "elf.h"
> -#include "exec/cpu-all.h"
>  #include "sysemu/dump.h"
>  #include "sysemu/kvm.h"
>  
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index 9b0bf92698..6f61ff95af 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -15,7 +15,6 @@
>  #include "cpu.h"
>  #include "internal.h"
>  #include "elf.h"
> -#include "exec/cpu-all.h"
>  #include "sysemu/dump.h"
>  
>  

Re: [Qemu-devel] [PATCH v2 01/19] exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

[Thomas Huth]

On 04.09.2017 17:42, David Hildenbrand wrote:
> All but a handfull of filles include exec/cpu-all.h via cpu.h only.
> As these files already include cpu.h, let's just drop the additional
> include.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  dump.c                            | 1 -
>  exec.c                            | 2 --
>  target/i386/arch_dump.c           | 1 -
>  target/i386/arch_memory_mapping.c | 1 -
>  target/i386/svm_helper.c          | 1 -
>  target/ppc/arch_dump.c            | 1 -
>  target/s390x/arch_dump.c          | 1 -
>  7 files changed, 8 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index a79773d0f7..2ef6a678e8 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -15,7 +15,6 @@
>  #include "qemu/cutils.h"
>  #include "elf.h"
>  #include "cpu.h"
> -#include "exec/cpu-all.h"
>  #include "exec/hwaddr.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/kvm.h"
> diff --git a/exec.c b/exec.c
> index d20c34ca83..d109de2014 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -23,7 +23,6 @@
>  
>  #include "qemu/cutils.h"
>  #include "cpu.h"
> -#include "exec/exec-all.h"

You should maybe mention exec-all.h in the patch description, too.
(I think it can be dropped here because it is included by
translate-all.h already).

If you tweak the patch description:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 01/19] exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

[David Hildenbrand]

On 07.09.2017 07:46, Thomas Huth wrote:
>>  #include "qemu/cutils.h"
>>  #include "cpu.h"
>> -#include "exec/exec-all.h"
> You should maybe mention exec-all.h in the patch description, too.
> (I think it can be dropped here because it is included by
> translate-all.h already).
> 
> If you tweak the patch description:
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I'll just drop that hunk although it can also be removed.

Yes, this was not included on purpose - these similar names still
confuse me :)

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 01/19] exec, dump, i386, ppc, s390x: don't include exec/cpu-all.h explicitly

[Cornelia Huck]

On Thu, 7 Sep 2017 14:41:21 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 07.09.2017 07:46, Thomas Huth wrote:
> >>  #include "qemu/cutils.h"
> >>  #include "cpu.h"
> >> -#include "exec/exec-all.h"  
> > You should maybe mention exec-all.h in the patch description, too.
> > (I think it can be dropped here because it is included by
> > translate-all.h already).
> > 
> > If you tweak the patch description:
> > Reviewed-by: Thomas Huth <thuth@redhat.com>  
> 
> I'll just drop that hunk although it can also be removed.
> 
> Yes, this was not included on purpose - these similar names still
> confuse me :)
> 

Not only you :)

If we can do a sweep of exec-all.h as well, it should be a separate
patch.

[Qemu-devel] [PATCH v2 02/19] cpu: drop old comments describing members

[David Hildenbrand]

These comments are obviously stale.

Acked-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qom/cpu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index b7ac9491c8..386ab0af40 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -378,10 +378,10 @@ struct CPUState {
     DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS);
 
     /* TODO Move common fields from CPUArchState here. */
-    int cpu_index; /* used by alpha TCG */
-    uint32_t halted; /* used by alpha, cris, ppc TCG */
+    int cpu_index;
+    uint32_t halted;
     uint32_t can_do_io;
-    int32_t exception_index; /* used by m68k TCG */
+    int32_t exception_index;
 
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;
-- 
2.13.5

[Qemu-devel] [PATCH v2 03/19] s390x: get rid of s390-virtio.c

[David Hildenbrand]

It is a leftover from the days where we had still the !ccw virtio
machine. As this one is long gone, let's move everything to
s390-virtio-ccw.c.

Suggested-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/Makefile.objs     |   1 -
 hw/s390x/s390-virtio-ccw.c | 157 ++++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     |  37 -----------
 hw/s390x/s390-virtio.h     |  15 -----
 4 files changed, 156 insertions(+), 54 deletions(-)

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 7ee19d3abc..dc704b57d6 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -1,4 +1,3 @@
-obj-y += s390-virtio.o
 obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index dd504dd5ae..4c3a3e177b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -2,6 +2,7 @@
  * virtio ccw machine
  *
  * Copyright 2012 IBM Corp.
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or (at
@@ -31,6 +32,46 @@
 #include "hw/s390x/css-bridge.h"
 #include "migration/register.h"
 #include "cpu_models.h"
+#include "qapi/qmp/qerror.h"
+#include "hw/nmi.h"
+
+static S390CPU **cpu_states;
+
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }
+
+    /* Fast lookup via CPU ID */
+    return cpu_states[cpu_addr];
+}
+
+static void s390_init_cpus(MachineState *machine)
+{
+    int i;
+    gchar *name;
+
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = s390_default_cpu_model_name();
+    }
+
+    cpu_states = g_new0(S390CPU *, max_cpus);
+
+    for (i = 0; i < max_cpus; i++) {
+        name = g_strdup_printf("cpu[%i]", i);
+        object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
+                                 (Object **) &cpu_states[i],
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+        g_free(name);
+    }
+
+    for (i = 0; i < smp_cpus; i++) {
+        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
+    }
+}
 
 static const char *const reset_dev_types[] = {
     TYPE_VIRTUAL_CSS_BRIDGE,
@@ -94,7 +135,7 @@ static void virtio_ccw_register_hcalls(void)
                                    virtio_ccw_hcall_early_printk);
 }
 
-void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(ram_addr_t mem_size)
 {
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
@@ -109,11 +150,103 @@ void s390_memory_init(ram_addr_t mem_size)
     s390_stattrib_init();
 }
 
+#define S390_TOD_CLOCK_VALUE_MISSING    0x00
+#define S390_TOD_CLOCK_VALUE_PRESENT    0x01
+
+static void gtod_save(QEMUFile *f, void *opaque)
+{
+    uint64_t tod_low;
+    uint8_t tod_high;
+    int r;
+
+    r = s390_get_clock(&tod_high, &tod_low);
+    if (r) {
+        fprintf(stderr, "WARNING: Unable to get guest clock for migration. "
+                        "Error code %d. Guest clock will not be migrated "
+                        "which could cause the guest to hang.\n", r);
+        qemu_put_byte(f, S390_TOD_CLOCK_VALUE_MISSING);
+        return;
+    }
+
+    qemu_put_byte(f, S390_TOD_CLOCK_VALUE_PRESENT);
+    qemu_put_byte(f, tod_high);
+    qemu_put_be64(f, tod_low);
+}
+
+static int gtod_load(QEMUFile *f, void *opaque, int version_id)
+{
+    uint64_t tod_low;
+    uint8_t tod_high;
+    int r;
+
+    if (qemu_get_byte(f) == S390_TOD_CLOCK_VALUE_MISSING) {
+        fprintf(stderr, "WARNING: Guest clock was not migrated. This could "
+                        "cause the guest to hang.\n");
+        return 0;
+    }
+
+    tod_high = qemu_get_byte(f);
+    tod_low = qemu_get_be64(f);
+
+    r = s390_set_clock(&tod_high, &tod_low);
+    if (r) {
+        fprintf(stderr, "WARNING: Unable to set guest clock value. "
+                        "s390_get_clock returned error %d. This could cause "
+                        "the guest to hang.\n", r);
+    }
+
+    return 0;
+}
+
 static SaveVMHandlers savevm_gtod = {
     .save_state = gtod_save,
     .load_state = gtod_load,
 };
 
+static void s390_init_ipl_dev(const char *kernel_filename,
+                              const char *kernel_cmdline,
+                              const char *initrd_filename, const char *firmware,
+                              const char *netboot_fw, bool enforce_bios)
+{
+    Object *new = object_new(TYPE_S390_IPL);
+    DeviceState *dev = DEVICE(new);
+
+    if (kernel_filename) {
+        qdev_prop_set_string(dev, "kernel", kernel_filename);
+    }
+    if (initrd_filename) {
+        qdev_prop_set_string(dev, "initrd", initrd_filename);
+    }
+    qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
+    qdev_prop_set_string(dev, "firmware", firmware);
+    qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
+    qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
+    object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
+                              new, NULL);
+    object_unref(new);
+    qdev_init_nofail(dev);
+}
+
+static void s390_create_virtio_net(BusState *bus, const char *name)
+{
+    int i;
+
+    for (i = 0; i < nb_nics; i++) {
+        NICInfo *nd = &nd_table[i];
+        DeviceState *dev;
+
+        if (!nd->model) {
+            nd->model = g_strdup("virtio");
+        }
+
+        qemu_check_nic_model(nd, "virtio");
+
+        dev = qdev_create(bus, name);
+        qdev_set_nic_properties(dev, nd);
+        qdev_init_nofail(dev);
+    }
+}
+
 static void ccw_init(MachineState *machine)
 {
     int ret;
@@ -177,6 +310,19 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     g_free(name);
 }
 
+static void s390_machine_reset(void)
+{
+    S390CPU *ipl_cpu = S390_CPU(qemu_get_cpu(0));
+
+    s390_cmma_reset();
+    qemu_devices_reset();
+    s390_crypto_reset();
+
+    /* all cpus are stopped - configure and start the ipl cpu only */
+    s390_ipl_prepare_cpu(ipl_cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+}
+
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
@@ -201,6 +347,15 @@ static void s390_hot_add_cpu(const int64_t id, Error **errp)
     s390x_new_cpu(machine->cpu_model, id, errp);
 }
 
+static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
+{
+    CPUState *cs = qemu_get_cpu(cpu_index);
+
+    if (s390_cpu_restart(S390_CPU(cs))) {
+        error_setg(errp, QERR_UNSUPPORTED);
+    }
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index da3f49e80e..9316be1cfc 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -48,17 +48,6 @@
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
-static S390CPU **cpu_states;
-
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
-{
-    if (cpu_addr >= max_cpus) {
-        return NULL;
-    }
-
-    /* Fast lookup via CPU ID */
-    return cpu_states[cpu_addr];
-}
 
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
@@ -86,32 +75,6 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(MachineState *machine)
-{
-    int i;
-    gchar *name;
-
-    if (machine->cpu_model == NULL) {
-        machine->cpu_model = s390_default_cpu_model_name();
-    }
-
-    cpu_states = g_new0(S390CPU *, max_cpus);
-
-    for (i = 0; i < max_cpus; i++) {
-        name = g_strdup_printf("cpu[%i]", i);
-        object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
-                                 (Object **) &cpu_states[i],
-                                 object_property_allow_set_link,
-                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
-                                 &error_abort);
-        g_free(name);
-    }
-
-    for (i = 0; i < smp_cpus; i++) {
-        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
-    }
-}
-
 
 void s390_create_virtio_net(BusState *bus, const char *name)
 {
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index ca97fd6814..d984cd4115 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -12,24 +12,9 @@
 #ifndef HW_S390_VIRTIO_H
 #define HW_S390_VIRTIO_H
 
-#include "hw/nmi.h"
 #include "standard-headers/asm-s390/kvm_virtio.h"
 #include "standard-headers/asm-s390/virtio-ccw.h"
 
 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(MachineState *machine);
-void s390_init_ipl_dev(const char *kernel_filename,
-                       const char *kernel_cmdline,
-                       const char *initrd_filename,
-                       const char *firmware,
-                       const char *netboot_fw,
-                       bool enforce_bios);
-void s390_create_virtio_net(BusState *bus, const char *name);
-void s390_nmi(NMIState *n, int cpu_index, Error **errp);
-void s390_machine_reset(void);
-void s390_memory_init(ram_addr_t mem_size);
-void gtod_save(QEMUFile *f, void *opaque);
-int gtod_load(QEMUFile *f, void *opaque, int version_id);
 #endif
-- 
2.13.5

[Qemu-devel] [PATCH v2 04/19] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[David Hildenbrand]

The only interface left, so let's properly rename it.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c                      | 2 +-
 hw/s390x/s390-virtio-hcall.c                    | 2 +-
 hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (77%)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4c3a3e177b..f67b4b5d58 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -16,7 +16,7 @@
 #include "cpu.h"
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
-#include "s390-virtio.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index 23d67d6170..ec7cf8beb3 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -11,7 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hw/s390x/s390-virtio.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 
 #define MAX_DIAG_SUBCODES 255
 
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio-hcall.h
similarity index 77%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..59fcba3a06 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -1,5 +1,5 @@
 /*
- * Virtio interfaces for s390
+ * Support for virtio hypercalls on s390x
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -9,12 +9,12 @@
  * directory.
  */
 
-#ifndef HW_S390_VIRTIO_H
-#define HW_S390_VIRTIO_H
+#ifndef HW_S390_VIRTIO_HCALL_H
+#define HW_S390_VIRTIO_HCALL_H
 
 #include "standard-headers/asm-s390/kvm_virtio.h"
 #include "standard-headers/asm-s390/virtio-ccw.h"
 
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
-#endif
+#endif /* HW_S390_VIRTIO_HCALL_H */
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 04/19] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[Christian Borntraeger]

On 09/04/2017 05:43 PM, David Hildenbrand wrote:
> The only interface left, so let's properly rename it.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  hw/s390x/s390-virtio-ccw.c                      | 2 +-
>  hw/s390x/s390-virtio-hcall.c                    | 2 +-
>  hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 8 ++++----
>  3 files changed, 6 insertions(+), 6 deletions(-)
>  rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (77%)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4c3a3e177b..f67b4b5d58 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -16,7 +16,7 @@
>  #include "cpu.h"
>  #include "hw/boards.h"
>  #include "exec/address-spaces.h"
> -#include "s390-virtio.h"
> +#include "hw/s390x/s390-virtio-hcall.h"
>  #include "hw/s390x/sclp.h"
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/ioinst.h"
> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
> index 23d67d6170..ec7cf8beb3 100644
> --- a/hw/s390x/s390-virtio-hcall.c
> +++ b/hw/s390x/s390-virtio-hcall.c
> @@ -11,7 +11,7 @@
> 
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> -#include "hw/s390x/s390-virtio.h"
> +#include "hw/s390x/s390-virtio-hcall.h"
> 
>  #define MAX_DIAG_SUBCODES 255
> 
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio-hcall.h
> similarity index 77%
> rename from hw/s390x/s390-virtio.h
> rename to hw/s390x/s390-virtio-hcall.h
> index d984cd4115..59fcba3a06 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio-hcall.h
> @@ -1,5 +1,5 @@
>  /*
> - * Virtio interfaces for s390
> + * Support for virtio hypercalls on s390x
>   *
>   * Copyright 2012 IBM Corp.
>   * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
> @@ -9,12 +9,12 @@
>   * directory.
>   */
> 
> -#ifndef HW_S390_VIRTIO_H
> -#define HW_S390_VIRTIO_H
> +#ifndef HW_S390_VIRTIO_HCALL_H
> +#define HW_S390_VIRTIO_HCALL_H
> 
>  #include "standard-headers/asm-s390/kvm_virtio.h"
>  #include "standard-headers/asm-s390/virtio-ccw.h"
> 
>  typedef int (*s390_virtio_fn)(const uint64_t *args);
>  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
> -#endif
> +#endif /* HW_S390_VIRTIO_HCALL_H */
> 

[Qemu-devel] [PATCH v2 05/19] target/s390x: move typedef of S390CPU to its definition

[David Hildenbrand]

Let's move it to the palce where struct S390CPU is defined.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h | 2 --
 target/s390x/cpu.h     | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index 4e936e7788..c8fbf8ae03 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -61,6 +61,4 @@ typedef struct S390CPUClass {
     void (*initial_cpu_reset)(CPUState *cpu);
 } S390CPUClass;
 
-typedef struct S390CPU S390CPU;
-
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 0bd97a5670..a7426ef6e2 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -187,7 +187,7 @@ static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr)
  *
  * An S/390 CPU.
  */
-struct S390CPU {
+typedef struct S390CPU {
     /*< private >*/
     CPUState parent_obj;
     /*< public >*/
@@ -198,7 +198,7 @@ struct S390CPU {
     /* needed for live migration */
     void *irqstate;
     uint32_t irqstate_saved_size;
-};
+} S390CPU;
 
 static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
 {
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 05/19] target/s390x: move typedef of S390CPU to its definition

[Cornelia Huck]

On Mon,  4 Sep 2017 17:43:02 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's move it to the palce where struct S390CPU is defined.

s/palce/place/

Can also fix on applying.

> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu-qom.h | 2 --
>  target/s390x/cpu.h     | 4 ++--
>  2 files changed, 2 insertions(+), 4 deletions(-)

Re: [Qemu-devel] [PATCH v2 05/19] target/s390x: move typedef of S390CPU to its definition

[Thomas Huth]

On 04.09.2017 17:43, David Hildenbrand wrote:
> Let's move it to the palce where struct S390CPU is defined.

s/palce/place/

> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu-qom.h | 2 --
>  target/s390x/cpu.h     | 4 ++--
>  2 files changed, 2 insertions(+), 4 deletions(-)

Seems to compile fine for me with this patch applied, so:

Tested-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v2 06/19] s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h

[David Hildenbrand]

Implemented in hw/s390x/s390-virtio-hcall.c, so let's move it to the
right header file.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-hcall.h | 1 +
 target/s390x/cpu.h           | 1 -
 target/s390x/kvm.c           | 1 +
 target/s390x/misc_helper.c   | 1 +
 4 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 59fcba3a06..cbc270eef3 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -17,4 +17,5 @@
 
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
+int s390_virtio_hypercall(CPUS390XState *env);
 #endif /* HW_S390_VIRTIO_HCALL_H */
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index a7426ef6e2..2343f2f1f7 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -723,6 +723,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 extern void subsystem_reset(void);
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
-int s390_virtio_hypercall(CPUS390XState *env);
 
 #endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index d07763ff2c..ed59896423 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -48,6 +48,7 @@
 #include "hw/s390x/ebcdic.h"
 #include "exec/memattrs.h"
 #include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 50cc046ca2..b142db71c6 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -34,6 +34,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/sysemu.h"
 #include "hw/s390x/ebcdic.h"
+#include "hw/s390x/s390-virtio-hcall.h"
 #endif
 
 /* #define DEBUG_HELPER */
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 06/19] s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h

[Thomas Huth]

On 04.09.2017 17:43, David Hildenbrand wrote:
> Implemented in hw/s390x/s390-virtio-hcall.c, so let's move it to the
> right header file.

That's fair.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v2 07/19] s390x: move two function declarations to s390-virtio-ccw.h

[David Hildenbrand]

We can also drop the extern. Fix up includes.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/hw/s390x/s390-virtio-ccw.h | 3 +++
 target/s390x/cpu.h                 | 2 --
 target/s390x/diag.c                | 1 +
 target/s390x/interrupt.c           | 3 +++
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 41a9d2862b..42f2ccdb43 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -56,4 +56,7 @@ bool gs_allowed(void);
  */
 bool css_migration_enabled(void);
 
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
+void subsystem_reset(void);
+
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 2343f2f1f7..6c0abb9894 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -720,8 +720,6 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 
 
 /* outside of target/s390x/ */
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-extern void subsystem_reset(void);
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 
 #endif
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index e6b5e6de37..82a623948d 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -20,6 +20,7 @@
 #include "hw/watchdog/wdt_diag288.h"
 #include "sysemu/cpus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 static int modified_clear_reset(S390CPU *cpu)
 {
diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
index 058e219fe5..9aa6b89301 100644
--- a/target/s390x/interrupt.c
+++ b/target/s390x/interrupt.c
@@ -15,6 +15,9 @@
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
 #include "hw/s390x/ioinst.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/s390x/s390-virtio-ccw.h"
+#endif
 
 /* Ensure to exit the TB after this call! */
 void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 07/19] s390x: move two function declarations to s390-virtio-ccw.h

[Thomas Huth]

On 04.09.2017 17:43, David Hildenbrand wrote:
> We can also drop the extern. Fix up includes.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/hw/s390x/s390-virtio-ccw.h | 3 +++
>  target/s390x/cpu.h                 | 2 --
>  target/s390x/diag.c                | 1 +
>  target/s390x/interrupt.c           | 3 +++
>  4 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 41a9d2862b..42f2ccdb43 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -56,4 +56,7 @@ bool gs_allowed(void);
>   */
>  bool css_migration_enabled(void);
>  
> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> +void subsystem_reset(void);
> +
>  #endif
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 2343f2f1f7..6c0abb9894 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -720,8 +720,6 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
>  
>  
>  /* outside of target/s390x/ */
> -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> -extern void subsystem_reset(void);
>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
>  
>  #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index e6b5e6de37..82a623948d 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -20,6 +20,7 @@
>  #include "hw/watchdog/wdt_diag288.h"
>  #include "sysemu/cpus.h"
>  #include "hw/s390x/ipl.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
>  
>  static int modified_clear_reset(S390CPU *cpu)
>  {
> diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
> index 058e219fe5..9aa6b89301 100644
> --- a/target/s390x/interrupt.c
> +++ b/target/s390x/interrupt.c
> @@ -15,6 +15,9 @@
>  #include "exec/exec-all.h"
>  #include "sysemu/kvm.h"
>  #include "hw/s390x/ioinst.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#endif
>  
>  /* Ensure to exit the TB after this call! */
>  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)
> 

If I listen to my gut feeling, I still think we should rather move
s390_cpu_addr2state() to cpu.c instead. It does not sound as it's really
specific to the virtio-ccw machine... Just my 0.02 €

 Thomas

Re: [Qemu-devel] [PATCH v2 07/19] s390x: move two function declarations to s390-virtio-ccw.h

[David Hildenbrand]

>> +++ b/target/s390x/interrupt.c
>> @@ -15,6 +15,9 @@
>>  #include "exec/exec-all.h"
>>  #include "sysemu/kvm.h"
>>  #include "hw/s390x/ioinst.h"
>> +#if !defined(CONFIG_USER_ONLY)
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#endif
>>  
>>  /* Ensure to exit the TB after this call! */
>>  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)
>>
> 
> If I listen to my gut feeling, I still think we should rather move
> s390_cpu_addr2state() to cpu.c instead. It does not sound as it's really
> specific to the virtio-ccw machine... Just my 0.02 €
> 

I prefer to leave it as is for now, so just a header cleanup. E.g. with
patch 18 we could implement it directly using ms->possible_cpus.

>  Thomas


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 07/19] s390x: move two function declarations to s390-virtio-ccw.h

[Cornelia Huck]

On Thu, 7 Sep 2017 14:46:44 +0200
David Hildenbrand <david@redhat.com> wrote:

> >> +++ b/target/s390x/interrupt.c
> >> @@ -15,6 +15,9 @@
> >>  #include "exec/exec-all.h"
> >>  #include "sysemu/kvm.h"
> >>  #include "hw/s390x/ioinst.h"
> >> +#if !defined(CONFIG_USER_ONLY)
> >> +#include "hw/s390x/s390-virtio-ccw.h"
> >> +#endif
> >>  
> >>  /* Ensure to exit the TB after this call! */
> >>  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)
> >>  
> > 
> > If I listen to my gut feeling, I still think we should rather move
> > s390_cpu_addr2state() to cpu.c instead. It does not sound as it's really
> > specific to the virtio-ccw machine... Just my 0.02 €
> >   
> 
> I prefer to leave it as is for now, so just a header cleanup. E.g. with
> patch 18 we could implement it directly using ms->possible_cpus.

If you do that, would you also get rid of the conditional include above?

[Qemu-devel] [PATCH v2 08/19] s390x: move sclp_service_call() to interrupt.c

[David Hildenbrand]

Fix up includes and rename it to s390x_*.
cpu.h is now free of externally implemented functions.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/sclp.c            | 51 +---------------------------------------------
 include/hw/s390x/sclp.h    |  1 +
 target/s390x/cpu.h         |  5 +----
 target/s390x/interrupt.c   | 49 ++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/kvm.c         |  2 +-
 target/s390x/misc_helper.c |  2 +-
 6 files changed, 54 insertions(+), 56 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fd097262c7..9618dfac0a 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -24,7 +24,7 @@
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/ipl.h"
 
-static inline SCLPDevice *get_sclp_device(void)
+SCLPDevice *get_sclp_device(void)
 {
     static SCLPDevice *sclp;
 
@@ -424,55 +424,6 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
     }
 }
 
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
-{
-    SCLPDevice *sclp = get_sclp_device();
-    SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
-    int r = 0;
-    SCCB work_sccb;
-
-    hwaddr sccb_len = sizeof(SCCB);
-
-    /* first some basic checks on program checks */
-    if (env->psw.mask & PSW_MASK_PSTATE) {
-        r = -PGM_PRIVILEGED;
-        goto out;
-    }
-    if (cpu_physical_memory_is_io(sccb)) {
-        r = -PGM_ADDRESSING;
-        goto out;
-    }
-    if ((sccb & ~0x1fffUL) == 0 || (sccb & ~0x1fffUL) == env->psa
-        || (sccb & ~0x7ffffff8UL) != 0) {
-        r = -PGM_SPECIFICATION;
-        goto out;
-    }
-
-    /*
-     * we want to work on a private copy of the sccb, to prevent guests
-     * from playing dirty tricks by modifying the memory content after
-     * the host has checked the values
-     */
-    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
-
-    /* Valid sccb sizes */
-    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader) ||
-        be16_to_cpu(work_sccb.h.length) > SCCB_SIZE) {
-        r = -PGM_SPECIFICATION;
-        goto out;
-    }
-
-    sclp_c->execute(sclp, &work_sccb, code);
-
-    cpu_physical_memory_write(sccb, &work_sccb,
-                              be16_to_cpu(work_sccb.h.length));
-
-    sclp_c->service_interrupt(sclp, sccb);
-
-out:
-    return r;
-}
-
 static void service_interrupt(SCLPDevice *sclp, uint32_t sccb)
 {
     SCLPEventFacility *ef = sclp->event_facility;
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index a72d096081..1cc233d01f 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -237,6 +237,7 @@ static inline int sccb_data_len(SCCB *sccb)
 }
 
 
+SCLPDevice *get_sclp_device(void);
 void s390_sclp_init(void);
 sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void);
 sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void);
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 6c0abb9894..147aceba28 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -698,6 +698,7 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void *puc);
 
 
 /* interrupt.c */
+int s390x_sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
 void s390_crw_mchk(void);
 void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
                        uint32_t io_int_parm, uint32_t io_int_word);
@@ -718,8 +719,4 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 #define s390_cpu_virt_mem_check_write(cpu, laddr, ar, len)   \
         s390_cpu_virt_mem_rw(cpu, laddr, ar, NULL, len, true)
 
-
-/* outside of target/s390x/ */
-int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
-
 #endif
diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
index 9aa6b89301..a3b627dd40 100644
--- a/target/s390x/interrupt.c
+++ b/target/s390x/interrupt.c
@@ -17,6 +17,7 @@
 #include "hw/s390x/ioinst.h"
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 /* Ensure to exit the TB after this call! */
@@ -161,4 +162,52 @@ void s390_crw_mchk(void)
     }
 }
 
+int s390x_sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
+{
+    SCLPDevice *sclp = get_sclp_device();
+    SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
+    int r = 0;
+    SCCB work_sccb;
+
+    hwaddr sccb_len = sizeof(SCCB);
+
+    /* first some basic checks on program checks */
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        r = -PGM_PRIVILEGED;
+        goto out;
+    }
+    if (cpu_physical_memory_is_io(sccb)) {
+        r = -PGM_ADDRESSING;
+        goto out;
+    }
+    if ((sccb & ~0x1fffUL) == 0 || (sccb & ~0x1fffUL) == env->psa
+        || (sccb & ~0x7ffffff8UL) != 0) {
+        r = -PGM_SPECIFICATION;
+        goto out;
+    }
+
+    /*
+     * we want to work on a private copy of the sccb, to prevent guests
+     * from playing dirty tricks by modifying the memory content after
+     * the host has checked the values
+     */
+    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
+
+    /* Valid sccb sizes */
+    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader) ||
+        be16_to_cpu(work_sccb.h.length) > SCCB_SIZE) {
+        r = -PGM_SPECIFICATION;
+        goto out;
+    }
+
+    sclp_c->execute(sclp, &work_sccb, code);
+
+    cpu_physical_memory_write(sccb, &work_sccb,
+                              be16_to_cpu(work_sccb.h.length));
+
+    sclp_c->service_interrupt(sclp, sccb);
+
+out:
+    return r;
+}
 #endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ed59896423..c36e4efdec 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1069,7 +1069,7 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     sccb = env->regs[ipbh0 & 0xf];
     code = env->regs[(ipbh0 & 0xf0) >> 4];
 
-    r = sclp_service_call(env, sccb, code);
+    r = s390x_sclp_service_call(env, sccb, code);
     if (r < 0) {
         kvm_s390_program_interrupt(cpu, -r);
     } else {
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index b142db71c6..57c02ddf1b 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -76,7 +76,7 @@ void HELPER(exception)(CPUS390XState *env, uint32_t excp)
 uint32_t HELPER(servc)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 {
     qemu_mutex_lock_iothread();
-    int r = sclp_service_call(env, r1, r2);
+    int r = s390x_sclp_service_call(env, r1, r2);
     if (r < 0) {
         program_interrupt(env, -r, 4);
         r = 0;
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 08/19] s390x: move sclp_service_call() to interrupt.c

[Cornelia Huck]

On Mon,  4 Sep 2017 17:43:05 +0200
David Hildenbrand <david@redhat.com> wrote:

> Fix up includes and rename it to s390x_*.

I'm not quite sure whether that is the right direction: servc is just an
instruction that does something and then happens to also generate an
interrupt on conclusion. I'll think a bit more about it.

> cpu.h is now free of externally implemented functions.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/sclp.c            | 51 +---------------------------------------------
>  include/hw/s390x/sclp.h    |  1 +
>  target/s390x/cpu.h         |  5 +----
>  target/s390x/interrupt.c   | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  target/s390x/kvm.c         |  2 +-
>  target/s390x/misc_helper.c |  2 +-
>  6 files changed, 54 insertions(+), 56 deletions(-)

Re: [Qemu-devel] [PATCH v2 08/19] s390x: move sclp_service_call() to interrupt.c

[David Hildenbrand]

On 05.09.2017 14:38, Cornelia Huck wrote:
> On Mon,  4 Sep 2017 17:43:05 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Fix up includes and rename it to s390x_*.
> 
> I'm not quite sure whether that is the right direction: servc is just an
> instruction that does something and then happens to also generate an
> interrupt on conclusion. I'll think a bit more about it.
> 

Having CPU related stuff in sclp looks also wrong. Feel free to skip
this patch, should be unrelated to the following patches.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 08/19] s390x: move sclp_service_call() to interrupt.c

[Christian Borntraeger]

On 09/05/2017 02:42 PM, David Hildenbrand wrote:
> On 05.09.2017 14:38, Cornelia Huck wrote:
>> On Mon,  4 Sep 2017 17:43:05 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> Fix up includes and rename it to s390x_*.
>>
>> I'm not quite sure whether that is the right direction: servc is just an
>> instruction that does something and then happens to also generate an
>> interrupt on conclusion. I'll think a bit more about it.
>>
> 
> Having CPU related stuff in sclp looks also wrong. Feel free to skip
> this patch, should be unrelated to the following patches.

I think having the sclp instruction handler in sclp (as today) is
the best compromise.

Re: [Qemu-devel] [PATCH v2 08/19] s390x: move sclp_service_call() to interrupt.c

[Thomas Huth]

On 05.09.2017 14:46, Christian Borntraeger wrote:
> 
> 
> On 09/05/2017 02:42 PM, David Hildenbrand wrote:
>> On 05.09.2017 14:38, Cornelia Huck wrote:
>>> On Mon,  4 Sep 2017 17:43:05 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Fix up includes and rename it to s390x_*.
>>>
>>> I'm not quite sure whether that is the right direction: servc is just an
>>> instruction that does something and then happens to also generate an
>>> interrupt on conclusion. I'll think a bit more about it.
>>>
>>
>> Having CPU related stuff in sclp looks also wrong. Feel free to skip
>> this patch, should be unrelated to the following patches.
> 
> I think having the sclp instruction handler in sclp (as today) is
> the best compromise.

+1

But maybe you could at least move the prototype to sclp.h instead?

 Thomas

Re: [Qemu-devel] [PATCH v2 08/19] s390x: move sclp_service_call() to interrupt.c

[Cornelia Huck]

On Tue, 5 Sep 2017 14:52:44 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 05.09.2017 14:46, Christian Borntraeger wrote:
> > 
> > 
> > On 09/05/2017 02:42 PM, David Hildenbrand wrote:  
> >> On 05.09.2017 14:38, Cornelia Huck wrote:  
> >>> On Mon,  4 Sep 2017 17:43:05 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>  
> >>>> Fix up includes and rename it to s390x_*.  
> >>>
> >>> I'm not quite sure whether that is the right direction: servc is just an
> >>> instruction that does something and then happens to also generate an
> >>> interrupt on conclusion. I'll think a bit more about it.
> >>>  
> >>
> >> Having CPU related stuff in sclp looks also wrong. Feel free to skip
> >> this patch, should be unrelated to the following patches.  
> > 
> > I think having the sclp instruction handler in sclp (as today) is
> > the best compromise.  
> 
> +1
> 
> But maybe you could at least move the prototype to sclp.h instead?

That would be a good compromise.

[Qemu-devel] [PATCH v2 09/19] target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

[David Hildenbrand]

This looks cleaner. linux-user will not use the ilen field, so setting
it doesn't do any harm.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/excp_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 361f970db3..14d3160e92 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -59,8 +59,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
 {
     S390CPU *cpu = S390_CPU(cs);
 
-    cs->exception_index = EXCP_PGM;
-    cpu->env.int_pgm_code = PGM_ADDRESSING;
+    trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO);
     /* On real machines this value is dropped into LowMem.  Since this
        is userland, simply put this someplace that cpu_loop can find it.  */
     cpu->env.__excp_addr = address;
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 09/19] target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

[Thomas Huth]

On 04.09.2017 17:43, David Hildenbrand wrote:
> This looks cleaner. linux-user will not use the ilen field, so setting
> it doesn't do any harm.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/excp_helper.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index 361f970db3..14d3160e92 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -59,8 +59,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
>  {
>      S390CPU *cpu = S390_CPU(cs);
>  
> -    cs->exception_index = EXCP_PGM;
> -    cpu->env.int_pgm_code = PGM_ADDRESSING;
> +    trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO);
>      /* On real machines this value is dropped into LowMem.  Since this
>         is userland, simply put this someplace that cpu_loop can find it.  */
>      cpu->env.__excp_addr = address;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v2 10/19] target/s390x: use program_interrupt() in per_check_exception()

[David Hildenbrand]

Clean it up by reusing program_interrupt(). Add a concern regarding
ilen.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/misc_helper.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 57c02ddf1b..5096286157 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
 #ifndef CONFIG_USER_ONLY
 void HELPER(per_check_exception)(CPUS390XState *env)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
+    uint32_t ilen;
 
     if (env->per_perc_atmid) {
-        env->int_pgm_code = PGM_PER;
-        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
-
-        cs->exception_index = EXCP_PGM;
-        cpu_loop_exit(cs);
+        /*
+         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
+         * always has to match the instruction referenced in the PSW. E.g.
+         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
+         * of EXECUTE, while per_address contains the target of EXECUTE.
+         */
+        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
+        program_interrupt(env, PGM_PER, ilen);
     }
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 10/19] target/s390x: use program_interrupt() in per_check_exception()

[Cornelia Huck]

On Mon,  4 Sep 2017 17:43:07 +0200
David Hildenbrand <david@redhat.com> wrote:

> Clean it up by reusing program_interrupt(). Add a concern regarding
> ilen.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/misc_helper.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 57c02ddf1b..5096286157 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
>  #ifndef CONFIG_USER_ONLY
>  void HELPER(per_check_exception)(CPUS390XState *env)
>  {
> -    CPUState *cs = CPU(s390_env_get_cpu(env));
> +    uint32_t ilen;
>  
>      if (env->per_perc_atmid) {
> -        env->int_pgm_code = PGM_PER;
> -        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
> -
> -        cs->exception_index = EXCP_PGM;
> -        cpu_loop_exit(cs);
> +        /*
> +         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
> +         * always has to match the instruction referenced in the PSW. E.g.
> +         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
> +         * of EXECUTE, while per_address contains the target of EXECUTE.
> +         */
> +        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
> +        program_interrupt(env, PGM_PER, ilen);
>      }
>  }
>  

Wrapping my head around it: You preserve the current behavior, which
should probably be changed to use ILEN_AUTO to handle cases like
EXECUTE correctly?

Do you plan to do a follow up? (Should I spend some cycles on it?)

Re: [Qemu-devel] [PATCH v2 10/19] target/s390x: use program_interrupt() in per_check_exception()

[David Hildenbrand]

On 07.09.2017 15:32, Cornelia Huck wrote:
> On Mon,  4 Sep 2017 17:43:07 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Clean it up by reusing program_interrupt(). Add a concern regarding
>> ilen.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/misc_helper.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>> index 57c02ddf1b..5096286157 100644
>> --- a/target/s390x/misc_helper.c
>> +++ b/target/s390x/misc_helper.c
>> @@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
>>  #ifndef CONFIG_USER_ONLY
>>  void HELPER(per_check_exception)(CPUS390XState *env)
>>  {
>> -    CPUState *cs = CPU(s390_env_get_cpu(env));
>> +    uint32_t ilen;
>>  
>>      if (env->per_perc_atmid) {
>> -        env->int_pgm_code = PGM_PER;
>> -        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
>> -
>> -        cs->exception_index = EXCP_PGM;
>> -        cpu_loop_exit(cs);
>> +        /*
>> +         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
>> +         * always has to match the instruction referenced in the PSW. E.g.
>> +         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
>> +         * of EXECUTE, while per_address contains the target of EXECUTE.
>> +         */
>> +        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
>> +        program_interrupt(env, PGM_PER, ilen);
>>      }
>>  }
>>  
> 
> Wrapping my head around it: You preserve the current behavior, which
> should probably be changed to use ILEN_AUTO to handle cases like
> EXECUTE correctly?

Exactly, e.g. if EXECUTE executes instruction Y, and Y is to generate an
IFETCH event, AFAIK

a) per_address points at Y
b) old PGM PSW points at instruction after EXECUTE (ignoring nullifying
for now)
c) PGM ilen has to match EXECUTE (so the PSW can properly be rewound)

The case where per_address == (PGM PSW - ilen) (ignoring nullification)
should happen without EXECUTE being involved. That's why PER currently
works just fine with Linux (e.g. uprobe smoke tests when kernel boots up).

Execute handling with PER is just nasty. And one first has to find out
how that plays together with EXECUTE handling in TCG. Therefore, no easy
fix (although ILEN_AUTO might most probably really be the right thing to
do).

> 
> Do you plan to do a follow up? (Should I spend some cycles on it?)
> 

Somewhere on my list. The next thing (in this area) I want do is write
kvm-unit-tests for PER, because also KVM could benefit from that
(EXECUTE handling is just nasty).

So sure, go ahead and have a look at it if you have some spare cycles :)

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 10/19] target/s390x: use program_interrupt() in per_check_exception()

[Cornelia Huck]

On Thu, 7 Sep 2017 15:52:19 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 07.09.2017 15:32, Cornelia Huck wrote:
> > On Mon,  4 Sep 2017 17:43:07 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Clean it up by reusing program_interrupt(). Add a concern regarding
> >> ilen.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  target/s390x/misc_helper.c | 15 +++++++++------
> >>  1 file changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> >> index 57c02ddf1b..5096286157 100644
> >> --- a/target/s390x/misc_helper.c
> >> +++ b/target/s390x/misc_helper.c
> >> @@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
> >>  #ifndef CONFIG_USER_ONLY
> >>  void HELPER(per_check_exception)(CPUS390XState *env)
> >>  {
> >> -    CPUState *cs = CPU(s390_env_get_cpu(env));
> >> +    uint32_t ilen;
> >>  
> >>      if (env->per_perc_atmid) {
> >> -        env->int_pgm_code = PGM_PER;
> >> -        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
> >> -
> >> -        cs->exception_index = EXCP_PGM;
> >> -        cpu_loop_exit(cs);
> >> +        /*
> >> +         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
> >> +         * always has to match the instruction referenced in the PSW. E.g.
> >> +         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
> >> +         * of EXECUTE, while per_address contains the target of EXECUTE.
> >> +         */
> >> +        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
> >> +        program_interrupt(env, PGM_PER, ilen);
> >>      }
> >>  }
> >>    
> > 
> > Wrapping my head around it: You preserve the current behavior, which
> > should probably be changed to use ILEN_AUTO to handle cases like
> > EXECUTE correctly?  
> 
> Exactly, e.g. if EXECUTE executes instruction Y, and Y is to generate an
> IFETCH event, AFAIK
> 
> a) per_address points at Y
> b) old PGM PSW points at instruction after EXECUTE (ignoring nullifying
> for now)
> c) PGM ilen has to match EXECUTE (so the PSW can properly be rewound)
> 
> The case where per_address == (PGM PSW - ilen) (ignoring nullification)
> should happen without EXECUTE being involved. That's why PER currently
> works just fine with Linux (e.g. uprobe smoke tests when kernel boots up).
> 
> Execute handling with PER is just nasty. And one first has to find out
> how that plays together with EXECUTE handling in TCG. Therefore, no easy
> fix (although ILEN_AUTO might most probably really be the right thing to
> do).

That really sounds like a lot of fun...

> 
> > 
> > Do you plan to do a follow up? (Should I spend some cycles on it?)
> >   
> 
> Somewhere on my list. The next thing (in this area) I want do is write
> kvm-unit-tests for PER, because also KVM could benefit from that
> (EXECUTE handling is just nasty).
> 
> So sure, go ahead and have a look at it if you have some spare cycles :)

Spare cycles? Me? :)

I think we can live with the current state a bit longer. Adding the
FIXME is actually an improvement...

[Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the
guest tries to bring these CPUs up but fails), because we don't support
multiple CPUs on s390x under TCG.

Let's bail out if more than 1 is specified, so we don't raise people's
hope. Make it a define, so we can easily bump it up later.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 7 +++++++
 target/s390x/cpu.h         | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f67b4b5d58..f7ca20d77a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -23,6 +23,7 @@
 #include "hw/s390x/css.h"
 #include "virtio-ccw.h"
 #include "qemu/config-file.h"
+#include "qemu/error-report.h"
 #include "s390-pci-bus.h"
 #include "hw/s390x/storage-keys.h"
 #include "hw/s390x/storage-attributes.h"
@@ -55,6 +56,12 @@ static void s390_init_cpus(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = s390_default_cpu_model_name();
     }
+    if (tcg_enabled() && max_cpus > S390_TCG_MAX_CPUS) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by TCG (%d) on s390x", max_cpus,
+                     S390_TCG_MAX_CPUS);
+        exit(1);
+    }
 
     cpu_states = g_new0(S390CPU *, max_cpus);
 
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 147aceba28..dca6aa9aae 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -209,6 +209,8 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
 
 #define ENV_OFFSET offsetof(S390CPU, env)
 
+#define S390_TCG_MAX_CPUS 1
+
 #ifndef CONFIG_USER_ONLY
 extern const struct VMStateDescription vmstate_s390_cpu;
 #endif
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[Matthew Rosato]

On 09/04/2017 11:43 AM, David Hildenbrand wrote:
> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the
> guest tries to bring these CPUs up but fails), because we don't support
> multiple CPUs on s390x under TCG.
> 
> Let's bail out if more than 1 is specified, so we don't raise people's
> hope. Make it a define, so we can easily bump it up later.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

Makes sense.  Ran the described environment without this patch (errors)
and again with this patch (graceful exit w/ message).

Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

>  hw/s390x/s390-virtio-ccw.c | 7 +++++++
>  target/s390x/cpu.h         | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f67b4b5d58..f7ca20d77a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -23,6 +23,7 @@
>  #include "hw/s390x/css.h"
>  #include "virtio-ccw.h"
>  #include "qemu/config-file.h"
> +#include "qemu/error-report.h"
>  #include "s390-pci-bus.h"
>  #include "hw/s390x/storage-keys.h"
>  #include "hw/s390x/storage-attributes.h"
> @@ -55,6 +56,12 @@ static void s390_init_cpus(MachineState *machine)
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = s390_default_cpu_model_name();
>      }
> +    if (tcg_enabled() && max_cpus > S390_TCG_MAX_CPUS) {
> +        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> +                     "supported by TCG (%d) on s390x", max_cpus,
> +                     S390_TCG_MAX_CPUS);
> +        exit(1);
> +    }
> 
>      cpu_states = g_new0(S390CPU *, max_cpus);
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 147aceba28..dca6aa9aae 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -209,6 +209,8 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
> 
>  #define ENV_OFFSET offsetof(S390CPU, env)
> 
> +#define S390_TCG_MAX_CPUS 1
> +
>  #ifndef CONFIG_USER_ONLY
>  extern const struct VMStateDescription vmstate_s390_cpu;
>  #endif
> 

Re: [Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[Richard Henderson]

On 09/06/2017 11:16 AM, Matthew Rosato wrote:
> On 09/04/2017 11:43 AM, David Hildenbrand wrote:
>> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the
>> guest tries to bring these CPUs up but fails), because we don't support
>> multiple CPUs on s390x under TCG.
>>
>> Let's bail out if more than 1 is specified, so we don't raise people's
>> hope. Make it a define, so we can easily bump it up later.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
> 
> Makes sense.  Ran the described environment without this patch (errors)
> and again with this patch (graceful exit w/ message).
> 
> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Can someone review

  http://patchwork.ozlabs.org/patch/760010/

which does at least start to add the SIGP support.

Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N.  I don't
see the point of the define.


r~

Re: [Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

On 06.09.2017 23:20, Richard Henderson wrote:
> On 09/06/2017 11:16 AM, Matthew Rosato wrote:
>> On 09/04/2017 11:43 AM, David Hildenbrand wrote:
>>> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the
>>> guest tries to bring these CPUs up but fails), because we don't support
>>> multiple CPUs on s390x under TCG.
>>>
>>> Let's bail out if more than 1 is specified, so we don't raise people's
>>> hope. Make it a define, so we can easily bump it up later.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>
>> Makes sense.  Ran the described environment without this patch (errors)
>> and again with this patch (graceful exit w/ message).
>>
>> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> 
> Can someone review
> 
>   http://patchwork.ozlabs.org/patch/760010/
> 

Yes, we also discovered that patch during review of v1.

> which does at least start to add the SIGP support.
> 
> Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N.  I don't
> see the point of the define.
> 

Conny requested a define. So it boils down to

a) no define just as in v1
b) a define like S390_TCG_SMP_SUPPORTED

What do you suggest?

> 
> r~
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[Cornelia Huck]

On Thu, 7 Sep 2017 14:49:49 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 06.09.2017 23:20, Richard Henderson wrote:
> > On 09/06/2017 11:16 AM, Matthew Rosato wrote:  
> >> On 09/04/2017 11:43 AM, David Hildenbrand wrote:  
> >>> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the
> >>> guest tries to bring these CPUs up but fails), because we don't support
> >>> multiple CPUs on s390x under TCG.
> >>>
> >>> Let's bail out if more than 1 is specified, so we don't raise people's
> >>> hope. Make it a define, so we can easily bump it up later.
> >>>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>> ---  
> >>
> >> Makes sense.  Ran the described environment without this patch (errors)
> >> and again with this patch (graceful exit w/ message).
> >>
> >> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>  
> > 
> > Can someone review
> > 
> >   http://patchwork.ozlabs.org/patch/760010/
> >   
> 
> Yes, we also discovered that patch during review of v1.
> 
> > which does at least start to add the SIGP support.
> > 
> > Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N.  I don't
> > see the point of the define.
> >   
> 
> Conny requested a define. So it boils down to
> 
> a) no define just as in v1
> b) a define like S390_TCG_SMP_SUPPORTED
> 
> What do you suggest?

FWIW, I'd prefer b).

> 
> > 
> > r~
> >   
> 
> 

Re: [Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

On 06.09.2017 23:20, Richard Henderson wrote:
> On 09/06/2017 11:16 AM, Matthew Rosato wrote:
>> On 09/04/2017 11:43 AM, David Hildenbrand wrote:
>>> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the
>>> guest tries to bring these CPUs up but fails), because we don't support
>>> multiple CPUs on s390x under TCG.
>>>
>>> Let's bail out if more than 1 is specified, so we don't raise people's
>>> hope. Make it a define, so we can easily bump it up later.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>
>> Makes sense.  Ran the described environment without this patch (errors)
>> and again with this patch (graceful exit w/ message).
>>
>> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> 
> Can someone review
> 
>   http://patchwork.ozlabs.org/patch/760010/
> 
> which does at least start to add the SIGP support.

FWIW, I started factoring out today KVM SIGP code to make it usable by TCG.

I also started adding the missing SIGP instructions the kernel handles
for KVM. I dropped the old TCG SIGP handling code and completely reuse
the new SIGP code. I already got boot/reboot/shutdown  properly running
(implementing STOP and RESTART interrupts like KVM has).

But its still quite hacky and there are is a bunch of stuff to clean up,
especially:
- external interrupt handling (the queue approach we have right now is
  no good for external calls and emergency signals)
- floating interrupt support (io interrupts always going to CPU 0 is a
  hack)

I think I can at least implement SIGP properly and fix the external call
stuff. floating interrupts might require more thought.

Aurelien, please tell me if you are currently still working on this, so
we can coordinate.

Thanks!

> 
> Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N.  I don't
> see the point of the define.
> 
> 
> r~
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 11/19] s390x: allow only 1 CPU with TCG

[Cornelia Huck]

On Thu, 14 Sep 2017 20:13:49 +0200
David Hildenbrand <david@redhat.com> wrote:

> FWIW, I started factoring out today KVM SIGP code to make it usable by TCG.
> 
> I also started adding the missing SIGP instructions the kernel handles
> for KVM. I dropped the old TCG SIGP handling code and completely reuse
> the new SIGP code. I already got boot/reboot/shutdown  properly running
> (implementing STOP and RESTART interrupts like KVM has).

Cool!

> 
> But its still quite hacky and there are is a bunch of stuff to clean up,
> especially:
> - external interrupt handling (the queue approach we have right now is
>   no good for external calls and emergency signals)
> - floating interrupt support (io interrupts always going to CPU 0 is a
>   hack)
> 
> I think I can at least implement SIGP properly and fix the external call
> stuff. floating interrupts might require more thought.

I'm wondering whether something should move into the qemu version of
the flic. I'm also wondering whether we should look to the kvm floating
interrupt code for inspiration.

(I have 'look at floating interrupts with tcg' on my todo list as
well :)

[Qemu-devel] [PATCH v2 12/19] target/s390x: set cpu->id for linux user when realizing

[David Hildenbrand]

scc->next_cpu_id is updated when realizing. Setting it just before that
point looks cleaner.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 74b3e4fd0d..5f9315fb16 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -194,7 +194,11 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    ", max allowed: %d", cpu->id, max_cpus - 1);
         goto out;
     }
+#else
+    /* implicitly set for linux-user only */
+    cpu->id = scc->next_cpu_id;
 #endif
+
     if (cpu_exists(cpu->id)) {
         error_setg(&err, "Unable to add CPU: %" PRIi64
                    ", it already exists", cpu->id);
@@ -306,13 +310,6 @@ static void s390_cpu_initfn(Object *obj)
         inited = true;
         s390x_translate_init();
     }
-
-#if defined(CONFIG_USER_ONLY)
-    {
-        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
-        cpu->id = scc->next_cpu_id;
-    }
-#endif
 }
 
 static void s390_cpu_finalize(Object *obj)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 12/19] target/s390x: set cpu->id for linux user when realizing

[Matthew Rosato]

On 09/04/2017 11:43 AM, David Hildenbrand wrote:
> scc->next_cpu_id is updated when realizing. Setting it just before that
> point looks cleaner.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  target/s390x/cpu.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 74b3e4fd0d..5f9315fb16 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -194,7 +194,11 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>                     ", max allowed: %d", cpu->id, max_cpus - 1);
>          goto out;
>      }
> +#else
> +    /* implicitly set for linux-user only */
> +    cpu->id = scc->next_cpu_id;
>  #endif
> +
>      if (cpu_exists(cpu->id)) {
>          error_setg(&err, "Unable to add CPU: %" PRIi64
>                     ", it already exists", cpu->id);
> @@ -306,13 +310,6 @@ static void s390_cpu_initfn(Object *obj)
>          inited = true;
>          s390x_translate_init();
>      }
> -
> -#if defined(CONFIG_USER_ONLY)
> -    {
> -        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
> -        cpu->id = scc->next_cpu_id;
> -    }
> -#endif
>  }
> 
>  static void s390_cpu_finalize(Object *obj)
> 

[Qemu-devel] [PATCH v2 13/19] target/s390x: use "core-id" for cpu number/address/id handling

[David Hildenbrand]

Some time ago we discussed that using "id" as property name is not the
right thing to do, as it is a reserved property for other devices and
will not work with device_add.

Switch to the term "core-id" instead, and use it as an equivalent to
"CPU address" mentioned in the PoP. There is no such thing as cpu number,
so rename env.cpu_num to env.core_id. We use "core-id" as this is the
common term to use for device_add later on (x86 and ppc).

We can get rid of cpu->id now. Keep cpu_index and env->core_id in sync.
cpu_index was already implicitly used by e.g. cpu_exists(), so keeping
both in sync seems to be the right thing to do.

cpu_index will now no longer automatically get set via
cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
in sync.

Our new cpu property "core-id" can be a static property. Range checks can
be avoided by using the correct type and the "setting after realized"
check is done implicitly.

device_add will later need the reserved "id" property. Hotplugging a CPU
on s390x will then be: "device_add host-s390-cpu,id=cpu2,core-id=2".

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c |  2 +-
 target/s390x/cpu.c         | 71 +++++++++++++---------------------------------
 target/s390x/cpu.h         |  5 ++--
 target/s390x/cpu_models.c  |  2 +-
 target/s390x/excp_helper.c |  2 +-
 target/s390x/helper.c      |  4 +--
 target/s390x/misc_helper.c |  4 +--
 target/s390x/translate.c   |  5 +---
 8 files changed, 29 insertions(+), 66 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f7ca20d77a..22a8a1b45d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -311,7 +311,7 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     S390CPU *cpu = S390_CPU(dev);
     CPUState *cs = CPU(dev);
 
-    name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num);
+    name = g_strdup_printf("cpu[%i]", cpu->env.core_id);
     object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
                              errp);
     g_free(name);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 5f9315fb16..a2570bbc6b 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -36,6 +36,7 @@
 #include "trace.h"
 #include "qapi/visitor.h"
 #include "exec/exec-all.h"
+#include "hw/qdev-properties.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/hw.h"
 #include "sysemu/arch_init.h"
@@ -189,28 +190,30 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 #if !defined(CONFIG_USER_ONLY)
-    if (cpu->id >= max_cpus) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", max allowed: %d", cpu->id, max_cpus - 1);
+    if (cpu->env.core_id >= max_cpus) {
+        error_setg(&err, "Unable to add CPU: %" PRIu32 ", max allowed: %d",
+                   cpu->env.core_id, max_cpus - 1);
         goto out;
     }
 #else
     /* implicitly set for linux-user only */
-    cpu->id = scc->next_cpu_id;
+    cpu->env.core_id = scc->next_cpu_id;
 #endif
 
-    if (cpu_exists(cpu->id)) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", it already exists", cpu->id);
+    if (cpu_exists(cpu->env.core_id)) {
+        error_setg(&err, "Unable to add CPU: %" PRIu32 ", it already exists",
+                   cpu->env.core_id);
         goto out;
     }
-    if (cpu->id != scc->next_cpu_id) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", The next available id is %" PRIi64, cpu->id,
+    if (cpu->env.core_id != scc->next_cpu_id) {
+        error_setg(&err, "Unable to add CPU: %" PRIu32
+                   ", the next available nr is %" PRIi64, cpu->env.core_id,
                    scc->next_cpu_id);
         goto out;
     }
 
+    /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
+    cs->cpu_index = env->core_id;
     cpu_exec_realizefn(cs, &err);
     if (err != NULL) {
         goto out;
@@ -220,7 +223,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
-    env->cpu_num = cpu->id;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -241,45 +243,6 @@ out:
     error_propagate(errp, err);
 }
 
-static void s390x_cpu_get_id(Object *obj, Visitor *v, const char *name,
-                             void *opaque, Error **errp)
-{
-    S390CPU *cpu = S390_CPU(obj);
-    int64_t value = cpu->id;
-
-    visit_type_int(v, name, &value, errp);
-}
-
-static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
-                             void *opaque, Error **errp)
-{
-    S390CPU *cpu = S390_CPU(obj);
-    DeviceState *dev = DEVICE(obj);
-    const int64_t min = 0;
-    const int64_t max = UINT32_MAX;
-    Error *err = NULL;
-    int64_t value;
-
-    if (dev->realized) {
-        error_setg(errp, "Attempt to set property '%s' on '%s' after "
-                   "it was realized", name, object_get_typename(obj));
-        return;
-    }
-
-    visit_type_int(v, name, &value, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    if (value < min || value > max) {
-        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
-                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
-                   object_get_typename(obj), name, value, min, max);
-        return;
-    }
-    cpu->id = value;
-}
-
 static void s390_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -293,8 +256,6 @@ static void s390_cpu_initfn(Object *obj)
     cs->env_ptr = env;
     cs->halted = 1;
     cs->exception_index = EXCP_HLT;
-    object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
-                        s390x_cpu_set_id, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
     qemu_get_timedate(&tm, 0);
@@ -491,6 +452,11 @@ static gchar *s390_gdb_arch_name(CPUState *cs)
     return g_strdup("s390:64-bit");
 }
 
+static Property s390x_cpu_properties[] = {
+    DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void s390_cpu_class_init(ObjectClass *oc, void *data)
 {
     S390CPUClass *scc = S390_CPU_CLASS(oc);
@@ -500,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->next_cpu_id = 0;
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
+    dc->props = s390x_cpu_properties;
 
     scc->parent_reset = cc->reset;
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index dca6aa9aae..878b6cc83e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -149,7 +149,7 @@ typedef struct CPUS390XState {
 
     CPU_COMMON
 
-    uint32_t cpu_num;
+    uint32_t core_id; /* PoP "CPU address", same as cpu_index */
     uint64_t cpuid;
 
     uint64_t tod_offset;
@@ -193,7 +193,6 @@ typedef struct S390CPU {
     /*< public >*/
 
     CPUS390XState env;
-    int64_t id;
     S390CPUModel *model;
     /* needed for live migration */
     void *irqstate;
@@ -691,7 +690,7 @@ const char *s390_default_cpu_model_name(void);
 
 /* helper.c */
 #define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
-S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
+S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero
    is returned if the signal was handled by the virtual CPU.  */
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 18cbf91d9c..8e20e7637b 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -915,7 +915,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->env.cpuid = s390_cpuid_from_cpu_model(cpu->model);
     if (tcg_enabled()) {
         /* basic mode, write the cpu address into the first 4 bit of the ID */
-        cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.cpu_num);
+        cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.core_id);
     }
 }
 
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 14d3160e92..470cf8f5bc 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -250,7 +250,7 @@ static void do_ext_interrupt(CPUS390XState *env)
     lowcore->ext_params2 = cpu_to_be64(q->param64);
     lowcore->external_old_psw.mask = cpu_to_be64(get_psw_mask(env));
     lowcore->external_old_psw.addr = cpu_to_be64(env->psw.addr);
-    lowcore->cpu_addr = cpu_to_be16(env->cpu_num | VIRTIO_SUBCODE_64);
+    lowcore->cpu_addr = cpu_to_be16(env->core_id | VIRTIO_SUBCODE_64);
     mask = be64_to_cpu(lowcore->external_new_psw.mask);
     addr = be64_to_cpu(lowcore->external_new_psw.addr);
 
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index ba29504476..dfb24ef5b2 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -104,7 +104,7 @@ S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
     return S390_CPU(CPU(object_new(typename)));
 }
 
-S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp)
+S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp)
 {
     S390CPU *cpu;
     Error *err = NULL;
@@ -114,7 +114,7 @@ S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp)
         goto out;
     }
 
-    object_property_set_int(OBJECT(cpu), id, "id", &err);
+    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
     if (err != NULL) {
         goto out;
     }
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 5096286157..a3785a9c8d 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -231,7 +231,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
             /* XXX make different for different CPUs? */
             ebcdic_put(sysib.sequence, "QEMUQEMUQEMUQEMU", 16);
             ebcdic_put(sysib.plant, "QEMU", 4);
-            stw_p(&sysib.cpu_addr, env->cpu_num);
+            stw_p(&sysib.cpu_addr, env->core_id);
             cpu_physical_memory_write(a0, &sysib, sizeof(sysib));
         } else if ((sel1 == 2) && (sel2 == 2)) {
             /* Basic Machine CPUs */
@@ -259,7 +259,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
                 /* XXX make different for different CPUs? */
                 ebcdic_put(sysib.sequence, "QEMUQEMUQEMUQEMU", 16);
                 ebcdic_put(sysib.plant, "QEMU", 4);
-                stw_p(&sysib.cpu_addr, env->cpu_num);
+                stw_p(&sysib.cpu_addr, env->core_id);
                 stw_p(&sysib.cpu_id, 0);
                 cpu_physical_memory_write(a0, &sysib, sizeof(sysib));
             } else if ((sel1 == 2) && (sel2 == 2)) {
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4b0db7b7bd..b8963f2fe2 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)
 static ExitStatus op_stap(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    /* ??? Surely cpu address != cpu number.  In any case the previous
-       version of this stored more than the required half-word, so it
-       is unlikely this has ever been tested.  */
-    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
+    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));
     return NO_EXIT;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use "core-id" for cpu number/address/id handling

[Matthew Rosato]

On 09/04/2017 11:43 AM, David Hildenbrand wrote:
> Some time ago we discussed that using "id" as property name is not the
> right thing to do, as it is a reserved property for other devices and
> will not work with device_add.
> 
> Switch to the term "core-id" instead, and use it as an equivalent to
> "CPU address" mentioned in the PoP. There is no such thing as cpu number,
> so rename env.cpu_num to env.core_id. We use "core-id" as this is the
> common term to use for device_add later on (x86 and ppc).
> 
> We can get rid of cpu->id now. Keep cpu_index and env->core_id in sync.
> cpu_index was already implicitly used by e.g. cpu_exists(), so keeping
> both in sync seems to be the right thing to do.
> 

Rename & removal of 'id' is fine w/ me - in retrospect, it's a shame we
used 'id' in the first place.

> cpu_index will now no longer automatically get set via
> cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
> in sync.>

Good catch.

> Our new cpu property "core-id" can be a static property. Range checks can
> be avoided by using the correct type and the "setting after realized"
> check is done implicitly.
> 
> device_add will later need the reserved "id" property. Hotplugging a CPU
> on s390x will then be: "device_add host-s390-cpu,id=cpu2,core-id=2".
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

[...]

> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4b0db7b7bd..b8963f2fe2 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)
>  static ExitStatus op_stap(DisasContext *s, DisasOps *o)
>  {
>      check_privileged(s);
> -    /* ??? Surely cpu address != cpu number.  In any case the previous
> -       version of this stored more than the required half-word, so it
> -       is unlikely this has ever been tested.  */
> -    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
> +    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));
>      return NO_EXIT;
>  }
> 

Are you sure it's OK to remove this blurb in its entirety?  You are
certainly collapsing the various CPU identifiers, but you aren't
changing the size of the store from when this blurb was put in
(411fea3d) So, "the previous version of this stored more than the
required half-word" seems to be still relevant -- Unless you've gone
ahead and tested it out?

Outside of that nit, I like the changes.

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use "core-id" for cpu number/address/id handling

[David Hildenbrand]

>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
>> index 4b0db7b7bd..b8963f2fe2 100644
>> --- a/target/s390x/translate.c
>> +++ b/target/s390x/translate.c
>> @@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)
>>  static ExitStatus op_stap(DisasContext *s, DisasOps *o)
>>  {
>>      check_privileged(s);
>> -    /* ??? Surely cpu address != cpu number.  In any case the previous
>> -       version of this stored more than the required half-word, so it
>> -       is unlikely this has ever been tested.  */
>> -    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
>> +    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));
>>      return NO_EXIT;
>>  }
>>
> 
> Are you sure it's OK to remove this blurb in its entirety?  You are
> certainly collapsing the various CPU identifiers, but you aren't
> changing the size of the store from when this blurb was put in
> (411fea3d) So, "the previous version of this stored more than the
> required half-word" seems to be still relevant -- Unless you've gone
> ahead and tested it out?

z13 PoP (10-132):

"The CPU address by which this CPU is identified in a
multiprocessing configuration is stored at the half-
word location designated by the second-operand
address."

As far as I understand this comment, 411fea3d fixed the store to only be
the required half-word, no?

The previous version stored 32bit:
  tcg_gen_qemu_st32(tmp2, tmp, get_mem_index(s));

Now we have:
C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16, stap, 0)

which, due to m1_16, will use wout_m1_16
  tcg_gen_qemu_st16(o->out, o->addr1, get_mem_index(s));

So what's left is the confusion about num vs address. But core-id is
certainly the CPU number, which is t be stored. There is no such thing
as CPU number.

So I think this comment can now safely be dropped.

We have a kvm-unit-test for STAP, but we only check if anything has been
stored, not if "too much" has been stored. But I am not sure if we want
such checks, as the number of tests will explode. Usually, if it would
be broken, other things would then go wrong in our unit tests. I will
have a look.

> 
> Outside of that nit, I like the changes.
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> 


-- 

Thanks,

David

[Qemu-devel] [PATCH v2 14/19] target/s390x: rename next_cpu_id to next_core_id

[David Hildenbrand]

Adapt to the new term "core_id". While at it, fix the type and drop the
initialization to 0 (which is superfluous).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index c8fbf8ae03..21ea15e642 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -52,7 +52,7 @@ typedef struct S390CPUClass {
     bool is_migration_safe;
     const char *desc;
 
-    int64_t next_cpu_id;
+    uint32_t next_core_id;
 
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index a2570bbc6b..105ff13034 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -197,7 +197,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #else
     /* implicitly set for linux-user only */
-    cpu->env.core_id = scc->next_cpu_id;
+    cpu->env.core_id = scc->next_core_id;
 #endif
 
     if (cpu_exists(cpu->env.core_id)) {
@@ -205,10 +205,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    cpu->env.core_id);
         goto out;
     }
-    if (cpu->env.core_id != scc->next_cpu_id) {
+    if (cpu->env.core_id != scc->next_core_id) {
         error_setg(&err, "Unable to add CPU: %" PRIu32
-                   ", the next available nr is %" PRIi64, cpu->env.core_id,
-                   scc->next_cpu_id);
+                   ", the next available id is %" PRIu32, cpu->env.core_id,
+                   scc->next_core_id);
         goto out;
     }
 
@@ -218,7 +218,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     if (err != NULL) {
         goto out;
     }
-    scc->next_cpu_id++;
+    scc->next_core_id++;
 
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
@@ -463,7 +463,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(scc);
     DeviceClass *dc = DEVICE_CLASS(oc);
 
-    scc->next_cpu_id = 0;
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 14/19] target/s390x: rename next_cpu_id to next_core_id

[Matthew Rosato]

On 09/04/2017 11:43 AM, David Hildenbrand wrote:
> Adapt to the new term "core_id". While at it, fix the type and drop the
> initialization to 0 (which is superfluous).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Given the prior patch, this rename makes sense.

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  target/s390x/cpu-qom.h |  2 +-
>  target/s390x/cpu.c     | 11 +++++------
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
> index c8fbf8ae03..21ea15e642 100644
> --- a/target/s390x/cpu-qom.h
> +++ b/target/s390x/cpu-qom.h
> @@ -52,7 +52,7 @@ typedef struct S390CPUClass {
>      bool is_migration_safe;
>      const char *desc;
> 
> -    int64_t next_cpu_id;
> +    uint32_t next_core_id;
> 
>      DeviceRealize parent_realize;
>      void (*parent_reset)(CPUState *cpu);
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index a2570bbc6b..105ff13034 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -197,7 +197,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  #else
>      /* implicitly set for linux-user only */
> -    cpu->env.core_id = scc->next_cpu_id;
> +    cpu->env.core_id = scc->next_core_id;
>  #endif
> 
>      if (cpu_exists(cpu->env.core_id)) {
> @@ -205,10 +205,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>                     cpu->env.core_id);
>          goto out;
>      }
> -    if (cpu->env.core_id != scc->next_cpu_id) {
> +    if (cpu->env.core_id != scc->next_core_id) {
>          error_setg(&err, "Unable to add CPU: %" PRIu32
> -                   ", the next available nr is %" PRIi64, cpu->env.core_id,
> -                   scc->next_cpu_id);
> +                   ", the next available id is %" PRIu32, cpu->env.core_id,
> +                   scc->next_core_id);
>          goto out;
>      }
> 
> @@ -218,7 +218,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (err != NULL) {
>          goto out;
>      }
> -    scc->next_cpu_id++;
> +    scc->next_core_id++;
> 
>  #if !defined(CONFIG_USER_ONLY)
>      qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> @@ -463,7 +463,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      CPUClass *cc = CPU_CLASS(scc);
>      DeviceClass *dc = DEVICE_CLASS(oc);
> 
> -    scc->next_cpu_id = 0;
>      scc->parent_realize = dc->realize;
>      dc->realize = s390_cpu_realizefn;
>      dc->props = s390x_cpu_properties;
> 

[Qemu-devel] [PATCH v2 15/19] s390x: print CPU definitions in sorted order

[David Hildenbrand]

Other architectures provide nicely sorted lists, let's do it similarly on
s390x.

While at it, clean up the code we have to touch either way.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 63 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 8e20e7637b..092db34258 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -270,41 +270,60 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
     return last_compatible;
 }
 
-struct S390PrintCpuListInfo {
-    FILE *f;
-    fprintf_function print;
-};
-
-static void print_cpu_model_list(ObjectClass *klass, void *opaque)
+static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data)
 {
-    struct S390PrintCpuListInfo *info = opaque;
-    S390CPUClass *scc = S390_CPU_CLASS(klass);
-    char *name = g_strdup(object_class_get_name(klass));
-    const char *details = "";
-
-    if (scc->is_static) {
-        details = "(static, migration-safe)";
-    } else if (scc->is_migration_safe) {
-        details = "(migration-safe)";
-    }
+    CPUListState *s = user_data;
+    const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data);
+    char *name = g_strdup(object_class_get_name((ObjectClass *)data));
 
     /* strip off the -s390-cpu */
     g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
-    (*info->print)(info->f, "s390 %-15s %-35s %s\n", name, scc->desc,
-                   details);
+    (*s->cpu_fprintf)(s->file, "s390 %-15s %-35s (%smigration-safe)\n", name,
+                      scc->desc, scc->is_static ? "static, " : "");
     g_free(name);
 }
 
+static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    const S390CPUClass *cc_a = S390_CPU_CLASS((ObjectClass *)a);
+    const S390CPUClass *cc_b = S390_CPU_CLASS((ObjectClass *)b);
+    const char *name_a = object_class_get_name((ObjectClass *)a);
+    const char *name_b = object_class_get_name((ObjectClass *)b);
+
+    /* move qemu and host to the top of the list, qemu first, host second */
+    if (name_a[0] == 'q') {
+        return -1;
+    } else if (name_b[0] == 'q') {
+        return 1;
+    } else if (name_a[0] == 'h') {
+        return -1;
+    } else if (name_b[0] == 'h') {
+        return 1;
+    }
+
+    /* keep the same order we have in our table (sorted by release date) */
+    if (cc_a->cpu_def != cc_b->cpu_def) {
+        return cc_a->cpu_def - cc_b->cpu_def;
+    }
+
+    /* exact same definition - list base model first */
+    return cc_a->is_static ? -1 : 1;
+}
+
 void s390_cpu_list(FILE *f, fprintf_function print)
 {
-    struct S390PrintCpuListInfo info = {
-        .f = f,
-        .print = print,
+    CPUListState s = {
+        .file = f,
+        .cpu_fprintf = print,
     };
     S390FeatGroup group;
     S390Feat feat;
+    GSList *list;
 
-    object_class_foreach(print_cpu_model_list, TYPE_S390_CPU, false, &info);
+    list = object_class_get_list(TYPE_S390_CPU, false);
+    list = g_slist_sort(list, s390_cpu_list_compare);
+    g_slist_foreach(list, s390_print_cpu_model_list_entry, &s);
+    g_slist_free(list);
 
     (*print)(f, "\nRecognized feature flags:\n");
     for (feat = 0; feat < S390_FEAT_MAX; feat++) {
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 15/19] s390x: print CPU definitions in sorted order

[Cornelia Huck]

On Mon,  4 Sep 2017 17:43:12 +0200
David Hildenbrand <david@redhat.com> wrote:

> Other architectures provide nicely sorted lists, let's do it similarly on
> s390x.
> 
> While at it, clean up the code we have to touch either way.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu_models.c | 63 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 8e20e7637b..092db34258 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -270,41 +270,60 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
>      return last_compatible;
>  }
>  
> -struct S390PrintCpuListInfo {
> -    FILE *f;
> -    fprintf_function print;
> -};
> -
> -static void print_cpu_model_list(ObjectClass *klass, void *opaque)
> +static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data)
>  {
> -    struct S390PrintCpuListInfo *info = opaque;
> -    S390CPUClass *scc = S390_CPU_CLASS(klass);
> -    char *name = g_strdup(object_class_get_name(klass));
> -    const char *details = "";
> -
> -    if (scc->is_static) {
> -        details = "(static, migration-safe)";
> -    } else if (scc->is_migration_safe) {
> -        details = "(migration-safe)";
> -    }
> +    CPUListState *s = user_data;
> +    const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data);
> +    char *name = g_strdup(object_class_get_name((ObjectClass *)data));
>  
>      /* strip off the -s390-cpu */
>      g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
> -    (*info->print)(info->f, "s390 %-15s %-35s %s\n", name, scc->desc,
> -                   details);
> +    (*s->cpu_fprintf)(s->file, "s390 %-15s %-35s (%smigration-safe)\n", name,
> +                      scc->desc, scc->is_static ? "static, " : "");

Hm, that now prints 's390 host' as 'migration safe'...

>      g_free(name);
>  }
>  
> +static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b)
> +{
> +    const S390CPUClass *cc_a = S390_CPU_CLASS((ObjectClass *)a);
> +    const S390CPUClass *cc_b = S390_CPU_CLASS((ObjectClass *)b);
> +    const char *name_a = object_class_get_name((ObjectClass *)a);
> +    const char *name_b = object_class_get_name((ObjectClass *)b);
> +
> +    /* move qemu and host to the top of the list, qemu first, host second */
> +    if (name_a[0] == 'q') {
> +        return -1;
> +    } else if (name_b[0] == 'q') {
> +        return 1;
> +    } else if (name_a[0] == 'h') {
> +        return -1;
> +    } else if (name_b[0] == 'h') {
> +        return 1;
> +    }

Are we sure that there will never be models starting with q or h? But
we can worry about that later.

> +
> +    /* keep the same order we have in our table (sorted by release date) */
> +    if (cc_a->cpu_def != cc_b->cpu_def) {
> +        return cc_a->cpu_def - cc_b->cpu_def;
> +    }
> +
> +    /* exact same definition - list base model first */
> +    return cc_a->is_static ? -1 : 1;
> +}
> +
>  void s390_cpu_list(FILE *f, fprintf_function print)
>  {
> -    struct S390PrintCpuListInfo info = {
> -        .f = f,
> -        .print = print,
> +    CPUListState s = {
> +        .file = f,
> +        .cpu_fprintf = print,
>      };
>      S390FeatGroup group;
>      S390Feat feat;
> +    GSList *list;
>  
> -    object_class_foreach(print_cpu_model_list, TYPE_S390_CPU, false, &info);
> +    list = object_class_get_list(TYPE_S390_CPU, false);
> +    list = g_slist_sort(list, s390_cpu_list_compare);
> +    g_slist_foreach(list, s390_print_cpu_model_list_entry, &s);
> +    g_slist_free(list);
>  
>      (*print)(f, "\nRecognized feature flags:\n");
>      for (feat = 0; feat < S390_FEAT_MAX; feat++) {

Sorting looks good.

Re: [Qemu-devel] [PATCH v2 15/19] s390x: print CPU definitions in sorted order

[David Hildenbrand]

>> +    CPUListState *s = user_data;
>> +    const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data);
>> +    char *name = g_strdup(object_class_get_name((ObjectClass *)data));
>>  
>>      /* strip off the -s390-cpu */
>>      g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
>> -    (*info->print)(info->f, "s390 %-15s %-35s %s\n", name, scc->desc,
>> -                   details);
>> +    (*s->cpu_fprintf)(s->file, "s390 %-15s %-35s (%smigration-safe)\n", name,
>> +                      scc->desc, scc->is_static ? "static, " : "");
> 
> Hm, that now prints 's390 host' as 'migration safe'...

Very good point, will fix, thanks!

> 
>>      g_free(name);
>>  }
>>  
>> +static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b)
>> +{
>> +    const S390CPUClass *cc_a = S390_CPU_CLASS((ObjectClass *)a);
>> +    const S390CPUClass *cc_b = S390_CPU_CLASS((ObjectClass *)b);
>> +    const char *name_a = object_class_get_name((ObjectClass *)a);
>> +    const char *name_b = object_class_get_name((ObjectClass *)b);
>> +
>> +    /* move qemu and host to the top of the list, qemu first, host second */
>> +    if (name_a[0] == 'q') {
>> +        return -1;
>> +    } else if (name_b[0] == 'q') {
>> +        return 1;
>> +    } else if (name_a[0] == 'h') {
>> +        return -1;
>> +    } else if (name_b[0] == 'h') {
>> +        return 1;
>> +    }
> 
> Are we sure that there will never be models starting with q or h? But
> we can worry about that later.

Could be but unlikely. In the worst case sorting would be wrong, I think
we can live with that :)


-- 

Thanks,

David

[Qemu-devel] [PATCH v2 16/19] s390x: allow cpu hotplug via device_add

[David Hildenbrand]

E.g. the following now works:
    device_add host-s390-cpu,id=cpu1,core-id=1

The system will perform the same checks as when using cpu_add:
- If the core_id is already in use
- If the next sequential core_id isn't used
- If core-id >= max_cpu is specified

In addition, mixed CPU models are checked. E.g. if starting with
-cpu host and trying to hotplug "qemu-s390-cpu":
    "Mixed CPU models are not supported on s390x."

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 105ff13034..be20c2fb0f 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -466,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
     dc->props = s390x_cpu_properties;
+    dc->user_creatable = true;
 
     scc->parent_reset = cc->reset;
 #if !defined(CONFIG_USER_ONLY)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 16/19] s390x: allow cpu hotplug via device_add

[Matthew Rosato]

On 09/04/2017 11:43 AM, David Hildenbrand wrote:
> E.g. the following now works:
>     device_add host-s390-cpu,id=cpu1,core-id=1
> 
> The system will perform the same checks as when using cpu_add:
> - If the core_id is already in use
> - If the next sequential core_id isn't used
> - If core-id >= max_cpu is specified
> 
> In addition, mixed CPU models are checked. E.g. if starting with
> -cpu host and trying to hotplug "qemu-s390-cpu":
>     "Mixed CPU models are not supported on s390x."
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

As easy as flipping a switch, right?

Tested the new device_add path, the old cpu-add path and mixed cases
where I alternated adding cpus via both methods -- looking good.

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  target/s390x/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 105ff13034..be20c2fb0f 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -466,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      scc->parent_realize = dc->realize;
>      dc->realize = s390_cpu_realizefn;
>      dc->props = s390x_cpu_properties;
> +    dc->user_creatable = true;
> 
>      scc->parent_reset = cc->reset;
>  #if !defined(CONFIG_USER_ONLY)
> 

Re: [Qemu-devel] [PATCH v2 16/19] s390x: allow cpu hotplug via device_add

[David Hildenbrand]

On 07.09.2017 05:16, Matthew Rosato wrote:
> On 09/04/2017 11:43 AM, David Hildenbrand wrote:
>> E.g. the following now works:
>>     device_add host-s390-cpu,id=cpu1,core-id=1
>>
>> The system will perform the same checks as when using cpu_add:
>> - If the core_id is already in use
>> - If the next sequential core_id isn't used
>> - If core-id >= max_cpu is specified
>>
>> In addition, mixed CPU models are checked. E.g. if starting with
>> -cpu host and trying to hotplug "qemu-s390-cpu":
>>     "Mixed CPU models are not supported on s390x."
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> As easy as flipping a switch, right?

Yes, luckily all that hotplug handling already was in place, just
core-id vs. id had to be fixed.

> 
> Tested the new device_add path, the old cpu-add path and mixed cases
> where I alternated adding cpus via both methods -- looking good.
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> 

Thanks!

-- 

Thanks,

David

[Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[David Hildenbrand]

device_del on a CPU will currently do nothing. Let's emmit an error
telling that this is will never work (there is no architecture support
on s390x). Error message copied from ppc.

(qemu) device_del cpu1
device_del cpu1
CPU hot unplug not supported on this machine

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 22a8a1b45d..dd149567bb 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
+                                               DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        error_setg(errp, "CPU hot unplug not supported on this machine");
+        return;
+    }
+}
+
 static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
                                                 DeviceState *dev)
 {
@@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = 248;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
     hc->plug = s390_machine_device_plug;
+    hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[Christian Borntraeger]

On 09/04/2017 05:43 PM, David Hildenbrand wrote:
> device_del on a CPU will currently do nothing. Let's emmit an error
> telling that this is will never work (there is no architecture support
> on s390x). Error message copied from ppc.
> 
> (qemu) device_del cpu1
> device_del cpu1
> CPU hot unplug not supported on this machine

Given the fact that I get the question about unplug _every_ time when I give a presentation
about KVM on z, I will try to get some architecture folks look at this. Maybe we can define
something very simple like "if the CPU is in the stopped state we can remove this and just
piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".

So maybe add  "currently"


> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 22a8a1b45d..dd149567bb 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>      }
>  }
> 
> +static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> +                                               DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        error_setg(errp, "CPU hot unplug not supported on this machine");
> +        return;
> +    }
> +}
> +
>  static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>                                                  DeviceState *dev)
>  {
> @@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = 248;
>      mc->get_hotplug_handler = s390_get_hotplug_handler;
>      hc->plug = s390_machine_device_plug;
> +    hc->unplug_request = s390_machine_device_unplug_request;
>      nc->nmi_monitor_handler = s390_nmi;
>  }
> 

Re: [Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[David Hildenbrand]

On 05.09.2017 11:14, Christian Borntraeger wrote:
> On 09/04/2017 05:43 PM, David Hildenbrand wrote:
>> device_del on a CPU will currently do nothing. Let's emmit an error
>> telling that this is will never work (there is no architecture support
>> on s390x). Error message copied from ppc.
>>
>> (qemu) device_del cpu1
>> device_del cpu1
>> CPU hot unplug not supported on this machine
> 
> Given the fact that I get the question about unplug _every_ time when I give a presentation
> about KVM on z, I will try to get some architecture folks look at this. Maybe we can define
> something very simple like "if the CPU is in the stopped state we can remove this and just
> piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".
> 
> So maybe add  "currently"

Unfortunately it might not be that easy.

We would have to find a way that existing OS's don't break. If a guest
OS is not prepared for CPUs to get removed, we might run into
inconsistencies when simply deleting CPUs that are in the STOPPED state.

Especially, these CPUs would still show up in the guest as "offline".
Wonder what would then happen trying to "online" these.

But yes, I can add "currently".

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[Christian Borntraeger]

On 09/05/2017 02:01 PM, David Hildenbrand wrote:
> On 05.09.2017 11:14, Christian Borntraeger wrote:
>> On 09/04/2017 05:43 PM, David Hildenbrand wrote:
>>> device_del on a CPU will currently do nothing. Let's emmit an error
>>> telling that this is will never work (there is no architecture support
>>> on s390x). Error message copied from ppc.
>>>
>>> (qemu) device_del cpu1
>>> device_del cpu1
>>> CPU hot unplug not supported on this machine
>>
>> Given the fact that I get the question about unplug _every_ time when I give a presentation
>> about KVM on z, I will try to get some architecture folks look at this. Maybe we can define
>> something very simple like "if the CPU is in the stopped state we can remove this and just
>> piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".
>>
>> So maybe add  "currently"
> 
> Unfortunately it might not be that easy.
> 
> We would have to find a way that existing OS's don't break. If a guest
> OS is not prepared for CPUs to get removed, we might run into
> inconsistencies when simply deleting CPUs that are in the STOPPED state.

Yes, this needs to be validated across all things.
> 
> Especially, these CPUs would still show up in the guest as "offline".
> Wonder what would then happen trying to "online" these.

The main use case seems to be, that the admin does not want to allow a guest
to online back a guest CPU if it was taken away from the configuration. 
So maybe we could simply fail a SIGP START for those.

Or we might go one level below and only allow an unplug if the CPU is in
the deconfigured state and we would then have to forbid the configuration
step. Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in
QEMU.

Anyway, we need some architecture agreement here first (something that is
also ok with LPAR and z/VM).


> But yes, I can add "currently".
> 

Re: [Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[Cornelia Huck]

On Tue, 5 Sep 2017 14:14:21 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 09/05/2017 02:01 PM, David Hildenbrand wrote:
> > On 05.09.2017 11:14, Christian Borntraeger wrote:  
> >> On 09/04/2017 05:43 PM, David Hildenbrand wrote:  
> >>> device_del on a CPU will currently do nothing. Let's emmit an error
> >>> telling that this is will never work (there is no architecture support
> >>> on s390x). Error message copied from ppc.
> >>>
> >>> (qemu) device_del cpu1
> >>> device_del cpu1
> >>> CPU hot unplug not supported on this machine  
> >>
> >> Given the fact that I get the question about unplug _every_ time when I give a presentation
> >> about KVM on z, I will try to get some architecture folks look at this. Maybe we can define
> >> something very simple like "if the CPU is in the stopped state we can remove this and just
> >> piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification".
> >>
> >> So maybe add  "currently"  
> > 
> > Unfortunately it might not be that easy.
> > 
> > We would have to find a way that existing OS's don't break. If a guest
> > OS is not prepared for CPUs to get removed, we might run into
> > inconsistencies when simply deleting CPUs that are in the STOPPED state.  
> 
> Yes, this needs to be validated across all things.
> > 
> > Especially, these CPUs would still show up in the guest as "offline".
> > Wonder what would then happen trying to "online" these.  
> 
> The main use case seems to be, that the admin does not want to allow a guest
> to online back a guest CPU if it was taken away from the configuration. 
> So maybe we could simply fail a SIGP START for those.
> 
> Or we might go one level below and only allow an unplug if the CPU is in
> the deconfigured state and we would then have to forbid the configuration
> step.

Having the cpu in the unconfigured state as a requirement also makes
this less likely to break for older OSs, I guess.

> Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in
> QEMU.

Sounds like something we'd like to have in the future?

(Btw, what does cpuplugd trigger? Offline/online or
deconfigure/configure?)

> 
> Anyway, we need some architecture agreement here first (something that is
> also ok with LPAR and z/VM).

Certainly.

> > But yes, I can add "currently".

Yes, that makes sense.

Re: [Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[Christian Borntraeger]

On 09/05/2017 02:54 PM, Cornelia Huck wrote:
[...]
> Having the cpu in the unconfigured state as a requirement also makes
> this less likely to break for older OSs, I guess.
> 
>> Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in
>> QEMU.
> 
> Sounds like something we'd like to have in the future?
> 
> (Btw, what does cpuplugd trigger? Offline/online or
> deconfigure/configure?)

offline/online via sigp stop/start. When we hotplug we already
provide the new cpu in the configured state (like z/VM).

Re: [Qemu-devel] [PATCH v2 17/19] s390x: CPU hot unplug via device_del cannot work

[Matthew Rosato]

On 09/04/2017 11:43 AM, David Hildenbrand wrote:
> device_del on a CPU will currently do nothing. Let's emmit an error
> telling that this is will never work (there is no architecture support
> on s390x). Error message copied from ppc.
> 
> (qemu) device_del cpu1
> device_del cpu1
> CPU hot unplug not supported on this machine
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Architecture discussions aside, this code will work as-is to prevent CPU
unplug.  Add "currently" as Christian suggests and:

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  hw/s390x/s390-virtio-ccw.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 22a8a1b45d..dd149567bb 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>      }
>  }
> 
> +static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> +                                               DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        error_setg(errp, "CPU hot unplug not supported on this machine");
> +        return;
> +    }
> +}
> +
>  static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>                                                  DeviceState *dev)
>  {
> @@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = 248;
>      mc->get_hotplug_handler = s390_get_hotplug_handler;
>      hc->plug = s390_machine_device_plug;
> +    hc->unplug_request = s390_machine_device_unplug_request;
>      nc->nmi_monitor_handler = s390_nmi;
>  }
> 

[Qemu-devel] [PATCH v2 18/19] s390x: implement query-hotpluggable-cpus

[David Hildenbrand]

CPU hotplug is only possible on a per core basis on s390x.

As we now have ms->possible_cpus, we can get rid of the global variable
cpu_states.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 90 +++++++++++++++++++++++++++++++++-------------
 qapi-schema.json           | 16 +++++++++
 2 files changed, 81 insertions(+), 25 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index dd149567bb..458615252c 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -36,22 +36,40 @@
 #include "qapi/qmp/qerror.h"
 #include "hw/nmi.h"
 
-static S390CPU **cpu_states;
+static CPUArchId *s390_find_cpu_slot(MachineState *ms, uint32_t core_id,
+                                     int *idx)
+{
+    if (core_id >= ms->possible_cpus->len) {
+        return NULL;
+    }
+    /* core_id corresponds to the index */
+    if (idx) {
+        *idx = core_id;
+    }
+    return &ms->possible_cpus->cpus[core_id];
+}
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
-    if (cpu_addr >= max_cpus) {
-        return NULL;
+    static MachineState *ms;
+    CPUArchId *found;
+
+    if (!ms) {
+        ms = MACHINE(qdev_get_machine());
     }
 
-    /* Fast lookup via CPU ID */
-    return cpu_states[cpu_addr];
+    /* CPU address corresponds to core_id */
+    found = s390_find_cpu_slot(ms, cpu_addr, NULL);
+    if (!found) {
+        return NULL;
+    }
+    return S390_CPU(found->cpu);
 }
 
 static void s390_init_cpus(MachineState *machine)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     int i;
-    gchar *name;
 
     if (machine->cpu_model == NULL) {
         machine->cpu_model = s390_default_cpu_model_name();
@@ -63,17 +81,8 @@ static void s390_init_cpus(MachineState *machine)
         exit(1);
     }
 
-    cpu_states = g_new0(S390CPU *, max_cpus);
-
-    for (i = 0; i < max_cpus; i++) {
-        name = g_strdup_printf("cpu[%i]", i);
-        object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
-                                 (Object **) &cpu_states[i],
-                                 object_property_allow_set_link,
-                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
-                                 &error_abort);
-        g_free(name);
-    }
+    /* initialize possible_cpus */
+    mc->possible_cpu_arch_ids(machine);
 
     for (i = 0; i < smp_cpus; i++) {
         s390x_new_cpu(machine->cpu_model, i, &error_fatal);
@@ -304,17 +313,14 @@ static void ccw_init(MachineState *machine)
     register_savevm_live(NULL, "todclock", 0, 1, &savevm_gtod, NULL);
 }
 
-static void s390_cpu_plug(HotplugHandler *hotplug_dev,
-                        DeviceState *dev, Error **errp)
+static void s390_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                          Error **errp)
 {
-    gchar *name;
+    MachineState *ms = MACHINE(hotplug_dev);
     S390CPU *cpu = S390_CPU(dev);
-    CPUState *cs = CPU(dev);
 
-    name = g_strdup_printf("cpu[%i]", cpu->env.core_id);
-    object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
-                             errp);
-    g_free(name);
+    g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
+    ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
 }
 
 static void s390_machine_reset(void)
@@ -347,6 +353,37 @@ static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
     }
 }
 
+static CpuInstanceProperties s390_cpu_index_to_props(MachineState *machine,
+                                                     unsigned cpu_index)
+{
+    CPUArchId *slot = s390_find_cpu_slot(machine, cpu_index, NULL);
+
+    assert(slot);
+    return slot->props;
+}
+
+static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
+{
+    int i;
+
+    if (ms->possible_cpus) {
+        g_assert(ms->possible_cpus && ms->possible_cpus->len == max_cpus);
+        return ms->possible_cpus;
+    }
+
+    ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
+                                  sizeof(CPUArchId) * max_cpus);
+    ms->possible_cpus->len = max_cpus;
+    for (i = 0; i < ms->possible_cpus->len; i++) {
+        ms->possible_cpus->cpus[i].vcpus_count = 1;
+        ms->possible_cpus->cpus[i].arch_id = i;
+        ms->possible_cpus->cpus[i].props.has_core_id = true;
+        ms->possible_cpus->cpus[i].props.core_id = i;
+    }
+
+    return ms->possible_cpus;
+}
+
 static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
                                                 DeviceState *dev)
 {
@@ -394,7 +431,10 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->no_sdcard = 1;
     mc->use_sclp = 1;
     mc->max_cpus = 248;
+    mc->has_hotpluggable_cpus = true;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
+    mc->cpu_index_to_instance_props = s390_cpu_index_to_props;
+    mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
diff --git a/qapi-schema.json b/qapi-schema.json
index f3af2cb851..79e9f85404 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3121,6 +3121,22 @@
 #      }
 #    ]}
 #
+# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2 -cpu qemu
+# (Since: 2.11):
+#
+# -> { "execute": "query-hotpluggable-cpus" }
+# <- {"return": [
+#      {
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 1 }
+#      },
+#      {
+#         "qom-path": "/machine/unattached/device[0]",
+#         "type": "qemu-s390-cpu", "vcpus-count": 1,
+#         "props": { "core-id": 0 }
+#      }
+#    ]}
+#
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 18/19] s390x: implement query-hotpluggable-cpus

[Cornelia Huck]

On Mon,  4 Sep 2017 17:43:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> CPU hotplug is only possible on a per core basis on s390x.
> 
> As we now have ms->possible_cpus, we can get rid of the global variable
> cpu_states.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 90 +++++++++++++++++++++++++++++++++-------------
>  qapi-schema.json           | 16 +++++++++
>  2 files changed, 81 insertions(+), 25 deletions(-)

LGTM

[Qemu-devel] [PATCH v2 19/19] s390x: get rid of cpu_s390x_create()

[David Hildenbrand]

Now that there is only one user of cpu_s390x_create() left, make cpu
creation looks like on x86.
- Perform the model/properties split and checks in s390_init_cpus()
- Parse features only once without having to remember if already parsed
- Pass only the typename to s390x_new_cpu()
- Use the typename of an existing CPU for hotplug via cpu-add

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 29 +++++++++++++++++++++++++++--
 target/s390x/cpu.h         |  2 +-
 target/s390x/helper.c      | 45 ++-------------------------------------------
 target/s390x/internal.h    |  1 -
 4 files changed, 30 insertions(+), 47 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 458615252c..8bd063377c 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -69,6 +69,10 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 static void s390_init_cpus(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    const char *typename;
+    gchar **model_pieces;
+    ObjectClass *oc;
+    CPUClass *cc;
     int i;
 
     if (machine->cpu_model == NULL) {
@@ -84,8 +88,25 @@ static void s390_init_cpus(MachineState *machine)
     /* initialize possible_cpus */
     mc->possible_cpu_arch_ids(machine);
 
+    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        exit(1);
+    }
+
+    oc = cpu_class_by_name(TYPE_S390_CPU, model_pieces[0]);
+    if (!oc) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        exit(1);
+    }
+    typename = object_class_get_name(oc);
+    cc = CPU_CLASS(oc);
+    /* after parsing, properties will be applied to all *typename* instances */
+    cc->parse_features(typename, model_pieces[1], &error_fatal);
+    g_strfreev(model_pieces);
+
     for (i = 0; i < smp_cpus; i++) {
-        s390x_new_cpu(machine->cpu_model, i, &error_fatal);
+        s390x_new_cpu(typename, i, &error_fatal);
     }
 }
 
@@ -396,8 +417,12 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
 static void s390_hot_add_cpu(const int64_t id, Error **errp)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
+    ObjectClass *oc;
+
+    g_assert(machine->possible_cpus->cpus[0].cpu);
+    oc = OBJECT_CLASS(CPU_GET_CLASS(machine->possible_cpus->cpus[0].cpu));
 
-    s390x_new_cpu(machine->cpu_model, id, errp);
+    s390x_new_cpu(object_class_get_name(oc), id, errp);
 }
 
 static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 878b6cc83e..4c639240de 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -690,7 +690,7 @@ const char *s390_default_cpu_model_name(void);
 
 /* helper.c */
 #define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
-S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp);
+S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id, Error **errp);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero
    is returned if the signal was handled by the virtual CPU.  */
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index dfb24ef5b2..97adbcc86d 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -68,52 +68,11 @@ void s390x_cpu_timer(void *opaque)
 }
 #endif
 
-S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
+S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id, Error **errp)
 {
-    static bool features_parsed;
-    char *name, *features;
-    const char *typename;
-    ObjectClass *oc;
-    CPUClass *cc;
-
-    name = g_strdup(cpu_model);
-    features = strchr(name, ',');
-    if (features) {
-        features[0] = 0;
-        features++;
-    }
-
-    oc = cpu_class_by_name(TYPE_S390_CPU, name);
-    if (!oc) {
-        error_setg(errp, "Unknown CPU definition \'%s\'", name);
-        g_free(name);
-        return NULL;
-    }
-    typename = object_class_get_name(oc);
-
-    if (!features_parsed) {
-        features_parsed = true;
-        cc = CPU_CLASS(oc);
-        cc->parse_features(typename, features, errp);
-    }
-    g_free(name);
-
-    if (*errp) {
-        return NULL;
-    }
-    return S390_CPU(CPU(object_new(typename)));
-}
-
-S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t core_id, Error **errp)
-{
-    S390CPU *cpu;
+    S390CPU *cpu = S390_CPU(object_new(typename));
     Error *err = NULL;
 
-    cpu = cpu_s390x_create(cpu_model, &err);
-    if (err != NULL) {
-        goto out;
-    }
-
     object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
     if (err != NULL) {
         goto out;
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index b4d3583b24..bc8f83129a 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -337,7 +337,6 @@ uint64_t get_psw_mask(CPUS390XState *env);
 void s390_cpu_recompute_watchpoints(CPUState *cs);
 void s390x_tod_timer(void *opaque);
 void s390x_cpu_timer(void *opaque);
-S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
 void do_restart_interrupt(CPUS390XState *env);
 #ifndef CONFIG_USER_ONLY
 LowCore *cpu_map_lowcore(CPUS390XState *env);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 19/19] s390x: get rid of cpu_s390x_create()

[Cornelia Huck]

On Mon,  4 Sep 2017 17:43:16 +0200
David Hildenbrand <david@redhat.com> wrote:

> Now that there is only one user of cpu_s390x_create() left, make cpu
> creation looks like on x86.
> - Perform the model/properties split and checks in s390_init_cpus()
> - Parse features only once without having to remember if already parsed
> - Pass only the typename to s390x_new_cpu()
> - Use the typename of an existing CPU for hotplug via cpu-add
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 29 +++++++++++++++++++++++++++--
>  target/s390x/cpu.h         |  2 +-
>  target/s390x/helper.c      | 45 ++-------------------------------------------
>  target/s390x/internal.h    |  1 -
>  4 files changed, 30 insertions(+), 47 deletions(-)

LGTM

Re: [Qemu-devel] [PATCH v2 00/19] s390x cleanups and CPU hotplug via device_add

[Christian Borntraeger]

I think we certainly want to have device_add as well (as long as the old
interface stays for a while).

Adding Matt. Can you have a look at the cpu hotplug bits?



On 09/04/2017 05:42 PM, David Hildenbrand wrote:
> The first patches are a bunch of cleanups. I decided to go the
> extra mile and implement CPU hotplug via "device_add", as well as
> "query-hotpluggable-cpus". So the last patches in this series differ
> to v1. I tried to stick as much as possible to x86.
> 
> On s390x, only complete cores can be plugged. Cores have to be plugged in
> the right order (core-id 0, 1, 2 ...). CPU hot unplug is not supported
> by the architecture.
> 
> In addition, we now also print the CPU model list in sorted order and allow
> only 1 CPU for TCG.
> 
> v1 -> v2:
> - "exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly"
> -- added a few instances I missed
> - "s390x: get rid of s390-virtio.c"
> -- move everything in the first shot and clean the "cpu_states" stuff up
>    in later patches, when we have ms->possible_cpus.
> - Added "target/s390x: move typedef of S390CPU to its definition"
> - Added "s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h"
> - Added "s390x: move two function declarations to s390-virtio-ccw.h"
> - Added "s390x: move sclp_service_call() to interrupt.c"
> - "target/s390x: use program_interrupt() in per_check_exception()"
> -- Add a comment describing my concern
> - "s390x: allow only 1 CPU with TCG"
> -- Add a #define to easily be able to change it (e.g. for development)
> - Added "target/s390x: set cpu->id for linux user when realizing"
> - "target/s390x: use "core-id" for cpu number/address/id handling"
> -- I now use "core-id" as property name, because that will be used by
>    device_add
> - "target/s390x: rename next_cpu_id to next_core_id"
> -- Also change to the term "core_id"
> - Added "s390x: print CPU definitions in sorted order"
> - Added "s390x: allow cpu hotplug via device_add"
> - Added "s390x: CPU hot unplug via device_del cannot work"
> - Added "s390x: implement query-hotpluggable-cpu"
> - Added "s390x: get rid of cpu_s390x_create()"
> 
> David Hildenbrand (19):
>   exec,dump,i386,ppc,s390x: don't include exec/cpu-all.h explicitly
>   cpu: drop old comments describing members
>   s390x: get rid of s390-virtio.c
>   s390x: rename s390-virtio.h to s390-virtio-hcall.h
>   target/s390x: move typedef of S390CPU to its definition
>   s390x: move s390_virtio_hypercall() to s390-virtio-hcall.h
>   s390x: move two function declarations to s390-virtio-ccw.h
>   s390x: move sclp_service_call() to interrupt.c
>   target/s390x: use trigger_pgm_exception() in
>     s390_cpu_handle_mmu_fault()
>   target/s390x: use program_interrupt() in per_check_exception()
>   s390x: allow only 1 CPU with TCG
>   target/s390x: set cpu->id for linux user when realizing
>   target/s390x: use "core-id" for cpu number/address/id handling
>   target/s390x: rename next_cpu_id to next_core_id
>   s390x: print CPU definitions in sorted order
>   s390x: allow cpu hotplug via device_add
>   s390x: CPU hot unplug via device_del cannot work
>   s390x: implement query-hotpluggable-cpus
>   s390x: get rid of cpu_s390x_create()
> 
>  dump.c                             |   1 -
>  exec.c                             |   2 -
>  hw/s390x/Makefile.objs             |   1 -
>  hw/s390x/s390-virtio-ccw.c         | 259 +++++++++++++++++++++++++++++++++++--
>  hw/s390x/s390-virtio-hcall.c       |   2 +-
>  hw/s390x/s390-virtio-hcall.h       |  21 +++
>  hw/s390x/s390-virtio.c             |  37 ------
>  hw/s390x/s390-virtio.h             |  35 -----
>  hw/s390x/sclp.c                    |  51 +-------
>  include/hw/s390x/s390-virtio-ccw.h |   3 +
>  include/hw/s390x/sclp.h            |   1 +
>  include/qom/cpu.h                  |   6 +-
>  qapi-schema.json                   |  16 +++
>  target/i386/arch_dump.c            |   1 -
>  target/i386/arch_memory_mapping.c  |   1 -
>  target/i386/svm_helper.c           |   1 -
>  target/ppc/arch_dump.c             |   1 -
>  target/s390x/arch_dump.c           |   1 -
>  target/s390x/cpu-qom.h             |   4 +-
>  target/s390x/cpu.c                 |  86 ++++--------
>  target/s390x/cpu.h                 |  19 +--
>  target/s390x/cpu_models.c          |  65 ++++++----
>  target/s390x/diag.c                |   1 +
>  target/s390x/excp_helper.c         |   5 +-
>  target/s390x/helper.c              |  47 +------
>  target/s390x/internal.h            |   1 -
>  target/s390x/interrupt.c           |  52 ++++++++
>  target/s390x/kvm.c                 |   3 +-
>  target/s390x/misc_helper.c         |  22 ++--
>  target/s390x/translate.c           |   5 +-
>  30 files changed, 443 insertions(+), 307 deletions(-)
>  create mode 100644 hw/s390x/s390-virtio-hcall.h
>  delete mode 100644 hw/s390x/s390-virtio.h
> 

Re: [Qemu-devel] [PATCH v2 00/19] s390x cleanups and CPU hotplug via device_add

[David Hildenbrand]

On 05.09.2017 09:51, Christian Borntraeger wrote:
> I think we certainly want to have device_add as well (as long as the old
> interface stays for a while).
> 
> Adding Matt. Can you have a look at the cpu hotplug bits?

If there is no further feedback, I'll be sending a new version out
tomorrow, including the suggested changes. In addition, I'll be
including support to hotplug VCPUs in random order (2 additional patches).

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 00/19] s390x cleanups and CPU hotplug via device_add

[Matthew Rosato]

On 09/06/2017 12:03 PM, David Hildenbrand wrote:
> On 05.09.2017 09:51, Christian Borntraeger wrote:
>> I think we certainly want to have device_add as well (as long as the old
>> interface stays for a while).
>>
>> Adding Matt. Can you have a look at the cpu hotplug bits?
> 
> If there is no further feedback, I'll be sending a new version out
> tomorrow, including the suggested changes. In addition, I'll be
> including support to hotplug VCPUs in random order (2 additional patches).
> 

Hi David, just returning from vacation -- Will look at this set today.

Matt