* Re: [PATCH v2] s390x/css: revert SCSW ctrl/flag bits on error
2022-10-28 20:22 ` Eric Farman
@ 2022-10-28 20:26 ` Peter Jin
2022-10-31 14:08 ` Matthew Rosato
2022-11-06 10:13 ` Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Peter Jin @ 2022-10-28 20:26 UTC (permalink / raw)
To: Eric Farman, pasic, borntraeger, richard.henderson, david, cohuck,
thuth, mjrosato
Cc: peter, qemu-s390x, qemu-devel
On 10/28/22 16:22, 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.
> """
>
>> Signed-off-by: Peter Jin <pjin@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 <farman@linux.ibm.com>
Okay, that sounds much better :)
>> ---
>> 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,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] s390x/css: revert SCSW ctrl/flag bits on error
2022-10-28 20:22 ` Eric Farman
2022-10-28 20:26 ` Peter Jin
@ 2022-10-31 14:08 ` Matthew Rosato
2022-10-31 14:44 ` Peter Jin
2022-11-06 10:13 ` Thomas Huth
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Rosato @ 2022-10-31 14:08 UTC (permalink / raw)
To: Eric Farman, Peter Jin, pasic, borntraeger, richard.henderson,
david, cohuck, thuth
Cc: peter, qemu-s390x, qemu-devel
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 <pjin@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 <farman@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] s390x/css: revert SCSW ctrl/flag bits on error
2022-10-31 14:08 ` Matthew Rosato
@ 2022-10-31 14:44 ` Peter Jin
0 siblings, 0 replies; 6+ messages in thread
From: Peter Jin @ 2022-10-31 14:44 UTC (permalink / raw)
To: Matthew Rosato, Eric Farman, pasic, borntraeger,
richard.henderson, david, cohuck, thuth
Cc: peter, qemu-s390x, qemu-devel
To everyone else on the mailing lists,
I apologize for any confusion caused by this being a "v2" of the
original "v1" patch and not submitting a changelog. I am rather new to
these kinds of mailing lists, though Eric and Matt here have been really
helpful in getting this correct. I will remember to put changelogs for
v2, v3, etc. patches before submitting them!
Anyways, changes from v1:
- no code changes, only added additional explanatory text.
If you think I need to post a v3 to put this inline, let me know.
Peter
On 10/31/22 10:08, Matthew Rosato wrote:
> 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 <pjin@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 <farman@linux.ibm.com>
> Reviewed-by: Matthew Rosato <mjrosato@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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] s390x/css: revert SCSW ctrl/flag bits on error
2022-10-28 20:22 ` Eric Farman
2022-10-28 20:26 ` Peter Jin
2022-10-31 14:08 ` Matthew Rosato
@ 2022-11-06 10:13 ` Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2022-11-06 10:13 UTC (permalink / raw)
To: Eric Farman, Peter Jin, pasic, borntraeger, richard.henderson,
david, cohuck, mjrosato
Cc: peter, qemu-s390x, qemu-devel
On 28/10/2022 22.22, 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.
> """
>
>>
>> Signed-off-by: Peter Jin <pjin@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 <farman@linux.ibm.com>
Thanks, I've queued the patch now to my s390x-next branch with the updated
commit message. Please double-check whether that looks OK now:
https://gitlab.com/thuth/qemu/-/commits/s390x-next/
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread