[PATCH v3 0/4] s390x: Fix IRB sense data

[PATCH v3 0/4] s390x: Fix IRB sense data

[Eric Farman]

Conny, et al,

Here is a (significantly larger) set of patches to address
the problem with building passsthrough sense data in an IRB,
using a subchannel-specific callback.

The first three patches are primarily code refactoring.
But, since patch 3 doesn't implement the callback for
vfio-ccw subchannels, it fixes the problem encountered
with "dasdfmt -M quick" failing to run correctly in the
guest because we no longer fill out the other stuff in
the IRB associated with the sense data.

Patch 4 implements that for vfio-ccw.

v2->v3:
 - [EF] Drop Fixes tag
 - [CH] Implement a callback for the IRB sense data
 - [CH] Copy IRB.ESW from passthrough hardware
 - [CH] Only put sense in IRB.ECW if passthrough device did

v1->v2:
 - [MR] Add Fixes: tags
 - [CH] Reinstate the memcpy(sch->sense_data, irb.ecw) in vfio_ccw
 - [CH] Look at IRB.SCSW.E before copying sense into guest IRB

v2: https://lore.kernel.org/qemu-devel/20210611202151.615410-1-farman@linux.ibm.com/
v1: https://lore.kernel.org/qemu-devel/20210610202011.391029-1-farman@linux.ibm.com/

Eric Farman (4):
  s390x/css: Introduce an ESW struct
  s390x/css: Split out the IRB sense data
  s390x/css: Refactor IRB construction
  s390x/css: Add passthrough IRB

 hw/s390x/3270-ccw.c       |  1 +
 hw/s390x/css.c            | 84 ++++++++++++++++++++++++++++-----------
 hw/s390x/s390-ccw.c       |  1 +
 hw/s390x/virtio-ccw.c     |  1 +
 hw/vfio/ccw.c             |  4 ++
 include/hw/s390x/css.h    |  5 +++
 include/hw/s390x/ioinst.h | 12 +++++-
 7 files changed, 83 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] s390x/css: Introduce an ESW struct

[Eric Farman]

The Interrupt Response Block is comprised of several other
structures concatenated together, but only the 12-byte
Subchannel-Status Word (SCSW) is defined as a proper struct.
Everything else is a simple array of 32-bit words.

Let's define a proper struct for the 20-byte Extended-Status
Word (ESW) so that we can make good decisions about the sense
data that would go into the ECW area for virtual vs
passthrough devices.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c            | 19 +++++++++++++------
 include/hw/s390x/ioinst.h | 12 +++++++++++-
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index bed46f5ec3..8be21efb13 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1335,6 +1335,14 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
     }
 }
 
+static void copy_esw_to_guest(ESW *dest, const ESW *src)
+{
+    dest->sublog = cpu_to_be32(src->sublog);
+    dest->erw = cpu_to_be32(src->erw);
+    dest->f_addr = cpu_to_be64(src->f_addr);
+    dest->s_addr = cpu_to_be32(src->s_addr);
+}
+
 IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
 {
     int ret;
@@ -1604,9 +1612,8 @@ static void copy_irb_to_guest(IRB *dest, const IRB *src, const PMCW *pmcw,
 
     copy_scsw_to_guest(&dest->scsw, &src->scsw);
 
-    for (i = 0; i < ARRAY_SIZE(dest->esw); i++) {
-        dest->esw[i] = cpu_to_be32(src->esw[i]);
-    }
+    copy_esw_to_guest(&dest->esw, &src->esw);
+
     for (i = 0; i < ARRAY_SIZE(dest->ecw); i++) {
         dest->ecw[i] = cpu_to_be32(src->ecw[i]);
     }
@@ -1655,9 +1662,9 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
                         SCSW_CSTAT_CHN_CTRL_CHK |
                         SCSW_CSTAT_INTF_CTRL_CHK)) {
             irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF;
-            irb.esw[0] = 0x04804000;
+            irb.esw.sublog = 0x04804000;
         } else {
-            irb.esw[0] = 0x00800000;
+            irb.esw.sublog = 0x00800000;
         }
         /* If a unit check is pending, copy sense data. */
         if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
@@ -1670,7 +1677,7 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
             for (i = 0; i < ARRAY_SIZE(irb.ecw); i++) {
                 irb.ecw[i] = be32_to_cpu(irb.ecw[i]);
             }
