next round of s390x cleanups

All but two, namely exec.c and dump.c, include exec/exec-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 <[email protected]>

---
 dump.c | 1 -
 exec.c | 1 -
 2 files changed, 2 deletions(-)

dump.c +0 -0

index d9090a24cc..c00094475c 100644

dump.c +0 -0

 #include "exec/hwaddr.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"

exec.c +0 -0

index d20c34ca83..8d8b6a0769 100644

Quoted 15 lines · David Hildenbrand

All but two, namely exec.c and dump.c, include exec/exec-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 <[email protected]>

---
 dump.c | 1 -
 exec.c | 1 -
 2 files changed, 2 deletions(-)

dump.c +0 -0

index d9090a24cc..c00094475c 100644

dump.c line 18

Quoted 4 lines

dump.c +0 -1

@@ -15,7 +15,6 @@
 #include "qemu/cutils.h"
 #include "elf.h"
 #include "cpu.h"
-#include "exec/cpu-all.h"

That's cpu-all.h, not exec-all.h ...

Quoted 8 lines

dump.c +0 -0

 #include "exec/hwaddr.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"

exec.c +0 -0

index d20c34ca83..8d8b6a0769 100644

exec.c line 28

Quoted 7 lines

exec.c +0 -1

@@ -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"

... and if I do a grep for exec-all.h in the cpu.h files, I hardly get any matches. => Your patch description sounds wrong ... I guess you only wanted to handle cpu-all.h here?

Thomas

All but two, namely exec.c and dump.c, include exec/exec-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 <[email protected]>

---
 dump.c | 1 -
 exec.c | 1 -
 2 files changed, 2 deletions(-)

Quoted 10 lines · On Wed, 30 Aug 2017 19:05:51 +0200 David Hildenbrand <[email protected]> wrote:

All but two, namely exec.c and dump.c, include exec/exec-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 <[email protected]>

---
 dump.c | 1 -
 exec.c | 1 -
 2 files changed, 2 deletions(-)

I think there are some more (target/<arch>/arch_dump.c and some i386 files) - do you want to clean up those as well?

These comments are obviously stale.

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qom/cpu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

include/qom/cpu.h +0 -0

index 25eefea7ab..6b4b838e8f 100644
@@ -378,10 +378,10 @@ struct CPUState {
     DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS);
 
     /* TODO Move common fields from CPUArchState here. */

Let's avoid global variables. While at it, move both functions using it, so we won't have to temporarily add includes (we'll be getting rid of s390-virtio.c soon).

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c         | 39 ++++++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio.c             | 38 -------------------------------------
 hw/s390x/s390-virtio.h             |  1 -
 include/hw/s390x/s390-virtio-ccw.h |  3 +++
 4 files changed, 42 insertions(+), 39 deletions(-)

hw/s390x/s390-virtio-ccw.c +1 -0

index dd504dd5ae..ffd56af834 100644
@@ -32,6 +32,45 @@
 #include "migration/register.h"
 #include "cpu_models.h"
 
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)

hw/s390x/s390-virtio-ccw.c line 45

Quoted 12 lines

hw/s390x/s390-virtio-ccw.c +11 -0

+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }
+
+    /* Fast lookup via CPU ID */
+    return ms->cpus[cpu_addr];
+}

I wonder whether that function should rather go into a file in target/s390x/ instead, since it is also used there and its prototype is in cpu.h ?

[...]

Quoted 8 lines

include/hw/s390x/s390-virtio-ccw.h +0 -0

index 41a9d2862b..4bef28ec39 100644
@@ -21,11 +21,14 @@
 #define S390_MACHINE_CLASS(klass) \
     OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
 

include/hw/s390x/s390-virtio-ccw.h line 24

Quoted 1 line

include/hw/s390x/s390-virtio-ccw.h +1 -0

+struct S390CPU;

You define a "struct S390CPU" here ...

Quoted 6 lines

