On Mon, 2021-06-14 at 13:15 +0200, Cornelia Huck wrote:
> On Fri, Jun 11 2021, Eric Farman <far...@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 <far...@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-far...@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
