[Qemu-devel] [PULL 0/3] s390x/kvm: some fixes and cleanups

[Qemu-devel] [PULL 0/3] s390x/kvm: some fixes and cleanups

[Christian Borntraeger]

Peter,

The following changes since commit 07e2863d0271ac6c05206d8ce9e4f4c39b25d3ea:

  exec.c: fix setting 1-byte-long watchpoints (2014-09-19 17:42:16 +0100)

are available in the git repository at:

  git://github.com/borntraeger/qemu.git tags/s390x-20140923

for you to fetch changes up to e8601dd5d0ffa909068ddefe33bf6a53d8af063a:

  s390x/css: catch ccw sequence errors (2014-09-23 14:10:17 +0200)

----------------------------------------------------------------
s390x/kvm: some fixes and cleanups

1. sclp: get of of duplicate defines
2. ccw: implement and fix handling of some special cases

----------------------------------------------------------------
Cornelia Huck (2):
      s390x/css: support format-0 ccws
      s390x/css: catch ccw sequence errors

Jens Freimann (1):
      s390x: remove duplicate defines in SCLP code

 hw/s390x/css.c          | 40 +++++++++++++++++++++++++++++++---------
 hw/s390x/css.h          |  2 ++
 include/hw/s390x/sclp.h |  2 --
 target-s390x/ioinst.h   | 10 ++++++++++
 4 files changed, 43 insertions(+), 11 deletions(-)

[Qemu-devel] [PULL 1/3] s390x: remove duplicate defines in SCLP code

[Christian Borntraeger]

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

Let's get rid of these duplicate defines.

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/hw/s390x/sclp.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 5c43574..ec07a11 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -28,8 +28,6 @@
 #define SCLP_UNASSIGN_STORAGE                   0x000C0001
 #define SCLP_CMD_READ_EVENT_DATA                0x00770005
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
-#define SCLP_CMD_READ_EVENT_DATA                0x00770005
-#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
 
 /* SCLP Memory hotplug codes */
-- 
1.9.3

[Qemu-devel] [PULL 2/3] s390x/css: support format-0 ccws

[Christian Borntraeger]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

Add support for format-0 ccws in channel programs. As a format-1 ccw
contains the same information as format-0 ccws, only supporting larger
addresses, simply convert every ccw to format-1 as we walk the chain.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/css.c        | 30 +++++++++++++++++++++---------
 hw/s390x/css.h        |  1 +
 target-s390x/ioinst.h | 10 ++++++++++
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 49c2aaf..34637cb 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -243,17 +243,25 @@ static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
     }
 }
 
-static CCW1 copy_ccw_from_guest(hwaddr addr)
+static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
 {
-    CCW1 tmp;
+    CCW0 tmp0;
+    CCW1 tmp1;
     CCW1 ret;
 
-    cpu_physical_memory_read(addr, &tmp, sizeof(tmp));
-    ret.cmd_code = tmp.cmd_code;
-    ret.flags = tmp.flags;
-    ret.count = be16_to_cpu(tmp.count);
-    ret.cda = be32_to_cpu(tmp.cda);
-
+    if (fmt1) {
+        cpu_physical_memory_read(addr, &tmp1, sizeof(tmp1));
+        ret.cmd_code = tmp1.cmd_code;
+        ret.flags = tmp1.flags;
+        ret.count = be16_to_cpu(tmp1.count);
+        ret.cda = be32_to_cpu(tmp1.cda);
+    } else {
+        cpu_physical_memory_read(addr, &tmp0, sizeof(tmp0));
+        ret.cmd_code = tmp0.cmd_code;
+        ret.flags = tmp0.flags;
+        ret.count = be16_to_cpu(tmp0.count);
+        ret.cda = be16_to_cpu(tmp0.cda1) | (tmp0.cda0 << 16);
+    }
     return ret;
 }
 
@@ -268,7 +276,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr)
         return -EIO;
     }
 
