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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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