include/hw/s390x/s390-virtio-ccw.h +1 -0

 typedef struct S390CcwMachineState {
     /*< private >*/
     MachineState parent_obj;
 
     /*< public >*/
+    S390CPU **cpus;

... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I wonder whether the typedef is really in the right place?

include/hw/s390x/s390-virtio-ccw.h line 34

Quoted 3 lines

include/hw/s390x/s390-virtio-ccw.h +0 -0

     bool aes_key_wrap;
     bool dea_key_wrap;
     uint8_t loadparm[8];

Anyway, that were just nits, I'm also fine with the patch as it is, so:

Reviewed-by Thomas Huth <[email protected]>

Quoted 7 lines

Quoted 7 lines

hw/s390x/s390-virtio-ccw.c +7 -0

+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }

Quoted 8 lines

hw/s390x/s390-virtio-ccw.c line 45

Quoted 4 lines

hw/s390x/s390-virtio-ccw.c +4 -0

+
+    /* Fast lookup via CPU ID */
+    return ms->cpus[cpu_addr];
+}  

I wonder whether that function should rather go into a file in target/s390x/ instead, since it is also used there and its prototype is in cpu.h ?

I thought about the same thing, but as it works directly on the machine, like ri_allowed() and friends. So I decided to keep it here for now.

I'll think about moving the definition also into
include/hw/s390x/s390-virtio-ccw.h

Quoted 16 lines · On Thu, 31 Aug 2017 15:11:28 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 15 lines

hw/s390x/s390-virtio-ccw.c line 45

Quoted 11 lines

hw/s390x/s390-virtio-ccw.c +11 -0

+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }
+
+    /* Fast lookup via CPU ID */
+    return ms->cpus[cpu_addr];
+}  

I wonder whether that function should rather go into a file in target/s390x/ instead, since it is also used there and its prototype is in cpu.h ?

Quoted 5 lines

I thought about the same thing, but as it works directly on the machine, like ri_allowed() and friends. So I decided to keep it here for now.

I'll think about moving the definition also into
include/hw/s390x/s390-virtio-ccw.h

It would be a bit nicer.

Quoted 22 lines · Cornelia Huck

Quoted 19 lines · On Thu, 31 Aug 2017 15:11:28 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 7 lines

Quoted 7 lines

hw/s390x/s390-virtio-ccw.c +7 -0

+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }

Quoted 8 lines

hw/s390x/s390-virtio-ccw.c line 45

Quoted 4 lines

hw/s390x/s390-virtio-ccw.c +4 -0

+
+    /* Fast lookup via CPU ID */
+    return ms->cpus[cpu_addr];
+}    

I wonder whether that function should rather go into a file in target/s390x/ instead, since it is also used there and its prototype is in cpu.h ?

I thought about the same thing, but as it works directly on the machine, like ri_allowed() and friends. So I decided to keep it here for now.

Quoted 5 lines

Quoted 2 lines

I'll think about moving the definition also into
include/hw/s390x/s390-virtio-ccw.h

It would be a bit nicer.

Adding patches right now to move everything out of cpu.h that lies under the "/* outside of target/s390x/ */" section. :)

Quoted 29 lines · On Thu, 31 Aug 2017 16:30:59 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 27 lines · Cornelia Huck

Quoted 16 lines · On Thu, 31 Aug 2017 15:11:28 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 7 lines

Quoted 7 lines

hw/s390x/s390-virtio-ccw.c +7 -0

+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }

Quoted 8 lines

hw/s390x/s390-virtio-ccw.c line 45

Quoted 4 lines

hw/s390x/s390-virtio-ccw.c +4 -0

+
+    /* Fast lookup via CPU ID */
+    return ms->cpus[cpu_addr];
+}    

I wonder whether that function should rather go into a file in target/s390x/ instead, since it is also used there and its prototype is in cpu.h ?

Quoted 5 lines

I thought about the same thing, but as it works directly on the machine, like ri_allowed() and friends. So I decided to keep it here for now.

I'll think about moving the definition also into
include/hw/s390x/s390-virtio-ccw.h

It would be a bit nicer.

Quoted 3 lines

Adding patches right now to move everything out of cpu.h that lies under the "/* outside of target/s390x/ */" section. :)

Ah, you really care about your patch count, don't you? :)