-    ccw = copy_ccw_from_guest(ccw_addr);
+    /* Translate everything to format-1 ccws - the information is the same. */
+    ccw = copy_ccw_from_guest(ccw_addr, sch->ccw_fmt_1);
 
     /* Check for invalid command codes. */
     if ((ccw.cmd_code & 0x0f) == 0) {
@@ -386,6 +395,7 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
             s->ctrl |= (SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND);
             return;
         }
+        sch->ccw_fmt_1 = !!(orb->ctrl0 & ORB_CTRL0_MASK_FMT);
     } else {
         s->ctrl &= ~(SCSW_ACTL_SUSP | SCSW_ACTL_RESUME_PEND);
     }
@@ -1347,6 +1357,7 @@ void subch_device_save(SubchDev *s, QEMUFile *f)
         qemu_put_byte(f, s->id.ciw[i].command);
         qemu_put_be16(f, s->id.ciw[i].count);
     }
+    qemu_put_byte(f, s->ccw_fmt_1);
     return;
 }
 
@@ -1402,6 +1413,7 @@ int subch_device_load(SubchDev *s, QEMUFile *f)
         s->id.ciw[i].command = qemu_get_byte(f);
         s->id.ciw[i].count = qemu_get_be16(f);
     }
+    s->ccw_fmt_1 = qemu_get_byte(f);
     return 0;
 }
 
diff --git a/hw/s390x/css.h b/hw/s390x/css.h
index c864ea7..384a455 100644
--- a/hw/s390x/css.h
+++ b/hw/s390x/css.h
@@ -76,6 +76,7 @@ struct SubchDev {
     hwaddr channel_prog;
     CCW1 last_cmd;
     bool last_cmd_valid;
+    bool ccw_fmt_1;
     bool thinint_active;
     /* transport-provided data: */
     int (*ccw_cb) (SubchDev *, CCW1);
diff --git a/target-s390x/ioinst.h b/target-s390x/ioinst.h
index 5bbc67d..29f6423 100644
--- a/target-s390x/ioinst.h
+++ b/target-s390x/ioinst.h
@@ -156,6 +156,16 @@ typedef struct ORB {
 #define ORB_CTRL1_MASK_ORBX 0x01
 #define ORB_CTRL1_MASK_INVALID 0x3e
 
+/* channel command word (type 0) */
+typedef struct CCW0 {
+        uint8_t cmd_code;
+        uint8_t cda0;
+        uint16_t cda1;
+        uint8_t flags;
+        uint8_t reserved;
+        uint16_t count;
+} QEMU_PACKED CCW0;
+
 /* channel command word (type 1) */
 typedef struct CCW1 {
     uint8_t cmd_code;
-- 
1.9.3

Re: [Qemu-devel] [PULL 2/3] s390x/css: support format-0 ccws

[Andreas Färber]

Am 23.09.2014 um 14:14 schrieb Christian Borntraeger:
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Add support for format-0 ccws in channel programs. As a format-1 ccw
> contains the same information as format-0 ccws, only supporting larger
> addresses, simply convert every ccw to format-1 as we walk the chain.
> 
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/css.c        | 30 +++++++++++++++++++++---------
>  hw/s390x/css.h        |  1 +
>  target-s390x/ioinst.h | 10 ++++++++++
>  3 files changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 49c2aaf..34637cb 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -243,17 +243,25 @@ static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
>      }
>  }
>  
> -static CCW1 copy_ccw_from_guest(hwaddr addr)
> +static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
>  {
> -    CCW1 tmp;
> +    CCW0 tmp0;
> +    CCW1 tmp1;
>      CCW1 ret;
>  
> -    cpu_physical_memory_read(addr, &tmp, sizeof(tmp));
> -    ret.cmd_code = tmp.cmd_code;
> -    ret.flags = tmp.flags;
> -    ret.count = be16_to_cpu(tmp.count);
> -    ret.cda = be32_to_cpu(tmp.cda);
> -
> +    if (fmt1) {
> +        cpu_physical_memory_read(addr, &tmp1, sizeof(tmp1));
> +        ret.cmd_code = tmp1.cmd_code;
> +        ret.flags = tmp1.flags;
> +        ret.count = be16_to_cpu(tmp1.count);
> +        ret.cda = be32_to_cpu(tmp1.cda);
> +    } else {
> +        cpu_physical_memory_read(addr, &tmp0, sizeof(tmp0));
> +        ret.cmd_code = tmp0.cmd_code;
> +        ret.flags = tmp0.flags;
> +        ret.count = be16_to_cpu(tmp0.count);
> +        ret.cda = be16_to_cpu(tmp0.cda1) | (tmp0.cda0 << 16);
> +    }
>      return ret;
>  }
>  
> @@ -268,7 +276,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr)
>          return -EIO;
>      }
>  
> -    ccw = copy_ccw_from_guest(ccw_addr);
> +    /* Translate everything to format-1 ccws - the information is the same. */
> +    ccw = copy_ccw_from_guest(ccw_addr, sch->ccw_fmt_1);
>  
>      /* Check for invalid command codes. */
>      if ((ccw.cmd_code & 0x0f) == 0) {
> @@ -386,6 +395,7 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
>              s->ctrl |= (SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND);
>              return;
>          }
> +        sch->ccw_fmt_1 = !!(orb->ctrl0 & ORB_CTRL0_MASK_FMT);
>      } else {
>          s->ctrl &= ~(SCSW_ACTL_SUSP | SCSW_ACTL_RESUME_PEND);
>      }
> @@ -1347,6 +1357,7 @@ void subch_device_save(SubchDev *s, QEMUFile *f)
>          qemu_put_byte(f, s->id.ciw[i].command);
>          qemu_put_be16(f, s->id.ciw[i].count);
>      }
> +    qemu_put_byte(f, s->ccw_fmt_1);
>      return;
>  }
>  
> @@ -1402,6 +1413,7 @@ int subch_device_load(SubchDev *s, QEMUFile *f)
>          s->id.ciw[i].command = qemu_get_byte(f);
>          s->id.ciw[i].count = qemu_get_be16(f);
>      }
> +    s->ccw_fmt_1 = qemu_get_byte(f);
>      return 0;
>  }
>  

