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

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

[Eric Farman]

Conny, et al,

Here is a quick update to the series for fixing passthrough
sense data in the irb, using a subchannel-specific callback.

As before, the first three patches are code refactoring.
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.
Since the callback isn't invoked for passthrough subchannels
the SCSW and ERW bits don't get set indicating sense data
is present, even though the sense data itself is still zero.

Patch 4 implements that for vfio-ccw.

v3->v4:
 - [CH] Rename ESW.sublog to ESW.word0
 - [CH] Add comment that ESW.f_addr and .s_addr are only Fmt0 ESW
 - [CH] Always copy ECW data into IRB to include mysterious
        "model-dependent information" that could exist there
 - [TH] Added r-b to patch 2 (thank you!!)

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

v3: https://lore.kernel.org/qemu-devel/20210616014749.2460133-1-farman@linux.ibm.com/
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            | 87 ++++++++++++++++++++++++++++-----------
 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, 86 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v4 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..59d935570e 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->word0 = cpu_to_be32(src->word0);
+    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.word0 = 0x04804000;
         } else {
-            irb.esw[0] = 0x00800000;
+            irb.esw.word0 = 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..e7ab401781 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;
 
+/* format-0 extended-status word */
+typedef struct ESW {
+   uint32_t word0;
+   uint32_t erw;
+   uint64_t f_addr;     /* Zeros for other ESW formats */
+   uint32_t s_addr;     /* Zeros for other ESW formats */
+} 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 v4 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>
Reviewed-by: Thomas Huth <thuth@redhat.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 59d935570e..a54e384566 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 v4 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 a54e384566..736715e100 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.word0 = 0x04804000;
+        } else {
+            irb->esw.word0 = 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.word0 = 0x04804000;
-        } else {
-            irb.esw.word0 = 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 v4 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         | 16 +++++++++++++++-
 hw/s390x/s390-ccw.c    |  1 +
 hw/vfio/ccw.c          |  4 ++++
 include/hw/s390x/css.h |  3 +++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 736715e100..c1e4f65f82 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->word0 = cpu_to_be32(src->word0);
     dest->erw = cpu_to_be32(src->erw);
@@ -1650,6 +1650,20 @@ 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) is true, then the contents
+     * of the ECW is sense data. If false, then it is model-dependent
+     * information. Either way, copy it into the IRB for the guest to
+     * read/decide what to do with.
+     */
+    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 v4 1/4] s390x/css: Introduce an ESW struct

[Cornelia Huck]

On Fri, Jun 18 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..e7ab401781 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;
>  
> +/* format-0 extended-status word */
> +typedef struct ESW {
> +   uint32_t word0;

Maybe append /* subchannel logout for format 0 */? Can do when applying.

> +   uint32_t erw;
> +   uint64_t f_addr;     /* Zeros for other ESW formats */
> +   uint32_t s_addr;     /* Zeros for other ESW formats */
> +} 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;

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

[Cornelia Huck]

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

> Conny, et al,
>
> Here is a quick update to the series for fixing passthrough
> sense data in the irb, using a subchannel-specific callback.
>
> As before, the first three patches are code refactoring.
> 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.
> Since the callback isn't invoked for passthrough subchannels
> the SCSW and ERW bits don't get set indicating sense data
> is present, even though the sense data itself is still zero.
>
> Patch 4 implements that for vfio-ccw.
>

LGTM. I'll take it for a spin and probably queue it.

> v3->v4:
>  - [CH] Rename ESW.sublog to ESW.word0
>  - [CH] Add comment that ESW.f_addr and .s_addr are only Fmt0 ESW
>  - [CH] Always copy ECW data into IRB to include mysterious
>         "model-dependent information" that could exist there
>  - [TH] Added r-b to patch 2 (thank you!!)
>
> 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
>
> v3: https://lore.kernel.org/qemu-devel/20210616014749.2460133-1-farman@linux.ibm.com/
> 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            | 87 ++++++++++++++++++++++++++++-----------
>  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, 86 insertions(+), 25 deletions(-)

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

