On 10/28/22 4:22 PM, Eric Farman wrote: > On Thu, 2022-10-27 at 23:23 +0200, Peter Jin wrote: >> Revert the control and flag bits in the subchannel status word in >> case >> the SSCH operation fails with non-zero CC (ditto for CSCH and HSCH). >> According to POPS, the control and flag bits are only changed if >> SSCH, >> CSCH, and HSCH return CC 0, and no other action should be taken >> otherwise. >> In order to simulate that after the fact, the bits need to be >> reverted on >> non-zero CC. >> > > I'm okay to this point... > >> This change is necessary due to the fact that the pwrite() in vfio- >> ccw >> which triggers the SSCH can fail at any time. Previously, there was >> only virtio-ccw, whose do_subchannel_work function was only able to >> return CC0. However, once vfio-ccw went into the mix, it has become >> necessary to handle errors in code paths that were previously assumed >> to always return success. >> >> In our case, we found that in case of pwrite() failure (which was >> discovered by strace injection), the subchannel could be stuck in >> start >> pending state, which could be problematic if the pwrite() call >> returns >> CC2. Experimentation shows that the guest tries to retry the SSCH >> call as >> normal for CC2, but it actually continously fails due to the fact >> that >> the subchannel is stuck in start pending state even though no start >> function is actually taking place. > > ...but the two paragraphs above are a bit cumbersome to digest. Maybe > it's just too late in the week for me. What about something like this? > > """ > While the do_subchannel_work logic for virtual (virtio) devices will > return condition code 0, passthrough (vfio) devices may encounter > errors from either the host kernel or real hardware that need to be > accounted for after this point. This includes restoring the state of > the Subchannel Status Word to reflect the subchannel, as these bits > would not be set in the event of a non-zero condition code from the > affected instructions. > > Experimentation has shown that a failure on a START SUBCHANNEL (SSCH) > to a passthrough device would leave the subchannel with the START > PENDING activity control bit set, thus blocking subsequent SSCH > operations in css_do_ssch() until some form of error recovery was > undertaken since no interrupt would be expected. > """
+1 to this re-write > >> >> Signed-off-by: Peter Jin <p...@linux.ibm.com> > > We've talked previously about clearing this within the > do_subchannel_work_passthrough routine in order to keep the _virtual > paths untouched, but this seems like a reasonable approach to me. > > The commit message is probably fine either way, but as far as the code > goes: > > Reviewed-by: Eric Farman <far...@linux.ibm.com> Reviewed-by: Matthew Rosato <mjros...@linux.ibm.com> Thanks Peter! > >> --- >> hw/s390x/css.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++- >> -- >> 1 file changed, 48 insertions(+), 3 deletions(-) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 7d9523f811..95d1b3a3ce 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -1522,21 +1522,37 @@ IOInstEnding css_do_xsch(SubchDev *sch) >> IOInstEnding css_do_csch(SubchDev *sch) >> { >> SCHIB *schib = &sch->curr_status; >> + uint16_t old_scsw_ctrl; >> + IOInstEnding ccode; >> >> if (~(schib->pmcw.flags) & (PMCW_FLAGS_MASK_DNV | >> PMCW_FLAGS_MASK_ENA)) { >> return IOINST_CC_NOT_OPERATIONAL; >> } >> >> + /* >> + * Save the current scsw.ctrl in case CSCH fails and we need >> + * to revert the scsw to the status quo ante. >> + */ >> + old_scsw_ctrl = schib->scsw.ctrl; >> + >> /* Trigger the clear function. */ >> schib->scsw.ctrl &= ~(SCSW_CTRL_MASK_FCTL | >> SCSW_CTRL_MASK_ACTL); >> schib->scsw.ctrl |= SCSW_FCTL_CLEAR_FUNC | SCSW_ACTL_CLEAR_PEND; >> >> - return do_subchannel_work(sch); >> + ccode = do_subchannel_work(sch); >> + >> + if (ccode != IOINST_CC_EXPECTED) { >> + schib->scsw.ctrl = old_scsw_ctrl; >> + } >> + >> + return ccode; >> } >> >> IOInstEnding css_do_hsch(SubchDev *sch) >> { >> SCHIB *schib = &sch->curr_status; >> + uint16_t old_scsw_ctrl; >> + IOInstEnding ccode; >> >> if (~(schib->pmcw.flags) & (PMCW_FLAGS_MASK_DNV | >> PMCW_FLAGS_MASK_ENA)) { >> return IOINST_CC_NOT_OPERATIONAL; >> @@ -1553,6 +1569,12 @@ IOInstEnding css_do_hsch(SubchDev *sch) >> return IOINST_CC_BUSY; >> } >> >> + /* >> + * Save the current scsw.ctrl in case HSCH fails and we need >> + * to revert the scsw to the status quo ante. >> + */ >> + old_scsw_ctrl = schib->scsw.ctrl; >> + >> /* Trigger the halt function. */ >> schib->scsw.ctrl |= SCSW_FCTL_HALT_FUNC; >> schib->scsw.ctrl &= ~SCSW_FCTL_START_FUNC; >> @@ -1564,7 +1586,13 @@ IOInstEnding css_do_hsch(SubchDev *sch) >> } >> schib->scsw.ctrl |= SCSW_ACTL_HALT_PEND; >> >> - return do_subchannel_work(sch); >> + ccode = do_subchannel_work(sch); >> + >> + if (ccode != IOINST_CC_EXPECTED) { >> + schib->scsw.ctrl = old_scsw_ctrl; >> + } >> + >> + return ccode; >> } >> >> static void css_update_chnmon(SubchDev *sch) >> @@ -1605,6 +1633,8 @@ static void css_update_chnmon(SubchDev *sch) >> IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb) >> { >> SCHIB *schib = &sch->curr_status; >> + uint16_t old_scsw_ctrl, old_scsw_flags; >> + IOInstEnding ccode; >> >> if (~(schib->pmcw.flags) & (PMCW_FLAGS_MASK_DNV | >> PMCW_FLAGS_MASK_ENA)) { >> return IOINST_CC_NOT_OPERATIONAL; >> @@ -1626,11 +1656,26 @@ IOInstEnding css_do_ssch(SubchDev *sch, ORB >> *orb) >> } >> sch->orb = *orb; >> sch->channel_prog = orb->cpa; >> + >> + /* >> + * Save the current scsw.ctrl and scsw.flags in case SSCH fails >> and we need >> + * to revert the scsw to the status quo ante. >> + */ >> + old_scsw_ctrl = schib->scsw.ctrl; >> + old_scsw_flags = schib->scsw.flags; >> + >> /* Trigger the start function. */ >> schib->scsw.ctrl |= (SCSW_FCTL_START_FUNC | >> SCSW_ACTL_START_PEND); >> schib->scsw.flags &= ~SCSW_FLAGS_MASK_PNO; >> >> - return do_subchannel_work(sch); >> + ccode = do_subchannel_work(sch); >> + >> + if (ccode != IOINST_CC_EXPECTED) { >> + schib->scsw.ctrl = old_scsw_ctrl; >> + schib->scsw.flags = old_scsw_flags; >> + } >> + >> + return ccode; >> } >> >> static void copy_irb_to_guest(IRB *dest, const IRB *src, const PMCW >> *pmcw, >