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

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

[Eric Farman]

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.

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.

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

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

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

[Eric Farman]

On Mon, 2021-06-14 at 13:15 +0200, Cornelia Huck wrote:
> 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?

Correct. I can make this point more explicit so we don't have to
reference the POPS to remind ourselves.

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

I wrestled with this. The best answer would be to pass the ERW from
hardware via vfio-ccw back to QEMU, since that contains the "is this
sense data or is this model-dependent info" bit (whatever that latter
might be). ...

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

... Ah, I should read ahead. :)

Yeah, that was where I was going to go above. I was considering that
this would be a simpler solution near term, to get "normal" behavior
behaving properly, but fixing it all in one go is probably better. Will
see how bad a v3 cook starts looking.

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