Same load/save issue here as in 3/3.

Regards,
Andreas

> diff --git a/hw/s390x/css.h b/hw/s390x/css.h
> index c864ea7..384a455 100644
> --- a/hw/s390x/css.h
> +++ b/hw/s390x/css.h
> @@ -76,6 +76,7 @@ struct SubchDev {
>      hwaddr channel_prog;
>      CCW1 last_cmd;
>      bool last_cmd_valid;
> +    bool ccw_fmt_1;
>      bool thinint_active;
>      /* transport-provided data: */
>      int (*ccw_cb) (SubchDev *, CCW1);
> diff --git a/target-s390x/ioinst.h b/target-s390x/ioinst.h
> index 5bbc67d..29f6423 100644
> --- a/target-s390x/ioinst.h
> +++ b/target-s390x/ioinst.h
> @@ -156,6 +156,16 @@ typedef struct ORB {
>  #define ORB_CTRL1_MASK_ORBX 0x01
>  #define ORB_CTRL1_MASK_INVALID 0x3e
>  
> +/* channel command word (type 0) */
> +typedef struct CCW0 {
> +        uint8_t cmd_code;
> +        uint8_t cda0;
> +        uint16_t cda1;
> +        uint8_t flags;
> +        uint8_t reserved;
> +        uint16_t count;
> +} QEMU_PACKED CCW0;
> +
>  /* channel command word (type 1) */
>  typedef struct CCW1 {
>      uint8_t cmd_code;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PULL 3/3] s390x/css: catch ccw sequence errors

[Christian Borntraeger]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

We must not allow chains of more than 255 ccws without data transfer.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/css.c | 10 ++++++++++
 hw/s390x/css.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 34637cb..b67c039 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -294,6 +294,13 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr)
 
     check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
 
+    if (!ccw.cda) {
+        if (sch->ccw_no_data_cnt == 255) {
+            return -EINVAL;
+        }
+        sch->ccw_no_data_cnt++;
+    }
+
     /* Look at the command. */
     switch (ccw.cmd_code) {
     case CCW_CMD_NOOP:
@@ -396,6 +403,7 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
             return;
         }
         sch->ccw_fmt_1 = !!(orb->ctrl0 & ORB_CTRL0_MASK_FMT);
+        sch->ccw_no_data_cnt = 0;
     } else {
         s->ctrl &= ~(SCSW_ACTL_SUSP | SCSW_ACTL_RESUME_PEND);
     }
