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

From: Eric Farman <farman@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>,
	qemu-devel@nongnu.org, qemu-s390x@nongnu.org
Cc: Thomas Huth <thuth@redhat.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	David Hildenbrand <david@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Halil Pasic <pasic@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>
Subject: Re: [PATCH v2] s390x/css: Selectively copy sense data to IRB
Date: Mon, 14 Jun 2021 09:03:02 -0400	[thread overview]
Message-ID: <393c292444af5a43f8e020de411dd09842c7a9ca.camel@linux.ibm.com> (raw)
In-Reply-To: <87sg1ky9x8.fsf@redhat.com>

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