[Cornelia Huck]

On Fri, Jun 18 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..e7ab401781 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;
>  
> +/* format-0 extended-status word */
> +typedef struct ESW {
> +   uint32_t word0;
> +   uint32_t erw;
> +   uint64_t f_addr;     /* Zeros for other ESW formats */
> +   uint32_t s_addr;     /* Zeros for other ESW formats */
> +} QEMU_PACKED ESW;

Eww, this fails with mingw:
https://gitlab.com/cohuck/qemu/-/jobs/1358335494

i686-w64-mingw32-gcc -Ilibcommon.fa.p -I../slirp -I../slirp/src -I../dtc/libfdt -I../capstone/include/capstone -I. -Iqapi -Itrace -Iui -Iui/shader -I/usr/i686-w64-mingw32/sys-root/mingw/include -I/usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0 -I/usr/i686-w64-mingw32/sys-root/mingw/lib/glib-2.0/include -I/usr/i686-w64-mingw32/sys-root/mingw/include/gtk-3.0 -I/usr/i686-w64-mingw32/sys-root/mingw/include/cairo -I/usr/i686-w64-mingw32/sys-root/mingw/include/pango-1.0 -I/usr/i686-w64-mingw32/sys-root/mingw/include/fribidi -I/usr/i686-w64-mingw32/sys-root/mingw/include/harfbuzz -I/usr/i686-w64-mingw32/sys-root/mingw/include/atk-1.0 -I/usr/i686-w64-mingw32/sys-root/mingw/include/pixman-1 -I/usr/i686-w64-mingw32/sys-root/mingw/include/freetype2 -I/usr/i686-w64-mingw32/sys-root/mingw/include/libpng16 -I/usr/i686-w64-mingw32/sys-root/mingw/include/gdk-pixbuf-2.0 -I/usr/i686-w64-mingw32/sys-root/mingw/lib/libffi-3.1/include -I/usr/i686-w64-mingw32/sys-root/mingw/include/p11-kit-1 -I/usr/i686-w64-mingw32/sys-root/mingw/include/SDL2 -fdiagnostics-color=auto -pipe -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g -iquote . -iquote /builds/cohuck/qemu -iquote /builds/cohuck/qemu/include -iquote /builds/cohuck/qemu/disas/libvixl -iquote /builds/cohuck/qemu/tcg/i386 -mms-bitfields -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -m32 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -Dmain=SDL_main -Wno-undef -mms-bitfields -mms-bitfields -mms-bitfields -MD -MQ libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj -MF libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj.d -o libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj -c ../hw/s390x/virtio-ccw-gpu.c
In file included from /usr/i686-w64-mingw32/sys-root/mingw/include/winsock2.h:54,
                 from /builds/cohuck/qemu/include/sysemu/os-win32.h:29,
                 from /builds/cohuck/qemu/include/qemu/osdep.h:135,
                 from ../hw/s390x/virtio-ccw-gpu.c:11:
/builds/cohuck/qemu/include/hw/s390x/ioinst.h:131:13: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
  131 |    uint32_t s_addr;     /* Zeros for other ESW formats */
      |             ^~~~~~
In file included from /builds/cohuck/qemu/include/qemu/osdep.h:37,
                 from ../hw/s390x/virtio-ccw-gpu.c:11:
/builds/cohuck/qemu/include/qemu/compiler.h:80:36: error: static assertion failed: "size of IRB is wrong"
   80 | #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
      |                                    ^~~~~~~~~~~~~~
/builds/cohuck/qemu/include/hw/s390x/ioinst.h:143:1: note: in expansion of macro 'QEMU_BUILD_BUG_MSG'
  143 | QEMU_BUILD_BUG_MSG(sizeof(IRB) != 96, "size of IRB is wrong");
      | ^~~~~~~~~~~~~~~~~~

