[PATCH 0/1] vfio-ccw: Fix garbage sense data on I/O error

[PATCH 0/1] vfio-ccw: Fix garbage sense data on I/O error

[Eric Farman]

Hi Conny,

Per our offline discussion, here's a fix for the error when a guest
issues "dasdfmt -M quick". It basically reverts commit 334e76850bbb
("vfio/ccw: update sense data if a unit check is pending")
and modifies the check that builds sense data in the TSCH handler.

I opted to NOT disable PMCW.CSENSE, because doing so prevents
vfio-ccw devices from coming online at all (didn't pursue deep
enough to explain why). Turning it off in reaction to a unit
check (in this now-reverted codepath) works, but only because of
the corresponding PMCW.CSENSE check in the TSCH code.

I don't know if anything is needed for the (unaltered) ECW data
that commit b498484ed49a ("s390x/css: sense data endianness")
addressed for the copied sense_data bytes, but figure we can
use this as a starting point. Thoughts?

Eric Farman (1):
  vfio-ccw: Keep passthrough sense data intact

 hw/s390x/css.c | 3 ++-
 hw/vfio/ccw.c  | 6 ------
 2 files changed, 2 insertions(+), 7 deletions(-)

-- 
2.25.1

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

[Eric Farman]

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)) {
-        memcpy(sch->sense_data, irb.ecw, sizeof(irb.ecw));
-    }
-
 read_err:
     css_inject_io_interrupt(sch);
 }
-- 
2.25.1

Re: [PATCH 0/1] vfio-ccw: Fix garbage sense data on I/O error

[Matthew Rosato]

On 6/10/21 4:20 PM, Eric Farman wrote:
> Hi Conny,
> 
> Per our offline discussion, here's a fix for the error when a guest
> issues "dasdfmt -M quick". It basically reverts commit 334e76850bbb
> ("vfio/ccw: update sense data if a unit check is pending")
> and modifies the check that builds sense data in the TSCH handler.

Should it includes a Fixes: tag then?

> 
> I opted to NOT disable PMCW.CSENSE, because doing so prevents
> vfio-ccw devices from coming online at all (didn't pursue deep
> enough to explain why). Turning it off in reaction to a unit
> check (in this now-reverted codepath) works, but only because of
> the corresponding PMCW.CSENSE check in the TSCH code.
> 
> I don't know if anything is needed for the (unaltered) ECW data
> that commit b498484ed49a ("s390x/css: sense data endianness")
> addressed for the copied sense_data bytes, but figure we can
> use this as a starting point. Thoughts?
> 
> Eric Farman (1):
>    vfio-ccw: Keep passthrough sense data intact
> 
>   hw/s390x/css.c | 3 ++-
>   hw/vfio/ccw.c  | 6 ------
>   2 files changed, 2 insertions(+), 7 deletions(-)
> 

Re: [PATCH 0/1] vfio-ccw: Fix garbage sense data on I/O error

[Eric Farman]

On Thu, 2021-06-10 at 16:25 -0400, Matthew Rosato wrote:
> On 6/10/21 4:20 PM, Eric Farman wrote:
> > Hi Conny,
> > 
> > Per our offline discussion, here's a fix for the error when a guest
> > issues "dasdfmt -M quick". It basically reverts commit 334e76850bbb
> > ("vfio/ccw: update sense data if a unit check is pending")
> > and modifies the check that builds sense data in the TSCH handler.
> 
> Should it includes a Fixes: tag then?

Yeah, probably. I'll fix it locally so it's prepped for a v2.

> 
> > I opted to NOT disable PMCW.CSENSE, because doing so prevents
> > vfio-ccw devices from coming online at all (didn't pursue deep
> > enough to explain why). Turning it off in reaction to a unit
> > check (in this now-reverted codepath) works, but only because of
> > the corresponding PMCW.CSENSE check in the TSCH code.
> > 
> > I don't know if anything is needed for the (unaltered) ECW data
> > that commit b498484ed49a ("s390x/css: sense data endianness")
> > addressed for the copied sense_data bytes, but figure we can
> > use this as a starting point. Thoughts?
> > 
> > Eric Farman (1):
> >    vfio-ccw: Keep passthrough sense data intact
> > 
> >   hw/s390x/css.c | 3 ++-
> >   hw/vfio/ccw.c  | 6 ------
> >   2 files changed, 2 insertions(+), 7 deletions(-)
> > 

Re: [PATCH 0/1] vfio-ccw: Fix garbage sense data on I/O error

[Cornelia Huck]

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

