[Qemu-devel] [PATCH v1 00/11] next round of s390x cleanups

[Qemu-devel] [PATCH v1 00/11] next round of s390x cleanups

[David Hildenbrand]

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

-- 
2.13.5

[Qemu-devel] [PATCH v1 01/11] exec, dump: don't include exec/exec-all.h explicitly

[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 <david@redhat.com>
---
 dump.c | 1 -
 exec.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/dump.c b/dump.c
index d9090a24cc..c00094475c 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..8d8b6a0769 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"
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 01/11] exec, dump: don't include exec/exec-all.h explicitly

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand 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 <david@redhat.com>
> ---
>  dump.c | 1 -
>  exec.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index d9090a24cc..c00094475c 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"

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

>  #include "exec/hwaddr.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/kvm.h"
> diff --git a/exec.c b/exec.c
> index d20c34ca83..8d8b6a0769 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"

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

Re: [Qemu-devel] [PATCH v1 01/11] exec, dump: don't include exec/exec-all.h explicitly

[David Hildenbrand]

On 30.08.2017 20:55, Thomas Huth wrote:
> On 30.08.2017 19:05, David Hildenbrand 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 <david@redhat.com>
>> ---
>>  dump.c | 1 -
>>  exec.c | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/dump.c b/dump.c
>> index d9090a24cc..c00094475c 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"
> 
> That's cpu-all.h, not exec-all.h ...
> 
>>  #include "exec/hwaddr.h"
>>  #include "monitor/monitor.h"
>>  #include "sysemu/kvm.h"
>> diff --git a/exec.c b/exec.c
>> index d20c34ca83..8d8b6a0769 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"
> 
> ... 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
> 

exec/cpu-all.h it is. Fixed the subject+description.

I'd really love to know what my brain was doing while composing that
message. It will remain a mystery.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 01/11] exec, dump: don't include exec/exec-all.h explicitly

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:51 +0200
David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> ---
>  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?

Re: [Qemu-devel] [PATCH v1 01/11] exec, dump: don't include exec/exec-all.h explicitly

[David Hildenbrand]

On 31.08.2017 16:21, Cornelia Huck wrote:
> On Wed, 30 Aug 2017 19:05:51 +0200
> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>> ---
>>  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?
> 

Whoops, missed these. Yes, will clean them up, too.

-- 

Thanks,

David

[Qemu-devel] [PATCH v1 02/11] cpu: drop old comments describing members

[David Hildenbrand]

These comments are obviously stale.

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 25eefea7ab..6b4b838e8f 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

Re: [Qemu-devel] [PATCH v1 02/11] cpu: drop old comments describing members

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:52 +0200
David Hildenbrand <david@redhat.com> wrote:

> These comments are obviously stale.
> 
> 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 25eefea7ab..6b4b838e8f 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;

Yup, no need to drag these around. In case I don't take this through
s390-next:

Acked-by: Cornelia Huck <cohuck@redhat.com>

[Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[David Hildenbrand]

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 <david@redhat.com>
---
 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(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index dd504dd5ae..ffd56af834 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -32,6 +32,45 @@
 #include "migration/register.h"
 #include "cpu_models.h"
 
+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];
+}
+
+static void s390_init_cpus(MachineState *machine)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+    int i;
+    gchar *name;
+
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = s390_default_cpu_model_name();
+    }
+
+    ms->cpus = 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 **) &ms->cpus[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,
     "s390-sclp-event-facility",
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index da3f49e80e..464b5c71f8 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -48,18 +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,
                        const char *initrd_filename,
@@ -86,32 +74,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..b6660e3ae9 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -19,7 +19,6 @@
 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,
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 41a9d2862b..4bef28ec39 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -21,11 +21,14 @@
 #define S390_MACHINE_CLASS(klass) \
     OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
 
+struct S390CPU;
+
 typedef struct S390CcwMachineState {
     /*< private >*/
     MachineState parent_obj;
 
     /*< public >*/
+    S390CPU **cpus;
     bool aes_key_wrap;
     bool dea_key_wrap;
     uint8_t loadparm[8];
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  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(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index dd504dd5ae..ffd56af834 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -32,6 +32,45 @@
>  #include "migration/register.h"
>  #include "cpu_models.h"
>  
> +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 ?

[...]
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 41a9d2862b..4bef28ec39 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -21,11 +21,14 @@
>  #define S390_MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>  
> +struct S390CPU;

You define a "struct S390CPU" here ...

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

>      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 <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[David Hildenbrand]

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

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

> 
> [...]
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 41a9d2862b..4bef28ec39 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -21,11 +21,14 @@
>>  #define S390_MACHINE_CLASS(klass) \
>>      OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>>  
>> +struct S390CPU;
> 
> You define a "struct S390CPU" here ...
> 
>>  typedef struct S390CcwMachineState {
>>      /*< private >*/
>>      MachineState parent_obj;
>>  
>>      /*< public >*/
>> +    S390CPU **cpus;

I'll simply convert this to struct S390CPU, so this header stays
independent from cpu headers.

> 
> ... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I
> wonder whether the typedef is really in the right place?
> 
>>      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 <thuth@redhat.com>
> 

Thanks!

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Cornelia Huck]

On Thu, 31 Aug 2017 15:11:28 +0200
David Hildenbrand <david@redhat.com> wrote:

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

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[David Hildenbrand]

On 31.08.2017 16:29, Cornelia Huck wrote:
> On Thu, 31 Aug 2017 15:11:28 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>> +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 ?  
>>
>> 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.
> 

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

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Cornelia Huck]

On Thu, 31 Aug 2017 16:30:59 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 31.08.2017 16:29, Cornelia Huck wrote:
> > On Thu, 31 Aug 2017 15:11:28 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >>>> +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 ?    
> >>
> >> 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.
> >   
> 
> 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.)

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[David Hildenbrand]

On 31.08.2017 16:38, Cornelia Huck wrote:
> On Thu, 31 Aug 2017 16:30:59 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 31.08.2017 16:29, Cornelia Huck wrote:
>>> On Thu, 31 Aug 2017 15:11:28 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>>>> +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 ?    
>>>>
>>>> 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.
>>>   
>>
>> 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.)
> 

.... so you want me to squash everything into a single patch then?! ;)

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[David Hildenbrand]

>> +struct S390CPU;
> 
> You define a "struct S390CPU" here ...
> 
>>  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?

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?


> 
>>      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 <thuth@redhat.com>
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Thomas Huth]

On 31.08.2017 16:23, David Hildenbrand wrote:
> 
>>> +struct S390CPU;
>>
>> You define a "struct S390CPU" here ...
>>
>>>  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?
> 
> 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

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[David Hildenbrand]

On 31.08.2017 16:31, Thomas Huth wrote:
> On 31.08.2017 16:23, David Hildenbrand wrote:
>>
>>>> +struct S390CPU;
>>>
>>> You define a "struct S390CPU" here ...
>>>
>>>>  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?
>>
>> 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.

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

>  Thomas
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Thomas Huth]

On 31.08.2017 16:36, David Hildenbrand wrote:
> On 31.08.2017 16:31, Thomas Huth wrote:
>> On 31.08.2017 16:23, David Hildenbrand wrote:
>>>
>>>>> +struct S390CPU;
>>>>
>>>> You define a "struct S390CPU" here ...
>>>>
>>>>>  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?
>>>
>>> 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.
> 
>> 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)

Well, doing a forward declaration with "struct S390CPU;" and then using
the typedef'd "S390CPU" without "struct" does not make much sense -
these are two "different" types. If you can use "S390CPU" here without
the "struct S390CPU;" declaration, that means the cpu-qom.h header got
included indirectly already - so I'd suggest to simply remove the
"struct S390CPU;" forward declaration from your patch.

 Thomas