(I think it's a good idea.)

Quoted 2 lines

+struct S390CPU;

You define a "struct S390CPU" here ...

Quoted 4 lines

 typedef struct S390CcwMachineState {
     /*< private >*/
     MachineState parent_obj;

Quoted 2 lines

     /*< public >*/

+ S390CPU **cpus;

... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I wonder whether the typedef is really in the right place?

Quoted 4 lines

     bool aes_key_wrap;
     bool dea_key_wrap;
     uint8_t loadparm[8];

Anyway, that were just nits, I'm also fine with the patch as it is, so:

Reviewed-by Thomas Huth <[email protected]>

Quoted 12 lines

Quoted 2 lines

+struct S390CPU;

You define a "struct S390CPU" here ...

Quoted 6 lines

 typedef struct S390CcwMachineState {
     /*< private >*/
     MachineState parent_obj;
 
     /*< public >*/

+ S390CPU **cpus;

Quoted 2 lines

... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I wonder whether the typedef is really in the right place?

General question: how much do we care about headers that are not consistent?

E.g. shall I forward declare or simply ignore if compilers don't bite me?

Quoted 15 lines · David Hildenbrand

Quoted 14 lines

Quoted 2 lines

+struct S390CPU;

You define a "struct S390CPU" here ...

Quoted 6 lines

 typedef struct S390CcwMachineState {
     /*< private >*/
     MachineState parent_obj;
 
     /*< public >*/

+ S390CPU **cpus;

... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I wonder whether the typedef is really in the right place?

Quoted 3 lines

General question: how much do we care about headers that are not consistent?

E.g. shall I forward declare or simply ignore if compilers don't bite me?

My remark was not so much about your patch, but about the original definition instead: "struct S390CPU" is declared in target/s390x/cpu.h, but "typedef struct S390CPU S390CPU" is in target/s390x/cpu-qom.h. I think they should rather be declared in the same header file instead. Or

your "struct S390CPU;" forward declaration should go into cpu-qom.h instead, right in front of the typedef.

Thomas

Quoted 16 lines · Thomas Huth

Quoted 15 lines · David Hildenbrand

Quoted 14 lines

Quoted 2 lines

+struct S390CPU;

You define a "struct S390CPU" here ...

Quoted 6 lines

 typedef struct S390CcwMachineState {
     /*< private >*/
     MachineState parent_obj;
 
     /*< public >*/

+ S390CPU **cpus;

... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I wonder whether the typedef is really in the right place?

Quoted 8 lines

Quoted 3 lines

General question: how much do we care about headers that are not consistent?

E.g. shall I forward declare or simply ignore if compilers don't bite me?

My remark was not so much about your patch, but about the original definition instead: "struct S390CPU" is declared in target/s390x/cpu.h, but "typedef struct S390CPU S390CPU" is in target/s390x/cpu-qom.h. I think they should rather be declared in the same header file instead. Or

I agree, will have a look.

Quoted 3 lines

your "struct S390CPU;" forward declaration should go into cpu-qom.h instead, right in front of the typedef.

Let me rephrase my question:

include/hw/s390x/s390-virtio-ccw.h does not include cpu.h/cpu-qom.h

If compilers don't complain, do we have to forward declare at all? (I think it is cleaner, but I would like to know what is suggested)

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.

Cornelia Huck <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---

hw/s390x/s390-virtio-ccw.c +0 -0

index ffd56af834..41a9e976dc 100644

hw/s390x/s390-virtio-ccw.c +16 -0

+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);

hw/s390x/s390-virtio-ccw.c void s390_memory_init(ram_addr_t mem_size) · line 191

Quoted 5 lines

hw/s390x/s390-virtio-ccw.c +4 -0

+    return 0;
+}
+
+

Nit: One empty line should be enough here.

Reviewed-by Thomas Huth <[email protected]>

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.

Cornelia Huck <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/Makefile.objs     |   1 -
 hw/s390x/s390-virtio-ccw.c | 120 ++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     | 163 ---------------------------------------------
 hw/s390x/s390-virtio.h     |  14 ----
 4 files changed, 119 insertions(+), 179 deletions(-)
 delete mode 100644 hw/s390x/s390-virtio.c

hw/s390x/Makefile.objs +0 -1

index 7ee19d3abc..dc704b57d6 100644
@@ -1,4 +1,3 @@
-obj-y += s390-virtio.o
 obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o

hw/s390x/s390-virtio-ccw.c +119 -1

index ffd56af834..41a9e976dc 100644
@@ -2,6 +2,7 @@
  * virtio ccw machine
  *
  * Copyright 2012 IBM Corp.
+ * Copyright (c) 2009 Alexander Graf <[email protected]>
  * Author(s): Cornelia Huck <[email protected]>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or (at

@@ -31,6 +32,8 @@
 #include "hw/s390x/css-bridge.h"
 #include "migration/register.h"
 #include "cpu_models.h"
+#include "qapi/qmp/qerror.h"
+#include "hw/nmi.h"
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {

@@ -133,7 +136,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);

@@ -148,11 +151,104 @@ 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;

@@ -216,6 +312,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)
 {

@@ -240,6 +349,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);

hw/s390x/s390-virtio.c deleted +0 -163

deleted file mode 100644
index 464b5c71f8..0000000000
@@ -1,163 +0,0 @@
-/*
- * QEMU S390 virtio target
- *
- * Copyright (c) 2009 Alexander Graf <[email protected]>
- * Copyright IBM Corp 2012
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * Contributions after 2012-10-29 are licensed under the terms of the
- * GNU GPL, version 2 or (at your option) any later version.
- *
- * You should have received a copy of the GNU (Lesser) General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "hw/hw.h"
-#include "qapi/qmp/qerror.h"
-#include "qemu/error-report.h"
-#include "sysemu/block-backend.h"
-#include "sysemu/blockdev.h"
-#include "sysemu/sysemu.h"
-#include "net/net.h"
-#include "hw/boards.h"
-#include "hw/loader.h"
-#include "hw/virtio/virtio.h"
-#include "exec/address-spaces.h"
-#include "sysemu/qtest.h"
-
-#include "hw/s390x/sclp.h"
-#include "hw/s390x/s390_flic.h"
-#include "hw/s390x/s390-virtio.h"
-#include "hw/s390x/storage-keys.h"
-#include "hw/s390x/ipl.h"
-#include "cpu.h"
-
-#define MAX_BLK_DEVS                    10
-
-#define S390_TOD_CLOCK_VALUE_MISSING    0x00
-#define S390_TOD_CLOCK_VALUE_PRESENT    0x01
-
-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);
-}
-
-
-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);
-    }
-}
-
-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);
-}
-
-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;
-}
-
-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);
-    }
-}
-
-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);
-}

hw/s390x/s390-virtio.h +0 -14

index b6660e3ae9..d984cd4115 100644
@@ -12,23 +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_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

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.

Cornelia Huck <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---

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

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c                      | 2 +-
 hw/s390x/s390-virtio-hcall.c                    | 2 +-
 hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)

hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +0 -0

similarity index 92%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..64c5bbd827 100644

Quoted 9 lines · David Hildenbrand

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

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c                      | 2 +-
 hw/s390x/s390-virtio-hcall.c                    | 2 +-
 hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)

[...]

Quoted 9 lines

hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +0 -0

similarity index 92%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..64c5bbd827 100644

hw/s390x/s390-virtio-hcall.h line 5

Quoted 6 lines

hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +1 -1

@@ -1,5 +1,5 @@
 /*
- * Virtio interfaces for s390
+ * Support for virtio hypercalls on s390x
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <[email protected]>

Maybe also rename the HW_S390_VIRTIO_H header guard? Anyway:

Reviewed-by Thomas Huth <[email protected]>

Quoted 29 lines · Thomas Huth

Quoted 9 lines · David Hildenbrand

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

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c                      | 2 +-
 hw/s390x/s390-virtio-hcall.c                    | 2 +-
 hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)

[...]

Quoted 9 lines

hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +0 -0

similarity index 92%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..64c5bbd827 100644

hw/s390x/s390-virtio-hcall.h line 5

Quoted 6 lines

hw/s390x/s390-virtio-hcall.h renamed from hw/s390x/s390-virtio.h +1 -1

@@ -1,5 +1,5 @@
 /*
- * Virtio interfaces for s390
+ * Support for virtio hypercalls on s390x
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <[email protected]>

Maybe also rename the HW_S390_VIRTIO_H header guard? Anyway:

Quoted 2 lines

Reviewed-by Thomas Huth <[email protected]>

Sure, will do. Is HW_S390_VIRTIO_HCALL_H okay?

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.

Switch to the term "addr" instead, which matches the definition in the PoP called "CPU address". There is no such thing as cpu number, so rename env.cpu_num to env.cpu_addr.

We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr 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 "addr" can be a static property. Range checks can be avoided by using the correct type and the "setting after realized" check is done implicitly.

AFAIK, s390x only supports cpu_add and not device_add for cpus. So we should be able to safely rename that property (no the "id" property could properly be used for device_add, which needs an artificial id for identification purposes).

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c |  2 +-
 target/s390x/cpu.c         | 69 ++++++++++++----------------------------------
 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, 28 insertions(+), 65 deletions(-)

Quoted 19 lines · On Wed, 30 Aug 2017 19:05:56 +0200 David Hildenbrand <[email protected]> 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.

Switch to the term "addr" instead, which matches the definition in the PoP called "CPU address". There is no such thing as cpu number, so rename env.cpu_num to env.cpu_addr.

We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr 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 "addr" can be a static property. Range checks can be avoided by using the correct type and the "setting after realized" check is done implicitly.

Quoted 4 lines

AFAIK, s390x only supports cpu_add and not device_add for cpus. So we should be able to safely rename that property (no the "id" property could properly be used for device_add, which needs an artificial id for identification purposes).

I cannot parse the sentence in the brackets...

Quoted 13 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c |  2 +-
 target/s390x/cpu.c         | 69 ++++++++++++----------------------------------
 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, 28 insertions(+), 65 deletions(-)

...the patch seems fine, though :)

Quoted 27 lines · Cornelia Huck

Quoted 23 lines · On Wed, 30 Aug 2017 19:05:56 +0200 David Hildenbrand <[email protected]> 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.

Switch to the term "addr" instead, which matches the definition in the PoP called "CPU address". There is no such thing as cpu number, so rename env.cpu_num to env.cpu_addr.

We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr 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 "addr" can be a static property. Range checks can be avoided by using the correct type and the "setting after realized" check is done implicitly.

AFAIK, s390x only supports cpu_add and not device_add for cpus. So we should be able to safely rename that property (no the "id" property could properly be used for device_add, which needs an artificial id for identification purposes).

Quoted 1 line

I cannot parse the sentence in the brackets...

Me too :)

...So we should be able to safely rename that property. device_add will later need the reserved "id" property. Hotplugging a CPU would then look like this: "device_add host-s390-cpu id=cpu2 addr=2".

Quoted 15 lines · On Wed, 30 Aug 2017 19:05:56 +0200 David Hildenbrand <[email protected]> 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.

Switch to the term "addr" instead, which matches the definition in the PoP called "CPU address". There is no such thing as cpu number, so rename env.cpu_num to env.cpu_addr.

We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr 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.

Quoted 8 lines

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

AFAIK, s390x only supports cpu_add and not device_add for cpus. So we should be able to safely rename that property (no the "id" property could properly be used for device_add, which needs an artificial id for identification purposes).

Quoted 18 lines · On Thu, 31 Aug 2017 16:35:13 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 15 lines · On Wed, 30 Aug 2017 19:05:56 +0200 David Hildenbrand <[email protected]> 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.

Switch to the term "addr" instead, which matches the definition in the PoP called "CPU address". There is no such thing as cpu number, so rename env.cpu_num to env.cpu_addr.

We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr 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.

Quoted 8 lines

Quoted 8 lines

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

AFAIK, s390x only supports cpu_add and not device_add for cpus. So we should be able to safely rename that property (no the "id" property could properly be used for device_add, which needs an artificial id for identification purposes).

this patch seems to somewhat conflicting with
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06505.html
that were supposed to go via machine tree and which I've respinned today to fix conflicts due just merged pull req.

Cornelia,

Could you put/merge it via s390x tree so that David and I work won't clash again?

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 are specified, so we don't raise people's hope.

Quoted 4 lines · 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.

Quoted 2 lines

Let's bail out if more than 1 are specified, so we don't raise people's hope.

Aurelien recently posted a patch to add that basic SIGP support:

https://patchwork.kernel.org/patch/9717489/

I think it would make more sense to get that included instead.

Quoted 6 lines · 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 are specified, so we don't raise people's hope.

Aurelien recently posted a patch to add that basic SIGP support:

https://patchwork.kernel.org/patch/9717489/

I think it would make more sense to get that included instead.

Thomas

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 are specified, so we don't raise people's hope.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/s390x/s390-virtio-ccw.c +1 -0

index 7754e3eaf9..eff96808c4 100644
@@ -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"

hw/s390x/s390-virtio-ccw.c +2 -0

+        exit(1);
+    }
 
     ms->cpus = g_new0(S390CPU *, max_cpus);
 

Quoted 26 lines · On Wed, 30 Aug 2017 19:05:58 +0200 David Hildenbrand <[email protected]> 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 are specified, so we don't raise people's hope.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/s390x/s390-virtio-ccw.c +1 -0

index 7754e3eaf9..eff96808c4 100644
@@ -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"

@@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine)
     if (machine->cpu_model == NULL) {

hw/s390x/s390-virtio-ccw.c static void s390_init_cpus(MachineState *machine) · line 62

Quoted 5 lines

hw/s390x/s390-virtio-ccw.c +3 -0

         machine->cpu_model = s390_default_cpu_model_name();
     }
+    if (tcg_enabled() && max_cpus > 1) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by TCG (1) on s390x", max_cpus);

Make this a #define, so we can just flip the switch when smp support is ready?

Quoted 6 lines

hw/s390x/s390-virtio-ccw.c +2 -0

+        exit(1);
+    }
 
     ms->cpus = g_new0(S390CPU *, max_cpus);
 

Quoted 29 lines · Cornelia Huck

Quoted 26 lines · On Wed, 30 Aug 2017 19:05:58 +0200 David Hildenbrand <[email protected]> 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 are specified, so we don't raise people's hope.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/s390x/s390-virtio-ccw.c +1 -0

index 7754e3eaf9..eff96808c4 100644
@@ -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"

@@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine)
     if (machine->cpu_model == NULL) {

Quoted 8 lines

hw/s390x/s390-virtio-ccw.c static void s390_init_cpus(MachineState *machine) · line 62

Quoted 5 lines

hw/s390x/s390-virtio-ccw.c +3 -0

         machine->cpu_model = s390_default_cpu_model_name();
     }
+    if (tcg_enabled() && max_cpus > 1) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by TCG (1) on s390x", max_cpus);  

Make this a #define, so we can just flip the switch when smp support is ready?

As an alternative: yield a warning?

Quoted 8 lines

Quoted 7 lines

hw/s390x/s390-virtio-ccw.c +2 -0

+        exit(1);
+    }
 
     ms->cpus = g_new0(S390CPU *, max_cpus);
 

Quoted 39 lines · On Thu, 31 Aug 2017 17:03:21 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 37 lines · Cornelia Huck

Quoted 25 lines · On Wed, 30 Aug 2017 19:05:58 +0200 David Hildenbrand <[email protected]> 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 are specified, so we don't raise people's hope.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/s390x/s390-virtio-ccw.c +1 -0

index 7754e3eaf9..eff96808c4 100644
@@ -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"

hw/s390x/s390-virtio-ccw.c static void s390_init_cpus(MachineState *machine) · line 62

Quoted 6 lines

hw/s390x/s390-virtio-ccw.c +3 -0

@@ -56,6 +57,11 @@ 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 > 1) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by TCG (1) on s390x", max_cpus);  

Make this a #define, so we can just flip the switch when smp support is ready?

Quoted 1 line

As an alternative: yield a warning?

If we know that this can't work, it makes sense to stop immediately, no?

Quoted 1 line · Cornelia Huck

If we know that this can't work, it makes sense to stop immediately, no?

Hmm, like that than?

hw/s390x/s390-virtio-ccw.c +7 -0

index 18ed0c57e3..dae848fa5f 100644
@@ -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"

@@ -56,6 +57,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);
+    }
 
     ms->cpus = g_new0(S390CPU *, max_cpus);
 

target/s390x/cpu.h +2 -0

index 7ed9103b33..4e1de2102d 100644
@@ -210,6 +210,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

The assert should hold in both scenarios.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/interrupt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

target/s390x/interrupt.c +0 -0

index 058e219fe5..79bab5e2f3 100644

Quoted 17 lines · David Hildenbrand

The assert should hold in both scenarios.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/interrupt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

target/s390x/interrupt.c +0 -2

index 058e219fe5..79bab5e2f3 100644
@@ -32,9 +32,8 @@ static void tcg_s390_program_interrupt(CPUS390XState *env, uint32_t code,
 #ifdef CONFIG_TCG
     trigger_pgm_exception(env, code, ilen);
     cpu_loop_exit(CPU(s390_env_get_cpu(env)));
-#else
-    g_assert_not_reached();

target/s390x/interrupt.c static void tcg_s390_program_interrupt(CPUS390XState *env, uint32_t code, · line 37

Quoted 3 lines

target/s390x/interrupt.c +1 -0

 #endif
+    g_assert_not_reached();
 }

Not sure if this really makes sense ... cpu_loop_exit() is already marked with QEMU_NORETURN, so a know-it-all new version of GCC might complain one day if there's other code after this call. I'd better keep it the way it is.

Thomas

This looks cleaner.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

target/s390x/excp_helper.c +0 -0

index 2ac36535f7..f5f5967833 100644

Quoted 15 lines · David Hildenbrand

This looks cleaner.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

target/s390x/excp_helper.c +0 -0

index 2ac36535f7..f5f5967833 100644
@@ -59,8 +59,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
 {
     S390CPU *cpu = S390_CPU(cs);
 

target/s390x/excp_helper.c int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address, · line 65

Quoted 6 lines

target/s390x/excp_helper.c +1 -2

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

trigger_pgm_exception() additionally sets int_pgm_ilen ... I assume that's OK here? A comment in the patch description would be helpful.

Thomas

I am not sure if we are handling ilen the right way here. ilen should always match the instruction triggering the exception. This is relevant for per exceptions triggered via EXECUTE instructions. The ilen to be indicated has to match the EXECUTE instruction.

Clean it up for now but leave ilen as is, we can fix that later.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/misc_helper.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

target/s390x/misc_helper.c +1 -1

index eb7accc0ce..ac9657f23f 100644
@@ -445,14 +445,11 @@ 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;
 

No new functionality, only cleanups, some of the discussed during the last round of cleanups.

Not sure if the first two patches should be sent separatly? Anyhow, they are most probably not worth the trouble :)

The biggest part of this series is getting rid of s390-virtio.c and cleaning up our cpu number/id handling.

Based on: git://github.com/cohuck/qemu s390-next

David Hildenbrand (11):
exec,dump: don't include exec/exec-all.h explicitly
cpu: drop old comments describing members
s390x: store cpu states inside machine state
s390x: get rid of s390-virtio.c
s390x: rename s390-virtio.h to s390-virtio-hcall.h target/s390x: cleanup cpu number/address handling target/s390x: rename next_cpu_id to next_cpu_addr
s390x: allow only 1 CPU with TCG
target/s390x: tcg_s390_program_interrupt() will never return target/s390x: use trigger_pgm_exception() in

    s390_cpu_handle_mmu_fault()

target/s390x: use program_interrupt() in per_check_exception()

 dump.c                             |   1 -
 exec.c                             |   1 -
 hw/s390x/Makefile.objs             |   1 -
 hw/s390x/s390-virtio-ccw.c         | 169 ++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio-hcall.c       |   2 +-
 hw/s390x/s390-virtio-hcall.h       |  20 ++++
 hw/s390x/s390-virtio.c             | 201 -------------------------------------
 hw/s390x/s390-virtio.h             |  35 -------
 include/hw/s390x/s390-virtio-ccw.h |   3 +
 include/qom/cpu.h                  |   6 +-
 target/s390x/cpu-qom.h             |   2 +-
 target/s390x/cpu.c                 |  74 ++++----------
 target/s390x/cpu.h                 |   5 +-
 target/s390x/cpu_models.c          |   2 +-
 target/s390x/excp_helper.c         |   5 +-
 target/s390x/helper.c              |   8 +-
 target/s390x/interrupt.c           |   3 +-
 target/s390x/misc_helper.c         |  13 +--
 target/s390x/translate.c           |   5 +-
 19 files changed, 230 insertions(+), 326 deletions(-)
 create mode 100644 hw/s390x/s390-virtio-hcall.h
 delete mode 100644 hw/s390x/s390-virtio.c
 delete mode 100644 hw/s390x/s390-virtio.h