Re: [PATCH 1/1] vfio-ccw: Keep passthrough sense data intact

[Cornelia Huck <cohuck@redhat.com>]

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

> For virtual devices, there is space for sense data to be built
> and later copied into the IRB's ECW space once a TSCH is handled.
>
> For passthrough devices, the IRB is passed up from hardware.
> There might already be sense data in the ECW, in which case it
> would be unusual to overwrite the IRB ESW/ECW data with itself.
>
> In either case, if there isn't sense data, then an explicit SENSE
> operation might be needed from the guest driver. Regardless,
> fabricating sense data out of zeros seems like it would result
> in unexpected behavior.
>
> Let's remove the copying of the ECW/sense data in the vfio-ccw
> driver, and adapt the check in the TSCH handler to look for
> non-zero data in the local sense data before building a sense
> data response in the IRB.
>
> This fixes a "dasdfmt -M quick" command issued within a guest.
>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/s390x/css.c | 3 ++-
>  hw/vfio/ccw.c  | 6 ------
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index bed46f5ec3..29234daa27 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1661,7 +1661,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)) {
> +            (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE) &&
> +            (sch->sense_data[0] != 0)) {
>              int i;
>  
>              irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF | SCSW_FLAGS_MASK_ECTL;
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 139a3d9d1b..a4dc4acb34 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -371,12 +371,6 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
>      copy_scsw_to_guest(&s, &irb.scsw);
>      schib->scsw = s;
>  
> -    /* If a uint check is pending, copy sense data. */
> -    if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
> -        (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {

If I'm reading the PoP correctly, turning on concurrent sense only means
that we may have sense data already available, but not that it's
guaranteed. Would it be enough to look at the relevant bit in the erw
and only copy sense data if it is actually set (here and/or above)?

> -        memcpy(sch->sense_data, irb.ecw, sizeof(irb.ecw));
> -    }
> -
>  read_err:
>      css_inject_io_interrupt(sch);
>  }