> Hi Conny,
>
> Per our offline discussion, here's a fix for the error when a guest
> issues "dasdfmt -M quick". It basically reverts commit 334e76850bbb
> ("vfio/ccw: update sense data if a unit check is pending")
> and modifies the check that builds sense data in the TSCH handler.
>
> I opted to NOT disable PMCW.CSENSE, because doing so prevents
> vfio-ccw devices from coming online at all (didn't pursue deep
> enough to explain why). Turning it off in reaction to a unit
> check (in this now-reverted codepath) works, but only because of
> the corresponding PMCW.CSENSE check in the TSCH code.

It's a bit puzzling why fencing it off in msch doesn't work; but maybe
it would not be the right thing to do anyway, if we can support some
concurrent sense operations. I'm still reading the PoP.

>
> I don't know if anything is needed for the (unaltered) ECW data
> that commit b498484ed49a ("s390x/css: sense data endianness")
> addressed for the copied sense_data bytes, but figure we can
> use this as a starting point. Thoughts?
>
> Eric Farman (1):
>   vfio-ccw: Keep passthrough sense data intact
>
>  hw/s390x/css.c | 3 ++-
>  hw/vfio/ccw.c  | 6 ------
>  2 files changed, 2 insertions(+), 7 deletions(-)
>

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

[Cornelia Huck]

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);
>  }

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

[Cornelia Huck]

On Fri, Jun 11 2021, Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, Jun 10 2021, Eric Farman <farman@linux.ibm.com> wrote:
>> 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;

This function is where we build the esw/ecw...

>> 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)) {

...and here we actually do have the esw/ecw provided by the hardware.

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

Maybe the root of the problem is that we actually try to build the esw
ourselves? If we copy it from the irb received by the hardware, we
should already have the correct data, I think.

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

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

[Eric Farman]

On Fri, 2021-06-11 at 12:21 +0200, Cornelia Huck wrote:
> On Fri, Jun 11 2021, Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Thu, Jun 10 2021, Eric Farman <farman@linux.ibm.com> wrote:
> > > 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;
> 
> This function is where we build the esw/ecw...
> 
> > > 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)) {
> 
> ...and here we actually do have the esw/ecw provided by the hardware.
> 
> > 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.

Agreed.

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

Do we have the hardware ERW in the css_do_tsch routine?

Oh, but we have SCSW, and POPS says if ERW.S is set, SCSW.E is set. So
that would make this a pretty simple change then.

> 
> Maybe the root of the problem is that we actually try to build the
> esw
> ourselves? If we copy it from the irb received by the hardware, we
> should already have the correct data, I think.

Yeah, that's part of the problem. As you note above, the PMCW.CSENSE
bit only says if concurrent sense is possible, not that it was actually
stored in the IRB.

I (mistakenly) thought that removing this hunk would get the whole IRB
copied over, but I see now that css_do_tsch_get_irb() only copies the
SCSW, and builds the ESW/ECW based off sch->sense_data.

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

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

[Cornelia Huck]

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

> On Fri, 2021-06-11 at 12:21 +0200, Cornelia Huck wrote:
>> On Fri, Jun 11 2021, Cornelia Huck <cohuck@redhat.com> wrote:
>> 
>> > On Thu, Jun 10 2021, Eric Farman <farman@linux.ibm.com> wrote:
>> > > 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;
>> 
>> This function is where we build the esw/ecw...
>> 
>> > > 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)) {
>> 
>> ...and here we actually do have the esw/ecw provided by the hardware.
>> 
>> > 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.
>
> Agreed.
>
>> >  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)?
>
> Do we have the hardware ERW in the css_do_tsch routine?
>
> Oh, but we have SCSW, and POPS says if ERW.S is set, SCSW.E is set. So
> that would make this a pretty simple change then.

Nod, that looks good.

>
>> 
>> Maybe the root of the problem is that we actually try to build the
>> esw
>> ourselves? If we copy it from the irb received by the hardware, we
>> should already have the correct data, I think.
>
> Yeah, that's part of the problem. As you note above, the PMCW.CSENSE
> bit only says if concurrent sense is possible, not that it was actually
> stored in the IRB.
>
> I (mistakenly) thought that removing this hunk would get the whole IRB
> copied over, but I see now that css_do_tsch_get_irb() only copies the
> SCSW, and builds the ESW/ECW based off sch->sense_data.

Might be a good idea to go over what we pass through vs. what we
emulate for vfio-ccw devices, in case we have more conditions like
this. We probably should not overwrite information that we can just move
guestward.

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