| Message ID | 20210610202011.391029-2-farman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio-ccw: Fix garbage sense data on I/O error | expand |
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); > }
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); >> }
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); > > > }
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); >> > > }
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); }
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(-)