-            irb.esw[1] = 0x01000000 | (sizeof(sch->sense_data) << 8);
+            irb.esw.erw = ESW_ERW_SENSE | (sizeof(sch->sense_data) << 8);
         }
     }
     /* Store the irb to the guest. */
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index c6737a30d4..9613e0ccbb 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -123,10 +123,20 @@ typedef struct SCHIB {
     uint8_t mda[4];
 } QEMU_PACKED SCHIB;
 
+/* extended-status word */
+typedef struct ESW {
+   uint32_t sublog;
+   uint32_t erw;
+   uint64_t f_addr;
+   uint32_t s_addr;
+} QEMU_PACKED ESW;
+
+#define ESW_ERW_SENSE 0x01000000
+
 /* interruption response block */
 typedef struct IRB {
     SCSW scsw;
-    uint32_t esw[5];
+    ESW esw;
     uint32_t ecw[8];
     uint32_t emw[8];
 } IRB;
-- 
2.25.1

[PATCH v3 2/4] s390x/css: Split out the IRB sense data

[Eric Farman]

Let's move this logic into its own routine,
so it can be reused later.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 8be21efb13..554c9083fd 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1639,6 +1639,17 @@ static void copy_irb_to_guest(IRB *dest, const IRB *src, const PMCW *pmcw,
     *irb_len = sizeof(*dest);
 }
 
+static void build_irb_sense_data(SubchDev *sch, IRB *irb)
+{
+    int i;
+
+    /* Attention: sense_data is already BE! */
+    memcpy(irb->ecw, sch->sense_data, sizeof(sch->sense_data));
+    for (i = 0; i < ARRAY_SIZE(irb->ecw); i++) {
+        irb->ecw[i] = be32_to_cpu(irb->ecw[i]);
+    }
+}
+
 int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
 {
     SCHIB *schib = &sch->curr_status;
@@ -1669,14 +1680,8 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
         /* If a unit check is pending, copy sense data. */
         if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
             (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
-            int i;
-
             irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF | SCSW_FLAGS_MASK_ECTL;
-            /* Attention: sense_data is already BE! */
-            memcpy(irb.ecw, sch->sense_data, sizeof(sch->sense_data));
-            for (i = 0; i < ARRAY_SIZE(irb.ecw); i++) {
-                irb.ecw[i] = be32_to_cpu(irb.ecw[i]);
-            }
+            build_irb_sense_data(sch, &irb);
             irb.esw.erw = ESW_ERW_SENSE | (sizeof(sch->sense_data) << 8);
         }
     }
-- 
2.25.1

[PATCH v3 3/4] s390x/css: Refactor IRB construction

[Eric Farman]

Currently, all subchannel types have "sense data" copied into
the IRB.ECW space, and a couple flags enabled in the IRB.SCSW
and IRB.ESW. But for passthrough (vfio-ccw) subchannels,
this data isn't populated in the first place, so enabling
those flags leads to unexpected behavior if the guest tries to
process the sense data (zeros) in the IRB.ECW.

Let's add a subchannel callback that builds these portions of
the IRB, and move the existing code into a routine for those
virtual subchannels. The passthrough subchannels will be able
to piggy-back onto this later.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/3270-ccw.c    |  1 +
 hw/s390x/css.c         | 45 +++++++++++++++++++++++++++---------------
 hw/s390x/virtio-ccw.c  |  1 +
 include/hw/s390x/css.h |  2 ++
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 13e93d8d8f..69e6783ade 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -129,6 +129,7 @@ static void emulated_ccw_3270_realize(DeviceState *ds, Error **errp)
                                 EMULATED_CCW_3270_CHPID_TYPE);
     sch->do_subchannel_work = do_subchannel_work_virtual;
     sch->ccw_cb = emulated_ccw_3270_cb;
+    sch->irb_cb = build_irb_virtual;
 
     ck->init(dev, &err);
     if (err) {
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 554c9083fd..9bd6e512eb 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1650,6 +1650,30 @@ static void build_irb_sense_data(SubchDev *sch, IRB *irb)
     }
 }
 