> +
> +#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 v4 1/4] s390x/css: Introduce an ESW struct

[Cornelia Huck]

On Fri, Jun 18 2021, Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, Jun 18 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..e7ab401781 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;
>>  
>> +/* format-0 extended-status word */
>> +typedef struct ESW {
>> +   uint32_t word0;
>> +   uint32_t erw;
>> +   uint64_t f_addr;     /* Zeros for other ESW formats */
>> +   uint32_t s_addr;     /* Zeros for other ESW formats */
>> +} QEMU_PACKED ESW;
>
> Eww, this fails with mingw:
> https://gitlab.com/cohuck/qemu/-/jobs/1358335494
>
> i686-w64-mingw32-gcc -Ilibcommon.fa.p -I../slirp -I../slirp/src -I../dtc/libfdt -I../capstone/include/capstone -I. -Iqapi -Itrace -Iui -Iui/shader -I/usr/i686-w64-mingw32/sys-root/mingw/include -I/usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0 -I/usr/i686-w64-mingw32/sys-root/mingw/lib/glib-2.0/include -I/usr/i686-w64-mingw32/sys-root/mingw/include/gtk-3.0 -I/usr/i686-w64-mingw32/sys-root/mingw/include/cairo -I/usr/i686-w64-mingw32/sys-root/mingw/include/pango-1.0 -I/usr/i686-w64-mingw32/sys-root/mingw/include/fribidi -I/usr/i686-w64-mingw32/sys-root/mingw/include/harfbuzz -I/usr/i686-w64-mingw32/sys-root/mingw/include/atk-1.0 -I/usr/i686-w64-mingw32/sys-root/mingw/include/pixman-1 -I/usr/i686-w64-mingw32/sys-root/mingw/include/freetype2 -I/usr/i686-w64-mingw32/sys-root/mingw/include/libpng16 -I/usr/i686-w64-mingw32/sys-root/mingw/include/gdk-pixbuf-2.0 -I/usr/i686-w64-mingw32/sys-root/mingw/lib/libffi-3.1/include -I/usr/i686-w64-mingw32/sys-root/mingw/include/p11-kit-1 -I/usr/i686-w64-mingw32/sys-root/mingw/include/SDL2 -fdiagnostics-color=auto -pipe -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g -iquote . -iquote /builds/cohuck/qemu -iquote /builds/cohuck/qemu/include -iquote /builds/cohuck/qemu/disas/libvixl -iquote /builds/cohuck/qemu/tcg/i386 -mms-bitfields -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -m32 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -Dmain=SDL_main -Wno-undef -mms-bitfields -mms-bitfields -mms-bitfields -MD -MQ libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj -MF libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj.d -o libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj -c ../hw/s390x/virtio-ccw-gpu.c
> In file included from /usr/i686-w64-mingw32/sys-root/mingw/include/winsock2.h:54,
>                  from /builds/cohuck/qemu/include/sysemu/os-win32.h:29,
>                  from /builds/cohuck/qemu/include/qemu/osdep.h:135,
>                  from ../hw/s390x/virtio-ccw-gpu.c:11:
> /builds/cohuck/qemu/include/hw/s390x/ioinst.h:131:13: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
>   131 |    uint32_t s_addr;     /* Zeros for other ESW formats */
>       |             ^~~~~~

It seems to be the name that is tripping it; if I rename it to sec_addr
and the preceding field to fail_addr, the build passes.

Anyone know why that is? And if renaming is unavoidable, are fail_addr
and sec_addr ok, or can we find better names?

