Re: [PATCH v2] s390x/css: Selectively copy sense data to IRB

[Cornelia Huck]

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

> The SCHIB.PMCW.CSENSE bit is used to determine whether the
> IRB should be set up with sense data, but that bit only
> indicates whether sense data is requested, not if it was
> provided by the device. For virtual devices, this is fine.
>
> For passthrough devices, hardware would present sense data
> in IRB.ECW, but that field is only valid if IRB.SCSW.E and
> IRB.ERW.S were also set.

An important point is that IRB.ERW.S implies IRB.SCSW.E, I guess?

If I parse the table regarding the ecw in the POP correctly, we might
also have model-dependent information stored, in which case we would
need to indicate zero sense data in the erw that we build.

>
> Let's only build the sense data in the IRB if the first byte
> of sense is nonzero (indicating it may have come from a virtual
> device), or the IRB.SCSW.E bit is already set (indicating it
> came from the hardware). That way, the guest driver can read
> the sense data if valid, or respond with a Sense CCW to get
> the sense data if it wants/needs.

Hm, would it be possible that we get junk instead of proper sense data
from the hardware, if IRB.ERW.S is not set? E.g. some residual
data. That would potentially trigger the first condition.

Maybe we really need to special case virtual vs. passthrough devices
here. We can assume that a virtual device with a unit check always has
proper sense data available. For passthrough devices, maybe we need to
copy esw etc. from the irb we got hardware, and not try to construct it
ourselves?

>
> Fixes: df1fe5bb4924 ("s390: Virtual channel subsystem support.")
> Fixes: 334e76850bbb ("vfio/ccw: update sense data if a unit check is pending")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> Notes:
>     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
>     
>     v1: 
> 20210610202011.391029-1-farman@linux.ibm.com/">https://lore.kernel.org/qemu-devel/20210610202011.391029-1-farman@linux.ibm.com/
>
>  hw/s390x/css.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index bed46f5ec3..8935f948d5 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1659,9 +1659,15 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB 
> *target_irb, int *irb_len)
>          } else {
>              irb.esw[0] = 0x00800000;
>          }
> -        /* If a unit check is pending, copy sense data. */
> +        /*
> +         * If a unit check is pending and concurrent sense
> +         * is requested, copy the sense data if the sense
> +         * data is plausibly valid.
> +         */
>          if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
> -            (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
> +            (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE) &&
> +            ((schib->scsw.flags & SCSW_FLAGS_MASK_ECTL) ||
> +             (sch->sense_data[0] != 0))) {
>              int i;
>  
>              irb.scsw.flags |= SCSW_FLAGS_MASK_ESWF | SCSW_FLAGS_MASK_ECTL;
> -- 
> 2.25.1