+void build_irb_virtual(SubchDev *sch, IRB *irb)
+{
+    SCHIB *schib = &sch->curr_status;
+    uint16_t stctl = schib->scsw.ctrl & SCSW_CTRL_MASK_STCTL;
+
+    if (stctl & SCSW_STCTL_STATUS_PEND) {
+        if (schib->scsw.cstat & (SCSW_CSTAT_DATA_CHECK |
+                        SCSW_CSTAT_CHN_CTRL_CHK |
+                        SCSW_CSTAT_INTF_CTRL_CHK)) {
+            irb->scsw.flags |= SCSW_FLAGS_MASK_ESWF;
+            irb->esw.sublog = 0x04804000;
+        } else {
+            irb->esw.sublog = 0x00800000;
+        }
+        /* If a unit check is pending, copy sense data. */
+        if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
+            (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
+            irb->scsw.flags |= SCSW_FLAGS_MASK_ESWF | SCSW_FLAGS_MASK_ECTL;
+            build_irb_sense_data(sch, irb);
+            irb->esw.erw = ESW_ERW_SENSE | (sizeof(sch->sense_data) << 8);
+        }
+    }
+}
+
 int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
 {
     SCHIB *schib = &sch->curr_status;
@@ -1668,23 +1692,12 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
 
     /* Copy scsw from current status. */
     irb.scsw = schib->scsw;
-    if (stctl & SCSW_STCTL_STATUS_PEND) {
-        if (schib->scsw.cstat & (SCSW_CSTAT_DATA_CHECK |
-                        SCSW_CSTAT_CHN_CTRL_CHK |
-                        SCSW_CSTAT_INTF_CTRL_CHK)) {
-            irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF;
-            irb.esw.sublog = 0x04804000;
-        } else {
-            irb.esw.sublog = 0x00800000;
-        }
-        /* If a unit check is pending, copy sense data. */
-        if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
-            (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
-            irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF | SCSW_FLAGS_MASK_ECTL;
-            build_irb_sense_data(sch, &irb);
-            irb.esw.erw = ESW_ERW_SENSE | (sizeof(sch->sense_data) << 8);
-        }
+
+    /* Build other IRB data, if necessary */
+    if (sch->irb_cb) {
+        sch->irb_cb(sch, &irb);
     }
+
     /* Store the irb to the guest. */
     p = schib->pmcw;
     copy_irb_to_guest(target_irb, &irb, &p, irb_len);
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 220b9efcf9..d68888fccd 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -753,6 +753,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
     sch->id.reserved = 0xff;
     sch->id.cu_type = VIRTIO_CCW_CU_TYPE;
     sch->do_subchannel_work = do_subchannel_work_virtual;
+    sch->irb_cb = build_irb_virtual;
     ccw_dev->sch = sch;
     dev->indicators = NULL;
     dev->revision = -1;
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index bba7593d2e..7c23a13f3d 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -138,6 +138,7 @@ struct SubchDev {
     int (*ccw_cb) (SubchDev *, CCW1);
     void (*disable_cb)(SubchDev *);
     IOInstEnding (*do_subchannel_work) (SubchDev *);
+    void (*irb_cb)(SubchDev *, IRB *);
     SenseId id;
     void *driver_data;
 };
@@ -215,6 +216,7 @@ void css_clear_sei_pending(void);
 IOInstEnding s390_ccw_cmd_request(SubchDev *sch);
 IOInstEnding do_subchannel_work_virtual(SubchDev *sub);
 IOInstEnding do_subchannel_work_passthrough(SubchDev *sub);
+void build_irb_virtual(SubchDev *sch, IRB *irb);
 
 int s390_ccw_halt(SubchDev *sch);
 int s390_ccw_clear(SubchDev *sch);
-- 
2.25.1

[PATCH v3 4/4] s390x/css: Add passthrough IRB

[Eric Farman]

Wire in the subchannel callback for building the IRB
ESW and ECW space for passthrough devices, and copy
the hardware's ESW into the IRB we are building.

If the hardware presented concurrent sense, then copy
that sense data into the IRB's ECW space.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c         | 13 ++++++++++++-
 hw/s390x/s390-ccw.c    |  1 +
 hw/vfio/ccw.c          |  4 ++++
 include/hw/s390x/css.h |  3 +++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 9bd6e512eb..90b74b8b58 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1335,7 +1335,7 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
     }
 }
 