@@ -1358,6 +1366,7 @@ void subch_device_save(SubchDev *s, QEMUFile *f)
         qemu_put_be16(f, s->id.ciw[i].count);
     }
     qemu_put_byte(f, s->ccw_fmt_1);
+    qemu_put_byte(f, s->ccw_no_data_cnt);
     return;
 }
 
@@ -1414,6 +1423,7 @@ int subch_device_load(SubchDev *s, QEMUFile *f)
         s->id.ciw[i].count = qemu_get_be16(f);
     }
     s->ccw_fmt_1 = qemu_get_byte(f);
+    s->ccw_no_data_cnt = qemu_get_byte(f);
     return 0;
 }
 
diff --git a/hw/s390x/css.h b/hw/s390x/css.h
index 384a455..33104ac 100644
--- a/hw/s390x/css.h
+++ b/hw/s390x/css.h
@@ -78,6 +78,7 @@ struct SubchDev {
     bool last_cmd_valid;
     bool ccw_fmt_1;
     bool thinint_active;
+    uint8_t ccw_no_data_cnt;
     /* transport-provided data: */
     int (*ccw_cb) (SubchDev *, CCW1);
     SenseId id;
-- 
1.9.3

Re: [Qemu-devel] [PULL 3/3] s390x/css: catch ccw sequence errors

[Andreas Färber]

Christian, Conny,

Am 23.09.2014 um 14:14 schrieb Christian Borntraeger:
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> We must not allow chains of more than 255 ccws without data transfer.
> 
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/css.c | 10 ++++++++++
>  hw/s390x/css.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 34637cb..b67c039 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -294,6 +294,13 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr)
>  
>      check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
>  
> +    if (!ccw.cda) {
> +        if (sch->ccw_no_data_cnt == 255) {
> +            return -EINVAL;
> +        }
> +        sch->ccw_no_data_cnt++;
> +    }
> +
>      /* Look at the command. */
>      switch (ccw.cmd_code) {
>      case CCW_CMD_NOOP:
> @@ -396,6 +403,7 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
>              return;
>          }
>          sch->ccw_fmt_1 = !!(orb->ctrl0 & ORB_CTRL0_MASK_FMT);
> +        sch->ccw_no_data_cnt = 0;
>      } else {
>          s->ctrl &= ~(SCSW_ACTL_SUSP | SCSW_ACTL_RESUME_PEND);
>      }
> @@ -1358,6 +1366,7 @@ void subch_device_save(SubchDev *s, QEMUFile *f)
>          qemu_put_be16(f, s->id.ciw[i].count);
>      }
>      qemu_put_byte(f, s->ccw_fmt_1);
> +    qemu_put_byte(f, s->ccw_no_data_cnt);
>      return;
>  }
>  
> @@ -1414,6 +1423,7 @@ int subch_device_load(SubchDev *s, QEMUFile *f)
>          s->id.ciw[i].count = qemu_get_be16(f);
>      }
>      s->ccw_fmt_1 = qemu_get_byte(f);
> +    s->ccw_no_data_cnt = qemu_get_byte(f);
>      return 0;
>  }
>  

You need to bump the version saved if you add fields (same as VMState),
and on load you need to check the version for whether to load the field
(or deny loading).

