s390x/css: disabled subchannels cannot be status pending
PATCH 1/1 s390x/css: disabled subchannels cannot be status pending
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
To fix this, just don't do anything in css_conditional_io_interrupt() if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.
--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)
hw/s390x/css.c +8 -0
@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)void css_conditional_io_interrupt (SubchDev *sch) { +/* +* If the subchannel is not enabled, it is not made status pending +* (see PoP p. 16-17, "Status Control"). +*/ +if (!(sch->curr_status.pmcw.flags &PMCW_FLAGS_MASK_ENA )) { +return ; + } +/* * If the subchannel is not currently status pending, make it pending * with alert status.
Signature · 1 line
2.14.3
Quoted 26 lines · Cornelia Huck
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/s390x/css.c +1
-0@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)void css_conditional_io_interrupt (SubchDev *sch) { +/*
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 629
Quoted 10 lines
hw/s390x/css.c +7
-0+* If the subchannel is not enabled, it is not made status pending +* (see PoP p. 16-17, "Status Control"). +*/ +if (!(sch->curr_status.pmcw.flags &PMCW_FLAGS_MASK_ENA )) { +return ; + } +/* * If the subchannel is not currently status pending, make it pending * with alert status.
Christian Borntraeger
Acked-byQuoted 6 lines · Thomas Huth
Quoted 5 lines · Cornelia Huck
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
Quoted 7 lines
Quoted 7 lines
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>
I agree with your understanding of the PoP.
Quoted 27 lines
Quoted 14 lines
--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/s390x/css.c +1
-0@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)void css_conditional_io_interrupt (SubchDev *sch) { +/* hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 629
Quoted 10 lines
hw/s390x/css.c +7
-0+* If the subchannel is not enabled, it is not made status pending +* (see PoP p. 16-17, "Status Control"). +*/ +if (!(sch->curr_status.pmcw.flags &PMCW_FLAGS_MASK_ENA )) { +return ; + } +/* * If the subchannel is not currently status pending, make it pending * with alert status. Tested-by Thomas Huth <[email protected]>
Could we somehow get 'fix' or something similar into the title?
Also do we need cc stable for this? I guess this is broken for a while now.
Quoted 4 lines · Cornelia Huck
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
Maybe something like 'The spurious status will preclude the successful execution of msch (e.g. when the guest later tries to enable the subchannel).' I had difficulties getting your sentence right-away.
Quoted 6 lines
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.
And I guess vfio-ccw is also unaffected, as the passed through stuff is going to handle this correctly on the lower levels.
Quoted 2 lines
Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>
Overall I agree with the patch.
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 617
Quoted 11 lines
--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/s390x/css.c +0
-0@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)void css_conditional_io_interrupt (SubchDev *sch)
Loosely related:
What is the semantic difference between css_inject_io_interrupt and this css_conditional_io_interrup? The css_conditional_io_interrupt could be 'unsolicited' or 'virtio notification'. This function also relies on the serialization of io instructions or?
Also there is this paragraph to be considered if this is indeed about unsolicited:
"""
The subchannel and device status associated with
an unsolicited interruption condition is never merged with that of any currently existing interruption condi- tion. If the subchannel is currently status pending, the unsolicited interruption condition is held in abeyance in either the channel subsystem or the device, as appropriate, until the status-pending condition has been cleared.
"""
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 622
Quoted 5 lines
hw/s390x/css.c +4
-0{ +/* +* If the subchannel is not enabled, it is not made status pending +* (see PoP p. 16-17, "Status Control"). +*/
In not sure about the reference. We usually don't do that or? And the page number may change. I would probably just drop the PoP reference.
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 626
Quoted 4 lines
hw/s390x/css.c +4
-0+if (!(sch->curr_status.pmcw.flags &PMCW_FLAGS_MASK_ENA )) { +return ; + } +
We don't have a scenario where the guest is expected to do some 'recovery' after being educated about some condition by the means of an alert-status, do we?
I also wonder if this is the right place to handle the problem. The 3870 also manipulates scsw.dstat before calling this. And I'm not sure where/if is that cleared, or if it's OK to leave it set...
Regards,
Halil
Quoted 1 line · On Fri, 4 May 2018 17:02:07 +0200 Halil Pasic <[email protected]> wrote:
Could we somehow get 'fix' or something similar into the title?
Frankly, I have no idea how to do so in a readable way.
Quoted 3 lines
Also do we need cc stable for this? I guess this is broken for a while now.
The function has been like that since its introduction, but only 3270 tries to do something on non-enabled subchannels, and this is the easiest place to fence it off. Stable probably doesn't hurt.
Quoted 10 lines · Cornelia Huck
Quoted 4 lines
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
Maybe something like 'The spurious status will preclude the successful execution of msch (e.g. when the guest later tries to enable the subchannel).' I had difficulties getting your sentence right-away.
I would have problems parsing your sentence :)
"..., we will present a spurious cc 1 (status pending) when it uses msch on it later on, e.g. when trying to enable the subchannel."
?
Quoted 10 lines
Quoted 7 lines
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.And I guess vfio-ccw is also unaffected, as the passed through stuff is going to handle this correctly on the lower levels.
Yes, vfio-ccw does not use this code path at all.
Quoted 18 lines
Quoted 3 lines
Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>Overall I agree with the patch.
Acked-by Halil Pasic <[email protected]>Quoted 10 lines
--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/s390x/css.c +0
-0@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)
Quoted 7 lines
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 617
Quoted 1 line
hw/s390x/css.c +0
-0void css_conditional_io_interrupt (SubchDev *sch)Loosely related:
What is the semantic difference betweencss_inject_io_interruptand thiscss_conditional_io_interrup? Thecss_conditional_io_interruptcould be 'unsolicited' or 'virtio notification'. This function also relies on the serialization of io instructions or?
I introduced the "conditional interrupt" function to inject virtio notifications. 3270 started using this interface later on. I had not intended it to be a "generic unsolicited interrupt" interface.
Quoted 15 lines
Also there is this paragraph to be considered if this is indeed about unsolicited:
"""
The subchannel and device status associated with
an unsolicited interruption condition is never merged with that of any currently existing interruption condi- tion. If the subchannel is currently status pending, the unsolicited interruption condition is held in abeyance in either the channel subsystem or the device, as appropriate, until the status-pending condition has been cleared.
"""
Quoted 8 lines
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 622
Quoted 5 lines
hw/s390x/css.c +4
-0{ +/* +* If the subchannel is not enabled, it is not made status pending +* (see PoP p. 16-17, "Status Control"). +*/ In not sure about the reference. We usually don't do that or? And the page number may change. I would probably just drop the PoP reference.
I did not spot it immediately, that's why I added it.
Quoted 9 lines
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 626
Quoted 5 lines
hw/s390x/css.c +4
-0+if (!(sch->curr_status.pmcw.flags &PMCW_FLAGS_MASK_ENA )) { +return ; + } +We don't have a scenario where the guest is expected to do some 'recovery' after being educated about some condition by the means of an alert-status, do we?
As said, this function was initially introduced for virtio notifications, not as a generic interface.
Quoted 4 lines
I also wonder if this is the right place to handle the problem. The 3870 also manipulates scsw.dstat before calling this. And I'm not sure where/if is that cleared, or if it's OK to leave it set...
It's the easiest way to fix this. 3270 probably wants some review regarding its interaction with the css as well, but that's something for another day.
Quoted 7 lines · Cornelia Huck
Quoted 1 line · On Fri, 4 May 2018 17:02:07 +0200 Halil Pasic <[email protected]> wrote:
Could we somehow get 'fix' or something similar into the title?
Frankly, I have no idea how to do so in a readable way.
OK. Neither do I have a competitive counter proposal.
Quoted 19 lines
Quoted 3 lines
Also do we need cc stable for this? I guess this is broken for a while now.
The function has been like that since its introduction, but only 3270 tries to do something on non-enabled subchannels, and this is the easiest place to fence it off. Stable probably doesn't hurt.
Quoted 10 lines · Cornelia Huck
Quoted 4 lines
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
Maybe something like 'The spurious status will preclude the successful execution of msch (e.g. when the guest later tries to enable the subchannel).' I had difficulties getting your sentence right-away.
Quoted 6 lines
I would have problems parsing your sentence :)
"..., we will present a spurious cc 1 (status pending) when it uses msch on it later on, e.g. when trying to enable the subchannel."
?
I think this is better.
Quoted 40 lines
Quoted 11 lines
Quoted 8 lines
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.And I guess vfio-ccw is also unaffected, as the passed through stuff is going to handle this correctly on the lower levels.
Yes, vfio-ccw does not use this code path at all.
Quoted 18 lines
Quoted 3 lines
Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>Overall I agree with the patch.
Acked-by Halil Pasic <[email protected]>Quoted 10 lines
--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/s390x/css.c +0
-0@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)Quoted 7 lines
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 617
Quoted 1 line
hw/s390x/css.c +0
-0void css_conditional_io_interrupt (SubchDev *sch)Loosely related:
What is the semantic difference betweencss_inject_io_interruptand thiscss_conditional_io_interrup? Thecss_conditional_io_interruptcould be 'unsolicited' or 'virtio notification'. This function also relies on the serialization of io instructions or?
Quoted 4 lines
I introduced the "conditional interrupt" function to inject virtio notifications. 3270 started using this interface later on. I had not intended it to be a "generic unsolicited interrupt" interface.
Thanks for sharing. I think what 3270 does is a bit weird.
[..]
Quoted 9 lines
Quoted 4 lines
I also wonder if this is the right place to handle the problem. The 3870 also manipulates scsw.dstat before calling this. And I'm not sure where/if is that cleared, or if it's OK to leave it set...
It's the easiest way to fix this. 3270 probably wants some review regarding its interaction with the css as well, but that's something for another day.
Nod.
Regards,
Halil
Quoted 26 lines · Cornelia Huck
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/s390x/css.c +1
-0@@ -616,6 +616,14 @@ void css_inject_io_interrupt(SubchDev *sch)void css_conditional_io_interrupt (SubchDev *sch) { +/*
hw/s390x/css.c void css_inject_io_interrupt(SubchDev *sch) · line 629
Quoted 10 lines
hw/s390x/css.c +7
-0+* If the subchannel is not enabled, it is not made status pending +* (see PoP p. 16-17, "Status Control"). +*/ +if (!(sch->curr_status.pmcw.flags &PMCW_FLAGS_MASK_ENA )) { +return ; + } +/* * If the subchannel is not currently status pending, make it pending * with alert status.
Signature · 3 lines
Thanks, David / dhildenb
Cornelia Huck
AppliedQuoted 10 lines · On Fri, 4 May 2018 15:16:05 +0200 Cornelia Huck <[email protected]> wrote:
The 3270 code will try to post an attention interrupt when the 3270 emulator (e.g. x3270) attaches. If the guest has not yet enabled the subchannel for the 3270 device, we will give it a spurious status during msch when it does so later.
To fix this, just don't do anything in
css_conditional_io_interrupt()if the subchannel is not enabled. The 3270 code will work fine with that, and the other user of this function (virtio-ccw) never attempts to post an interrupt for a disabled device to begin with.
Quoted 5 lines
Reported-by Thomas Huth <[email protected]>Signed-off-by Cornelia Huck <[email protected]>--- hw/s390x/css.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Queued to s390-next (with commit message tweaked and cc:stable added).