-static void copy_esw_to_guest(ESW *dest, const ESW *src)
+void copy_esw_to_guest(ESW *dest, const ESW *src)
 {
     dest->sublog = cpu_to_be32(src->sublog);
     dest->erw = cpu_to_be32(src->erw);
@@ -1650,6 +1650,17 @@ static void build_irb_sense_data(SubchDev *sch, IRB *irb)
     }
 }
 
+void build_irb_passthrough(SubchDev *sch, IRB *irb)
+{
+    /* Copy ESW from hardware */
+    irb->esw = sch->esw;
+
+    if (irb->esw.erw & ESW_ERW_SENSE) {
+        /* Copy ECW from hardware */
+        build_irb_sense_data(sch, irb);
+    }
+}
+
 void build_irb_virtual(SubchDev *sch, IRB *irb)
 {
     SCHIB *schib = &sch->curr_status;
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index c227c77984..2fc8bb9c23 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -124,6 +124,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
     }
     sch->driver_data = cdev;
     sch->do_subchannel_work = do_subchannel_work_passthrough;
+    sch->irb_cb = build_irb_passthrough;
 
     ccw_dev->sch = sch;
     ret = css_sch_build_schib(sch, &cdev->hostid);
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 139a3d9d1b..000992fb9f 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -321,6 +321,7 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
     SCHIB *schib = &sch->curr_status;
     SCSW s;
     IRB irb;
