[v3] s390x: Fix IRB sense data

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

Posted by Eric Farman 4 years, 7 months ago

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

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

Posted by Cornelia Huck 4 years, 7 months ago

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

Posted by Eric Farman 4 years, 7 months ago

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

Posted by Cornelia Huck 4 years, 7 months ago

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;