Regards,
Andreas

> diff --git a/hw/s390x/css.h b/hw/s390x/css.h
> index 384a455..33104ac 100644
> --- a/hw/s390x/css.h
> +++ b/hw/s390x/css.h
> @@ -78,6 +78,7 @@ struct SubchDev {
>      bool last_cmd_valid;
>      bool ccw_fmt_1;
>      bool thinint_active;
> +    uint8_t ccw_no_data_cnt;
>      /* transport-provided data: */
>      int (*ccw_cb) (SubchDev *, CCW1);
>      SenseId id;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 3/3] s390x/css: catch ccw sequence errors

[Christian Borntraeger]

On 09/26/2014 12:36 PM, Andreas Färber wrote:

>> @@ -1414,6 +1423,7 @@ int subch_device_load(SubchDev *s, QEMUFile *f)
>>          s->id.ciw[i].count = qemu_get_be16(f);
>>      }
>>      s->ccw_fmt_1 = qemu_get_byte(f);
>> +    s->ccw_no_data_cnt = qemu_get_byte(f);
>>      return 0;
>>  }
>>  
> 
> You need to bump the version saved if you add fields (same as VMState),
> and on load you need to check the version for whether to load the field
> (or deny loading).

Alex brough up the same thing. As the cpu migration code is not yet upstream (in prep right now) we concluded that this is still ok.

http://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01094.html

Re: [Qemu-devel] [PULL 3/3] s390x/css: catch ccw sequence errors

[Andreas Färber]

Am 26.09.2014 um 12:49 schrieb Christian Borntraeger:
> On 09/26/2014 12:36 PM, Andreas Färber wrote:
> 
>>> @@ -1414,6 +1423,7 @@ int subch_device_load(SubchDev *s, QEMUFile *f)
>>>          s->id.ciw[i].count = qemu_get_be16(f);
>>>      }
>>>      s->ccw_fmt_1 = qemu_get_byte(f);
>>> +    s->ccw_no_data_cnt = qemu_get_byte(f);
>>>      return 0;
>>>  }
>>>  
>>
>> You need to bump the version saved if you add fields (same as VMState),
>> and on load you need to check the version for whether to load the field
>> (or deny loading).
> 
> Alex brough up the same thing. As the cpu migration code is not yet upstream (in prep right now) we concluded that this is still ok.
> 
> http://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01094.html

You should justify that in each such commit message then.

However, the number space is large enough to bump the number each time
to avoid forgetting it later.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 0/3] s390x/kvm: some fixes and cleanups

[Peter Maydell]

On 23 September 2014 13:14, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> Peter,
>
> The following changes since commit 07e2863d0271ac6c05206d8ce9e4f4c39b25d3ea:
>
>   exec.c: fix setting 1-byte-long watchpoints (2014-09-19 17:42:16 +0100)
>
> are available in the git repository at:
>
>   git://github.com/borntraeger/qemu.git tags/s390x-20140923
>
> for you to fetch changes up to e8601dd5d0ffa909068ddefe33bf6a53d8af063a:
>
>   s390x/css: catch ccw sequence errors (2014-09-23 14:10:17 +0200)
>
> ----------------------------------------------------------------
> s390x/kvm: some fixes and cleanups
>
> 1. sclp: get of of duplicate defines
> 2. ccw: implement and fix handling of some special cases
>
> ----------------------------------------------------------------
> Cornelia Huck (2):
>       s390x/css: support format-0 ccws
>       s390x/css: catch ccw sequence errors
>
> Jens Freimann (1):
>       s390x: remove duplicate defines in SCLP code
>
>  hw/s390x/css.c          | 40 +++++++++++++++++++++++++++++++---------
>  hw/s390x/css.h          |  2 ++
>  include/hw/s390x/sclp.h |  2 --
>  target-s390x/ioinst.h   | 10 ++++++++++
>  4 files changed, 43 insertions(+), 11 deletions(-)

Applied, thanks.

-- PMM