+    ESW esw;
     int size;
 
     if (!event_notifier_test_and_clear(&vcdev->io_notifier)) {
@@ -371,6 +372,9 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
     copy_scsw_to_guest(&s, &irb.scsw);
     schib->scsw = s;
 
+    copy_esw_to_guest(&esw, &irb.esw);
+    sch->esw = esw;
+
     /* If a uint check is pending, copy sense data. */
     if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
         (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 7c23a13f3d..10ed1df1bb 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -141,6 +141,7 @@ struct SubchDev {
     void (*irb_cb)(SubchDev *, IRB *);
     SenseId id;
     void *driver_data;
+    ESW esw;
 };
 
 static inline void sch_gen_unit_exception(SubchDev *sch)
@@ -202,6 +203,7 @@ int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id);
 unsigned int css_find_free_chpid(uint8_t cssid);
 uint16_t css_build_subchannel_id(SubchDev *sch);
 void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
+void copy_esw_to_guest(ESW *dest, const ESW *src);
 void css_inject_io_interrupt(SubchDev *sch);
 void css_reset(void);
 void css_reset_sch(SubchDev *sch);
@@ -216,6 +218,7 @@ void css_clear_sei_pending(void);
 IOInstEnding s390_ccw_cmd_request(SubchDev *sch);
 IOInstEnding do_subchannel_work_virtual(SubchDev *sub);
 IOInstEnding do_subchannel_work_passthrough(SubchDev *sub);
+void build_irb_passthrough(SubchDev *sch, IRB *irb);
 void build_irb_virtual(SubchDev *sch, IRB *irb);
 
 int s390_ccw_halt(SubchDev *sch);
-- 
2.25.1

Re: [PATCH v3 1/4] s390x/css: Introduce an ESW struct

[Cornelia Huck]

On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:

> The Interrupt Response Block is comprised of several other
> structures concatenated together, but only the 12-byte
> Subchannel-Status Word (SCSW) is defined as a proper struct.
> Everything else is a simple array of 32-bit words.
>
> Let's define a proper struct for the 20-byte Extended-Status
> Word (ESW) so that we can make good decisions about the sense
> data that would go into the ECW area for virtual vs
> passthrough devices.
>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/s390x/css.c            | 19 +++++++++++++------
>  include/hw/s390x/ioinst.h | 12 +++++++++++-
>  2 files changed, 24 insertions(+), 7 deletions(-)
>

(...)

> diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> index c6737a30d4..9613e0ccbb 100644
> --- a/include/hw/s390x/ioinst.h
> +++ b/include/hw/s390x/ioinst.h
> @@ -123,10 +123,20 @@ typedef struct SCHIB {
>      uint8_t mda[4];
>  } QEMU_PACKED SCHIB;
>  
> +/* extended-status word */
> +typedef struct ESW {
> +   uint32_t sublog;
> +   uint32_t erw;
> +   uint64_t f_addr;
> +   uint32_t s_addr;
> +} QEMU_PACKED ESW;

Strictly speaking, that's a format 0 esw. Doesn't matter too much in the
end, I think:
- erw is always the same
- f_addr and s_addr are always 0 for the other formats
- 'sublog' is always a u32 with the lpum in the same place (which we
  always set to 0x80 for virtual subchannels, as they have only one path)

> +
> +#define ESW_ERW_SENSE 0x01000000
> +
>  /* interruption response block */
>  typedef struct IRB {
>      SCSW scsw;
> -    uint32_t esw[5];
> +    ESW esw;
>      uint32_t ecw[8];
>      uint32_t emw[8];
>  } IRB;

Re: [PATCH v3 4/4] s390x/css: Add passthrough IRB

[Cornelia Huck]

On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:

> Wire in the subchannel callback for building the IRB
> ESW and ECW space for passthrough devices, and copy
> the hardware's ESW into the IRB we are building.
>
> If the hardware presented concurrent sense, then copy
> that sense data into the IRB's ECW space.
>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/s390x/css.c         | 13 ++++++++++++-
>  hw/s390x/s390-ccw.c    |  1 +
>  hw/vfio/ccw.c          |  4 ++++
>  include/hw/s390x/css.h |  3 +++
>  4 files changed, 20 insertions(+), 1 deletion(-)
>

(...)

> +void build_irb_passthrough(SubchDev *sch, IRB *irb)
> +{
> +    /* Copy ESW from hardware */
> +    irb->esw = sch->esw;
> +
> +    if (irb->esw.erw & ESW_ERW_SENSE) {
> +        /* Copy ECW from hardware */
> +        build_irb_sense_data(sch, irb);
> +    }

I'm wondering whether we should also copy "Model-dependent information"
(scsw 5 + 14 set, erw 7 unset). Seems more correct, and IIUC the guest
was tripped by the presence of erw 7 without valid sense data.

> +}
> +

Re: [PATCH v3 1/4] s390x/css: Introduce an ESW struct

[Eric Farman]

On Wed, 2021-06-16 at 11:46 +0200, Cornelia Huck wrote:
> On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:
> 
> > The Interrupt Response Block is comprised of several other
> > structures concatenated together, but only the 12-byte
> > Subchannel-Status Word (SCSW) is defined as a proper struct.
> > Everything else is a simple array of 32-bit words.
> > 
> > Let's define a proper struct for the 20-byte Extended-Status
> > Word (ESW) so that we can make good decisions about the sense
> > data that would go into the ECW area for virtual vs
> > passthrough devices.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  hw/s390x/css.c            | 19 +++++++++++++------
> >  include/hw/s390x/ioinst.h | 12 +++++++++++-
> >  2 files changed, 24 insertions(+), 7 deletions(-)
> > 
> 
> (...)
> 
> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> > index c6737a30d4..9613e0ccbb 100644
> > --- a/include/hw/s390x/ioinst.h
> > +++ b/include/hw/s390x/ioinst.h
> > @@ -123,10 +123,20 @@ typedef struct SCHIB {
> >      uint8_t mda[4];
> >  } QEMU_PACKED SCHIB;
> >  
> > +/* extended-status word */
> > +typedef struct ESW {
> > +   uint32_t sublog;
> > +   uint32_t erw;
> > +   uint64_t f_addr;
> > +   uint32_t s_addr;
> > +} QEMU_PACKED ESW;
> 
> Strictly speaking, that's a format 0 esw. 

True. I thought I put that in there, but I guess not. I can make a
clarification.

> Doesn't matter too much in the
> end, I think:
> - erw is always the same
> - f_addr and s_addr are always 0 for the other formats
> - 'sublog' is always a u32 with the lpum in the same place (which we
>   always set to 0x80 for virtual subchannels, as they have only one
> path)
> 

Yeah, I didn't want to get into the different formats' word 0. Since
the lpum is common to all, what if I just rename 'sublog' to 'word0'
and add some comments for f_addr and s_addr as only being applicable to
Format 0?

> > +
> > +#define ESW_ERW_SENSE 0x01000000
> > +
> >  /* interruption response block */
> >  typedef struct IRB {
> >      SCSW scsw;
> > -    uint32_t esw[5];
> > +    ESW esw;
> >      uint32_t ecw[8];
> >      uint32_t emw[8];
> >  } IRB;

Re: [PATCH v3 4/4] s390x/css: Add passthrough IRB

[Eric Farman]

On Wed, 2021-06-16 at 11:59 +0200, Cornelia Huck wrote:
> On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:
> 
> > Wire in the subchannel callback for building the IRB
> > ESW and ECW space for passthrough devices, and copy
> > the hardware's ESW into the IRB we are building.
> > 
> > If the hardware presented concurrent sense, then copy
> > that sense data into the IRB's ECW space.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  hw/s390x/css.c         | 13 ++++++++++++-
> >  hw/s390x/s390-ccw.c    |  1 +
> >  hw/vfio/ccw.c          |  4 ++++
> >  include/hw/s390x/css.h |  3 +++
> >  4 files changed, 20 insertions(+), 1 deletion(-)
> > 
> 
> (...)
> 
> > +void build_irb_passthrough(SubchDev *sch, IRB *irb)
> > +{
> > +    /* Copy ESW from hardware */
> > +    irb->esw = sch->esw;
> > +
> > +    if (irb->esw.erw & ESW_ERW_SENSE) {
> > +        /* Copy ECW from hardware */
> > +        build_irb_sense_data(sch, irb);
> > +    }
> 
> I'm wondering whether we should also copy "Model-dependent
> information"
> (scsw 5 + 14 set, erw 7 unset). Seems more correct, and IIUC the
> guest
> was tripped by the presence of erw 7 without valid sense data.
> 

This is true, but that's because the existing code in
css_do_tsch_get_irb() set ERW 7 to go with the zeros it copied into the
ECW. Since we're now copying the ESW.ERW from the passthrough device,
that bit wouldn't be set in the first place.

But, to be more correct with the possibility of model-dependent
information, I can unconditionally copy this data over too.

> > +}
> > +

Re: [PATCH v3 1/4] s390x/css: Introduce an ESW struct

[Cornelia Huck]

On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:

> On Wed, 2021-06-16 at 11:46 +0200, Cornelia Huck wrote:
>> On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:
>> 
>> > The Interrupt Response Block is comprised of several other
>> > structures concatenated together, but only the 12-byte
>> > Subchannel-Status Word (SCSW) is defined as a proper struct.
>> > Everything else is a simple array of 32-bit words.
>> > 
>> > Let's define a proper struct for the 20-byte Extended-Status
>> > Word (ESW) so that we can make good decisions about the sense
>> > data that would go into the ECW area for virtual vs
>> > passthrough devices.
>> > 
>> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> > ---
>> >  hw/s390x/css.c            | 19 +++++++++++++------
>> >  include/hw/s390x/ioinst.h | 12 +++++++++++-
>> >  2 files changed, 24 insertions(+), 7 deletions(-)
>> > 
>> 
>> (...)
>> 
>> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
>> > index c6737a30d4..9613e0ccbb 100644
>> > --- a/include/hw/s390x/ioinst.h
>> > +++ b/include/hw/s390x/ioinst.h
>> > @@ -123,10 +123,20 @@ typedef struct SCHIB {
>> >      uint8_t mda[4];
>> >  } QEMU_PACKED SCHIB;
>> >  
>> > +/* extended-status word */
>> > +typedef struct ESW {
>> > +   uint32_t sublog;
>> > +   uint32_t erw;
>> > +   uint64_t f_addr;
>> > +   uint32_t s_addr;
>> > +} QEMU_PACKED ESW;
>> 
>> Strictly speaking, that's a format 0 esw. 
>
> True. I thought I put that in there, but I guess not. I can make a
> clarification.

A short comment would be fine.

>
>> Doesn't matter too much in the
>> end, I think:
>> - erw is always the same
>> - f_addr and s_addr are always 0 for the other formats
>> - 'sublog' is always a u32 with the lpum in the same place (which we
>>   always set to 0x80 for virtual subchannels, as they have only one
>> path)
>> 
>
> Yeah, I didn't want to get into the different formats' word 0. Since
> the lpum is common to all, what if I just rename 'sublog' to 'word0'
> and add some comments for f_addr and s_addr as only being applicable to
> Format 0?

Works for me.

>
>> > +
>> > +#define ESW_ERW_SENSE 0x01000000
>> > +
>> >  /* interruption response block */
>> >  typedef struct IRB {
>> >      SCSW scsw;
>> > -    uint32_t esw[5];
>> > +    ESW esw;
>> >      uint32_t ecw[8];
>> >      uint32_t emw[8];
>> >  } IRB;

Re: [PATCH v3 4/4] s390x/css: Add passthrough IRB

[Cornelia Huck]

On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:

> On Wed, 2021-06-16 at 11:59 +0200, Cornelia Huck wrote:
>> On Wed, Jun 16 2021, Eric Farman <farman@linux.ibm.com> wrote:
>> 
>> > Wire in the subchannel callback for building the IRB
>> > ESW and ECW space for passthrough devices, and copy
>> > the hardware's ESW into the IRB we are building.
>> > 
>> > If the hardware presented concurrent sense, then copy
>> > that sense data into the IRB's ECW space.
>> > 
>> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> > ---
>> >  hw/s390x/css.c         | 13 ++++++++++++-
>> >  hw/s390x/s390-ccw.c    |  1 +
>> >  hw/vfio/ccw.c          |  4 ++++
>> >  include/hw/s390x/css.h |  3 +++
>> >  4 files changed, 20 insertions(+), 1 deletion(-)
>> > 
>> 
>> (...)
>> 
>> > +void build_irb_passthrough(SubchDev *sch, IRB *irb)
>> > +{
>> > +    /* Copy ESW from hardware */
>> > +    irb->esw = sch->esw;
>> > +
>> > +    if (irb->esw.erw & ESW_ERW_SENSE) {
>> > +        /* Copy ECW from hardware */
>> > +        build_irb_sense_data(sch, irb);
>> > +    }
>> 
>> I'm wondering whether we should also copy "Model-dependent
>> information"
>> (scsw 5 + 14 set, erw 7 unset). Seems more correct, and IIUC the
>> guest
>> was tripped by the presence of erw 7 without valid sense data.
>> 
>
> This is true, but that's because the existing code in
> css_do_tsch_get_irb() set ERW 7 to go with the zeros it copied into the
> ECW. Since we're now copying the ESW.ERW from the passthrough device,
> that bit wouldn't be set in the first place.

That's what I meant to say :)

>
> But, to be more correct with the possibility of model-dependent
> information, I can unconditionally copy this data over too.

Yep. Not that I have any idea what that "Model-dependent information2
would be...

>
>> > +}
>> > +

Re: [PATCH v3 2/4] s390x/css: Split out the IRB sense data

[Thomas Huth]

On 16/06/2021 03.47, Eric Farman wrote:
> Let's move this logic into its own routine,
> so it can be reused later.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   hw/s390x/css.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 8be21efb13..554c9083fd 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1639,6 +1639,17 @@ static void copy_irb_to_guest(IRB *dest, const IRB *src, const PMCW *pmcw,
>       *irb_len = sizeof(*dest);
>   }
>   
> +static void build_irb_sense_data(SubchDev *sch, IRB *irb)
> +{
> +    int i;
> +
> +    /* Attention: sense_data is already BE! */
> +    memcpy(irb->ecw, sch->sense_data, sizeof(sch->sense_data));
> +    for (i = 0; i < ARRAY_SIZE(irb->ecw); i++) {
> +        irb->ecw[i] = be32_to_cpu(irb->ecw[i]);
> +    }
> +}
> +
>   int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
>   {
>       SCHIB *schib = &sch->curr_status;
> @@ -1669,14 +1680,8 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb, int *irb_len)
>           /* If a unit check is pending, copy sense data. */
>           if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
>               (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
> -            int i;
> -
>               irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF | SCSW_FLAGS_MASK_ECTL;
> -            /* Attention: sense_data is already BE! */
> -            memcpy(irb.ecw, sch->sense_data, sizeof(sch->sense_data));
> -            for (i = 0; i < ARRAY_SIZE(irb.ecw); i++) {
> -                irb.ecw[i] = be32_to_cpu(irb.ecw[i]);
> -            }
> +            build_irb_sense_data(sch, &irb);
>               irb.esw.erw = ESW_ERW_SENSE | (sizeof(sch->sense_data) << 8);
>           }
>       }
> 

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