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

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;

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;

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;

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;

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;

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.

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