[Qemu-devel] [PATCH v1 04/11] 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.

Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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

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 ffd56af834..41a9e976dc 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,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);
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
deleted file mode 100644
index 464b5c71f8..0000000000
--- a/hw/s390x/s390-virtio.c
+++ /dev/null
@@ -1,163 +0,0 @@
-/*
- * QEMU S390 virtio target
- *
- * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
- * 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);
-}
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index b6660e3ae9..d984cd4115 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -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
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 04/11] s390x: get rid of s390-virtio.c

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand wrote:
> 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 <cohuck@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
[...]
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index ffd56af834..41a9e976dc 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
[...]
> +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;
> +}
> +
> +

Nit: One empty line should be enough here.

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

Re: [Qemu-devel] [PATCH v1 04/11] s390x: get rid of s390-virtio.c

[David Hildenbrand]

>> +    return 0;
>> +}
>> +
>> +
> 
> Nit: One empty line should be enough here.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Indeed, thanks!

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 04/11] s390x: get rid of s390-virtio.c

[Christian Borntraeger]

I have not reviewed this, but the whole idea is
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


On 08/30/2017 07:05 PM, David Hildenbrand wrote:
> 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 <cohuck@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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
> 
> 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 ffd56af834..41a9e976dc 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,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);
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> deleted file mode 100644
> index 464b5c71f8..0000000000
> --- a/hw/s390x/s390-virtio.c
> +++ /dev/null
> @@ -1,163 +0,0 @@
> -/*
> - * QEMU S390 virtio target
> - *
> - * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> - * 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);
> -}
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index b6660e3ae9..d984cd4115 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -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
> 

Re: [Qemu-devel] [PATCH v1 04/11] s390x: get rid of s390-virtio.c

[David Hildenbrand]

On 30.08.2017 19:05, David Hildenbrand wrote:
> 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 <cohuck@redhat.com>

I will add the missing Suggested-by: :)

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---



-- 

Thanks,

David

[Qemu-devel] [PATCH v1 05/11] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[David Hildenbrand]

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

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} | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 41a9e976dc..03c88a524b 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 92%
rename from hw/s390x/s390-virtio.h
rename to hw/s390x/s390-virtio-hcall.h
index d984cd4115..64c5bbd827 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>
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 05/11] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand wrote:
> The only interface left, so let's properly rename it.
> 
> 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} | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>  rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)
[...]
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio-hcall.h
> similarity index 92%
> rename from hw/s390x/s390-virtio.h
> rename to hw/s390x/s390-virtio-hcall.h
> index d984cd4115..64c5bbd827 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>
> 

Maybe also rename the HW_S390_VIRTIO_H header guard? Anyway:

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

Re: [Qemu-devel] [PATCH v1 05/11] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[David Hildenbrand]

On 31.08.2017 11:29, Thomas Huth wrote:
> On 30.08.2017 19:05, David Hildenbrand wrote:
>> The only interface left, so let's properly rename it.
>>
>> 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} | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>  rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)
> [...]
>> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio-hcall.h
>> similarity index 92%
>> rename from hw/s390x/s390-virtio.h
>> rename to hw/s390x/s390-virtio-hcall.h
>> index d984cd4115..64c5bbd827 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>
>>
> 
> Maybe also rename the HW_S390_VIRTIO_H header guard? Anyway:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Sure, will do. Is HW_S390_VIRTIO_HCALL_H okay?

(HW_S390X_S390_VIRTIO_HCALL_H doesn't sound necessary)

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 05/11] s390x: rename s390-virtio.h to s390-virtio-hcall.h

[Thomas Huth]

On 31.08.2017 15:18, David Hildenbrand wrote:
> On 31.08.2017 11:29, Thomas Huth wrote:
>> On 30.08.2017 19:05, David Hildenbrand wrote:
>>> The only interface left, so let's properly rename it.
>>>
>>> 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} | 2 +-
>>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>>  rename hw/s390x/{s390-virtio.h => s390-virtio-hcall.h} (92%)
>> [...]
>>> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio-hcall.h
>>> similarity index 92%
>>> rename from hw/s390x/s390-virtio.h
>>> rename to hw/s390x/s390-virtio-hcall.h
>>> index d984cd4115..64c5bbd827 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>
>>>
>>
>> Maybe also rename the HW_S390_VIRTIO_H header guard? Anyway:
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
> 
> Sure, will do. Is HW_S390_VIRTIO_HCALL_H okay?

Sounds fine.

 Thomas

[Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address 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.

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 <david@redhat.com>
---
 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(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 03c88a524b..7754e3eaf9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -306,7 +306,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.cpu_addr);
     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 7267b60d41..156589e921 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,24 +190,26 @@ 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.cpu_addr >= max_cpus) {
+        error_setg(&err, "Unable to add CPU: %" PRIu32 ", max allowed: %d",
+                   cpu->env.cpu_addr, max_cpus - 1);
         goto out;
     }
 #endif
-    if (cpu_exists(cpu->id)) {
-        error_setg(&err, "Unable to add CPU: %" PRIi64
-                   ", it already exists", cpu->id);
+    if (cpu_exists(cpu->env.cpu_addr)) {
+        error_setg(&err, "Unable to add CPU: %" PRIu32 ", it already exists",
+                   cpu->env.cpu_addr);
         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.cpu_addr != scc->next_cpu_id) {
+        error_setg(&err, "Unable to add CPU: %" PRIu32
+                   ", the next available nr is %" PRIi64, cpu->env.cpu_addr,
                    scc->next_cpu_id);
         goto out;
     }
 
+    /* sync cs->cpu_index and env->cpu_addr. The latter is needed for TCG. */
+    cs->cpu_index = env->cpu_addr;
     cpu_exec_realizefn(cs, &err);
     if (err != NULL) {
         goto out;
@@ -216,7 +219,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)
@@ -237,45 +239,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);
@@ -289,8 +252,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);
@@ -487,6 +448,11 @@ static gchar *s390_gdb_arch_name(CPUState *cs)
     return g_strdup("s390:64-bit");
 }
 