> In file included from /builds/cohuck/qemu/include/qemu/osdep.h:37,
>                  from ../hw/s390x/virtio-ccw-gpu.c:11:
> /builds/cohuck/qemu/include/qemu/compiler.h:80:36: error: static assertion failed: "size of IRB is wrong"
>    80 | #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
>       |                                    ^~~~~~~~~~~~~~
> /builds/cohuck/qemu/include/hw/s390x/ioinst.h:143:1: note: in expansion of macro 'QEMU_BUILD_BUG_MSG'
>   143 | QEMU_BUILD_BUG_MSG(sizeof(IRB) != 96, "size of IRB is wrong");
>       | ^~~~~~~~~~~~~~~~~~
>

These were just follow-on errors.

>> +
>> +#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 v4 1/4] s390x/css: Introduce an ESW struct

[Eric Farman]

On Fri, 2021-06-18 at 14:46 +0200, Cornelia Huck wrote:
> On Fri, Jun 18 2021, Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, Jun 18 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..e7ab401781 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;
> > >  
> > > +/* format-0 extended-status word */
> > > +typedef struct ESW {
> > > +   uint32_t word0;
> > > +   uint32_t erw;
> > > +   uint64_t f_addr;     /* Zeros for other ESW formats */
> > > +   uint32_t s_addr;     /* Zeros for other ESW formats */
> > > +} QEMU_PACKED ESW;
> > 
> > Eww, this fails with mingw:
> > https://gitlab.com/cohuck/qemu/-/jobs/1358335494
> > 
> > i686-w64-mingw32-gcc -Ilibcommon.fa.p -I../slirp -I../slirp/src
> > -I../dtc/libfdt -I../capstone/include/capstone -I. -Iqapi -Itrace
> > -Iui -Iui/shader -I/usr/i686-w64-mingw32/sys-root/mingw/include
> > -I/usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0
> > -I/usr/i686-w64-mingw32/sys-root/mingw/lib/glib-2.0/include
> > -I/usr/i686-w64-mingw32/sys-root/mingw/include/gtk-3.0 -I/usr/i686-
> > w64-mingw32/sys-root/mingw/include/cairo -I/usr/i686-w64-
> > mingw32/sys-root/mingw/include/pango-1.0 -I/usr/i686-w64-
> > mingw32/sys-root/mingw/include/fribidi -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/harfbuzz -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/atk-1.0 -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/pixman-1 -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/freetype2 -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/libpng16 -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/gdk-pixbuf-2.0 -I/usr/i686-w64-mingw32/sys-
> > root/mingw/lib/libffi-3.1/include -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/p11-kit-1 -I/usr/i686-w64-mingw32/sys-
> > root/mingw/include/SDL2 -fdiagnostics-color=auto -pipe -Wall
> > -Winvalid-pch -Werror -std=gnu99 -O2 -g -iquote . -iquote
> > /builds/cohuck/qemu -iquote /builds/cohuck/qemu/include -iquote
> > /builds/cohuck/qemu/disas/libvixl -iquote
> > /builds/cohuck/qemu/tcg/i386 -mms-bitfields -U_FORTIFY_SOURCE
> > -D_FORTIFY_SOURCE=2 -m32 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
> > -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef
> > -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-
> > common -fwrapv -Wold-style-declaration -Wold-style-definition
> > -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-
> > qualifiers -Wempty-body -Wnested-externs -Wendif-labels
> > -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-
> > include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-
> > protector-strong -Dmain=SDL_main -Wno-undef -mms-bitfields -mms-
> > bitfields -mms-bitfields -MD -MQ libcommon.fa.p/hw_s390x_virtio-
> > ccw-gpu.c.obj -MF libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj.d -o
> > libcommon.fa.p/hw_s390x_virtio-ccw-gpu.c.obj -c ../hw/s390x/virtio-
> > ccw-gpu.c
> > In file included from /usr/i686-w64-mingw32/sys-
> > root/mingw/include/winsock2.h:54,
> >                  from /builds/cohuck/qemu/include/sysemu/os-
> > win32.h:29,
> >                  from /builds/cohuck/qemu/include/qemu/osdep.h:135,
> >                  from ../hw/s390x/virtio-ccw-gpu.c:11:
> > /builds/cohuck/qemu/include/hw/s390x/ioinst.h:131:13: error:
> > expected ':', ',', ';', '}' or '__attribute__' before '.' token
> >   131 |    uint32_t s_addr;     /* Zeros for other ESW formats */
> >       |             ^~~~~~
> 
> It seems to be the name that is tripping it; if I rename it to
> sec_addr
> and the preceding field to fail_addr, the build passes.