+static Property s390x_cpu_properties[] = {
+    DEFINE_PROP_UINT32("addr", S390CPU, env.cpu_addr, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void s390_cpu_class_init(ObjectClass *oc, void *data)
 {
     S390CPUClass *scc = S390_CPU_CLASS(oc);
@@ -496,6 +462,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 4ec338077e..6766dbb579 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 cpu_addr; /* same as cpu->index */
     uint64_t cpuid;
 
     uint64_t tod_offset;
@@ -193,7 +193,6 @@ struct S390CPU {
     /*< public >*/
 
     CPUS390XState env;
-    int64_t id;
     S390CPUModel *model;
     /* needed for live migration */
     void *irqstate;
@@ -690,7 +689,7 @@ const char *s390_default_cpu_model_name(void);
 /* helper.c */
 S390CPU *cpu_s390x_init(const char *cpu_model);
 #define cpu_init(model) CPU(cpu_s390x_init(model))
-S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
+S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t addr, 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..1954c64c86 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.cpu_addr);
     }
 }
 
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 361f970db3..2ac36535f7 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -251,7 +251,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->cpu_addr | 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 3adb9de122..70d1ea8cf6 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 addr, 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), addr, "addr", &err);
     if (err != NULL) {
         goto out;
     }
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 50cc046ca2..eb7accc0ce 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -230,7 +230,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->cpu_addr);
             cpu_physical_memory_write(a0, &sysib, sizeof(sysib));
         } else if ((sel1 == 2) && (sel2 == 2)) {
             /* Basic Machine CPUs */
@@ -258,7 +258,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->cpu_addr);
                 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..80455bf8f0 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, cpu_addr));
     return NO_EXIT;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:56 +0200
David Hildenbrand <david@redhat.com> 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).

I cannot parse the sentence in the brackets...

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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 :)

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[David Hildenbrand]

On 31.08.2017 16:35, Cornelia Huck wrote:
> On Wed, 30 Aug 2017 19:05:56 +0200
> David Hildenbrand <david@redhat.com> 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).
> 
> 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".

> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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 :)
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Cornelia Huck]

On Thu, 31 Aug 2017 16:41:47 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 31.08.2017 16:35, Cornelia Huck wrote:
> > On Wed, 30 Aug 2017 19:05:56 +0200
> > David Hildenbrand <david@redhat.com> 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).  
> > 
> > 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".

Yup, that's understandable :)

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Igor Mammedov]

On Thu, 31 Aug 2017 16:35:13 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 30 Aug 2017 19:05:56 +0200
> David Hildenbrand <david@redhat.com> 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).  
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?

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Cornelia Huck]

On Thu, 31 Aug 2017 18:03:52 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 31 Aug 2017 16:35:13 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 30 Aug 2017 19:05:56 +0200
> > David Hildenbrand <david@redhat.com> 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).    
> 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?

I can do that, sure. I can put your patch on top of David's.

[Qemu-devel] [PATCH v1 07/11] target/s390x: rename next_cpu_id to next_cpu_addr

[David Hildenbrand]

Adapt the term address/adddr. 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     | 9 ++++-----
 target/s390x/helper.c  | 4 ++--
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index 4e936e7788..a3a3ada226 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_cpu_addr;
 
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 156589e921..ffc1ff9a2b 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -201,10 +201,10 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    cpu->env.cpu_addr);
         goto out;
     }
-    if (cpu->env.cpu_addr != scc->next_cpu_id) {
+    if (cpu->env.cpu_addr != scc->next_cpu_addr) {
         error_setg(&err, "Unable to add CPU: %" PRIu32
-                   ", the next available nr is %" PRIi64, cpu->env.cpu_addr,
-                   scc->next_cpu_id);
+                   ", the next available addr is %" PRIu32, cpu->env.cpu_addr,
+                   scc->next_cpu_addr);
         goto out;
     }
 
@@ -214,7 +214,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     if (err != NULL) {
         goto out;
     }
-    scc->next_cpu_id++;
+    scc->next_cpu_addr++;
 
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
@@ -459,7 +459,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;
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 70d1ea8cf6..d8f198cf6f 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -134,9 +134,9 @@ S390CPU *cpu_s390x_init(const char *cpu_model)
     Error *err = NULL;
     S390CPU *cpu;
     /* Use to track CPU ID for linux-user only */
-    static int64_t next_cpu_id;
+    static uint32_t next_cpu_addr;
 
-    cpu = s390x_new_cpu(cpu_model, next_cpu_id++, &err);
+    cpu = s390x_new_cpu(cpu_model, next_cpu_addr++, &err);
     if (err) {
         error_report_err(err);
     }
-- 
2.13.5

[Qemu-devel] [PATCH v1 08/11] 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 are specified, so we don't raise people's
hope.

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7754e3eaf9..eff96808c4 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"
@@ -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);
+        exit(1);
+    }
 
     ms->cpus = g_new0(S390CPU *, max_cpus);
 
-- 
2.13.5

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

[Thomas Huth]

On 30.08.2017 19:05, 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 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

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

[Cornelia Huck]

On Wed, 30 Aug 2017 21:06:55 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 30.08.2017 19:05, 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 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.

I'd look at it if it were reposted :)

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

[David Hildenbrand]

On 30.08.2017 21:06, Thomas Huth wrote:
> On 30.08.2017 19:05, 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 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
> 

Even then, it doesn't work reliably:

- "*very rough* SMP support"
- "this patch is nothing more than a way to determine what needs to
   be implemented"
- "It should be rewritten from scratch before reaching in an acceptable
   state."

Such broken feature should not be exposed to the user. Once we have
properly fixed that we can enable and announce it "s390x now supports
more than 1 VCPU under TCG". At this point, this is just wrong, even
with this patch included.

So I'd suggest including this now and reverting it once we have actual
support. "so we don't raise people's hope."

-- 

Thanks,

David

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

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:58 +0200
David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7754e3eaf9..eff96808c4 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"
> @@ -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?

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

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

[David Hildenbrand]

On 31.08.2017 16:41, Cornelia Huck wrote:
> On Wed, 30 Aug 2017 19:05:58 +0200
> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 7754e3eaf9..eff96808c4 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"
>> @@ -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?

As an alternative: yield a warning?

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


-- 

Thanks,

David

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

[Cornelia Huck]

On Thu, 31 Aug 2017 17:03:21 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 31.08.2017 16:41, Cornelia Huck wrote:
> > On Wed, 30 Aug 2017 19:05:58 +0200
> > David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> >> ---
> >>  hw/s390x/s390-virtio-ccw.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 7754e3eaf9..eff96808c4 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"
> >> @@ -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?  
> 
> As an alternative: yield a warning?

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

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

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

[David Hildenbrand]

On 31.08.2017 17:07, Cornelia Huck wrote:
> On Thu, 31 Aug 2017 17:03:21 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 31.08.2017 16:41, Cornelia Huck wrote:
>>> On Wed, 30 Aug 2017 19:05:58 +0200
>>> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>>>> ---
>>>>  hw/s390x/s390-virtio-ccw.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 7754e3eaf9..eff96808c4 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"
>>>> @@ -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?  
>>
>> As an alternative: yield a warning?
> 
> If we know that this can't work, it makes sense to stop immediately, no?

Hmm, like that than?

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 18ed0c57e3..dae848fa5f 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"
@@ -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);

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7ed9103b33..4e1de2102d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -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