I was just wondering if it might have been the underscore directly, not
that it was a single letter before the underscore. Weird.

> 
> Anyone know why that is? And if renaming is unavoidable, are
> fail_addr
> and sec_addr ok, or can we find better names?

Since they're zero for Format-!0 ESWs, and regardless we don't fill
them in anyway, could we just make them wordN and change the comment to
give the descriptive name?

 /* format-0 extended-status word */
 typedef struct ESW {
-   uint32_t
word0;
+   uint32_t word0;      /* subchannel logout for format 0 */
    
uint32_t erw;
-   uint64_t f_addr;     /* Zeros for other ESW formats */
-   uint32_t s_addr;     /* Zeros for other ESW formats */
+   uint64_t
word2;      /* failing-storage address for format 0 */
+   uint32_t
word4;      /* scondary-CCW address for format 0 */
 } QEMU_PACKED ESW;

> 
> > In file included from /builds/cohuck/qemu/include/qemu/osdep.h:37,
> >                  from ../hw/s390x/virtio-ccw-gpu.c:11:
> > /builds/cohuck/qemu/include/qemu/compiler.h:80:36: error: static
> > assertion failed: "size of IRB is wrong"
> >    80 | #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x),
> > msg)
> >       |                                    ^~~~~~~~~~~~~~
> > /builds/cohuck/qemu/include/hw/s390x/ioinst.h:143:1: note: in
> > expansion of macro 'QEMU_BUILD_BUG_MSG'
> >   143 | QEMU_BUILD_BUG_MSG(sizeof(IRB) != 96, "size of IRB is
> > wrong");
> >       | ^~~~~~~~~~~~~~~~~~
> > 
> 
> These were just follow-on errors.
> 
> > > +
> > > +#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 v4 1/4] s390x/css: Introduce an ESW struct

[Eric Farman]

On Fri, 2021-06-18 at 11:38 +0200, Cornelia Huck wrote:
> On Fri, Jun 18 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..e7ab401781 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;
> >  
> > +/* format-0 extended-status word */
> > +typedef struct ESW {
> > +   uint32_t word0;
> 
> Maybe append /* subchannel logout for format 0 */? Can do when
> applying.
> 

That's a good idea.

Eric

> > +   uint32_t erw;
> > +   uint64_t f_addr;     /* Zeros for other ESW formats */
> > +   uint32_t s_addr;     /* Zeros for other ESW formats */
> > +} 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;

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

[Cornelia Huck]

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

> On Fri, 2021-06-18 at 14:46 +0200, Cornelia Huck wrote:
>> On Fri, Jun 18 2021, Cornelia Huck <cohuck@redhat.com> wrote:
>> 
>> > On Fri, Jun 18 2021, Eric Farman <farman@linux.ibm.com> wrote:
>> > > +/* format-0 extended-status word */
>> > > +typedef struct ESW {
>> > > +   uint32_t word0;
>> > > +   uint32_t erw;
>> > > +   uint64_t f_addr;     /* Zeros for other ESW formats */
>> > > +   uint32_t s_addr;     /* Zeros for other ESW formats */
>> > > +} QEMU_PACKED ESW;
>> > 
>> > Eww, this fails with mingw:
>> > https://gitlab.com/cohuck/qemu/-/jobs/1358335494
>> > In file included from /usr/i686-w64-mingw32/sys-
>> > root/mingw/include/winsock2.h:54,
>> >                  from /builds/cohuck/qemu/include/sysemu/os-
>> > win32.h:29,
>> >                  from /builds/cohuck/qemu/include/qemu/osdep.h:135,
>> >                  from ../hw/s390x/virtio-ccw-gpu.c:11:
>> > /builds/cohuck/qemu/include/hw/s390x/ioinst.h:131:13: error:
>> > expected ':', ',', ';', '}' or '__attribute__' before '.' token
>> >   131 |    uint32_t s_addr;     /* Zeros for other ESW formats */
>> >       |             ^~~~~~
>> 
>> It seems to be the name that is tripping it; if I rename it to
>> sec_addr
>> and the preceding field to fail_addr, the build passes.
>
> I was just wondering if it might have been the underscore directly, not
> that it was a single letter before the underscore. Weird.
>
>> 
>> Anyone know why that is? And if renaming is unavoidable, are
>> fail_addr
>> and sec_addr ok, or can we find better names?
>
> Since they're zero for Format-!0 ESWs, and regardless we don't fill
> them in anyway, could we just make them wordN and change the comment to
> give the descriptive name?
>
>  /* format-0 extended-status word */
>  typedef struct ESW {
> -   uint32_t
> word0;
> +   uint32_t word0;      /* subchannel logout for format 0 */
>     
> uint32_t erw;
> -   uint64_t f_addr;     /* Zeros for other ESW formats */
> -   uint32_t s_addr;     /* Zeros for other ESW formats */
> +   uint64_t
> word2;      /* failing-storage address for format 0 */
> +   uint32_t
> word4;      /* scondary-CCW address for format 0 */
>  } QEMU_PACKED ESW;
>

Yeah, that looks even better.

I can change that myself, will push out to test.

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

[Cornelia Huck]

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

> Conny, et al,
>
> Here is a quick update to the series for fixing passthrough
> sense data in the irb, using a subchannel-specific callback.
>
> As before, the first three patches are code refactoring.
> 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.
> Since the callback isn't invoked for passthrough subchannels
> the SCSW and ERW bits don't get set indicating sense data
> is present, even though the sense data itself is still zero.
>
> Patch 4 implements that for vfio-ccw.

Thanks, applied (with the discussed modifications in patch 1).

[s390-next is getting full; I plan to send a pull req on Monday, as it
is a bit late in the day for me already.]

>
> v3->v4:
>  - [CH] Rename ESW.sublog to ESW.word0
>  - [CH] Add comment that ESW.f_addr and .s_addr are only Fmt0 ESW
>  - [CH] Always copy ECW data into IRB to include mysterious
>         "model-dependent information" that could exist there
>  - [TH] Added r-b to patch 2 (thank you!!)
>
> 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
>
> v3: https://lore.kernel.org/qemu-devel/20210616014749.2460133-1-farman@linux.ibm.com/
> 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            | 87 ++++++++++++++++++++++++++++-----------
>  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, 86 insertions(+), 25 deletions(-)

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

[Philippe Mathieu-Daudé]

Hi Eric,

On 6/18/21 1:25 AM, Eric Farman 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/hw/s390x/css.c b/hw/s390x/css.c
> index bed46f5ec3..59d935570e 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->word0 = cpu_to_be32(src->word0);
> +    dest->erw = cpu_to_be32(src->erw);
> +    dest->f_addr = cpu_to_be64(src->f_addr);

FYI you fixed an endianess bug, thanks.

Conny told me it was not a problem because f_addr was not used.

> +    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.word0 = 0x04804000;
>          } else {
> -            irb.esw[0] = 0x00800000;
> +            irb.esw.word0 = 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..e7ab401781 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;
>  
> +/* format-0 extended-status word */
> +typedef struct ESW {
> +   uint32_t word0;
> +   uint32_t erw;
> +   uint64_t f_addr;     /* Zeros for other ESW formats */
> +   uint32_t s_addr;     /* Zeros for other ESW formats */
> +} 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;
>