-- 

Thanks,

David

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

[Cornelia Huck]

On Thu, 31 Aug 2017 17:33:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 31.08.2017 17:07, Cornelia Huck wrote:

> > If we know that this can't work, it makes sense to stop immediately, no?  
> 
> Hmm, like that than?
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 18ed0c57e3..dae848fa5f 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"
> @@ -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);
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7ed9103b33..4e1de2102d 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -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
> 
> 

Looks good!

[Qemu-devel] [PATCH v1 09/11] target/s390x: tcg_s390_program_interrupt() will never return

[David Hildenbrand]

The assert should hold in both scenarios.

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

diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
index 058e219fe5..79bab5e2f3 100644
--- a/target/s390x/interrupt.c
+++ b/target/s390x/interrupt.c
@@ -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();
 #endif
+    g_assert_not_reached();
 }
 
 void program_interrupt(CPUS390XState *env, uint32_t code, int ilen)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 09/11] target/s390x: tcg_s390_program_interrupt() will never return

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand wrote:
> The assert should hold in both scenarios.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/interrupt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
> index 058e219fe5..79bab5e2f3 100644
> --- a/target/s390x/interrupt.c
> +++ b/target/s390x/interrupt.c
> @@ -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();
>  #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

Re: [Qemu-devel] [PATCH v1 09/11] target/s390x: tcg_s390_program_interrupt() will never return

[David Hildenbrand]

On 30.08.2017 22:45, Thomas Huth wrote:
> On 30.08.2017 19:05, David Hildenbrand wrote:
>> The assert should hold in both scenarios.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/interrupt.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
>> index 058e219fe5..79bab5e2f3 100644
>> --- a/target/s390x/interrupt.c
>> +++ b/target/s390x/interrupt.c
>> @@ -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();
>>  #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.

Good point, I'll drop this patch. Thanks!

> 
>  Thomas
> 


-- 

Thanks,

David

[Qemu-devel] [PATCH v1 10/11] target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

[David Hildenbrand]

This looks cleaner.

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 2ac36535f7..f5f5967833 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 v1 10/11] target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

[Thomas Huth]

On 30.08.2017 19:06, David Hildenbrand wrote:
> This looks cleaner.
> 
> 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 2ac36535f7..f5f5967833 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;

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

 Thomas

Re: [Qemu-devel] [PATCH v1 10/11] target/s390x: use trigger_pgm_exception() in s390_cpu_handle_mmu_fault()

[David Hildenbrand]

On 30.08.2017 21:11, Thomas Huth wrote:
> On 30.08.2017 19:06, David Hildenbrand wrote:
>> This looks cleaner.
>>
>> 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 2ac36535f7..f5f5967833 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;
> 
> trigger_pgm_exception() additionally sets int_pgm_ilen ... I assume
> that's OK here? A comment in the patch description would be helpful.
> 

As far as I can tell, ilen is completely ignored for linux-user (see
cpu_loop() in linux-user/main.c)

Will add a comment, thanks!

>  Thomas
> 


-- 

Thanks,

David

[Qemu-devel] [PATCH v1 11/11] target/s390x: use program_interrupt() in per_check_exception()

[David Hildenbrand]

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 <david@redhat.com>
---
 target/s390x/misc_helper.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index eb7accc0ce..ac9657f23f 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -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;
 
     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);
+        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
+        program_interrupt(env, PGM_PER, ilen);
     }
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 11/11] target/s390x: use program_interrupt() in per_check_exception()

[Thomas Huth]

On 30.08.2017 19:06, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  target/s390x/misc_helper.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index eb7accc0ce..ac9657f23f 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -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;
>  
>      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);
> +        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
> +        program_interrupt(env, PGM_PER, ilen);
>      }
>  }

The changes basically look fine to me, but may I suggest to
1) Add a comment to the code about your concerns with ilen
2) Change the patch description to focus on the work that is actually
done here instead of only talking about your ilen concerns?

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH v1 00/11] next round of s390x cleanups

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:50 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 :)

If you want to clean up the other unneeded includes of cpu-all.h as
well, it might make sense to send them separately. Otherwise, I'm happy
to route them through s390-next if nobody complains.

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

Looking forward to v2 :)

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