Re: [Qemu-devel] [PATCH] s390x/css: disabled subchannels cannot be status pending

[Halil Pasic <pasic@linux.ibm.com>]

On 05/07/2018 12:12 PM, Cornelia Huck wrote:
> On Fri, 4 May 2018 17:02:07 +0200
> Halil Pasic <pasic@linux.ibm.com> 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.

>>
>> 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.
> 
>>
>> On 05/04/2018 03:16 PM, Cornelia Huck 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.
>>
>> 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."
> 
> ?

I think this is better.

> 
>>
>>>
>>> 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.
> 
>>
>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>
>> Overall I agree with the patch.
>>
>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>>
>>> ---
>>>    hw/s390x/css.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index 301bf1772f..56c3fa8c89 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -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?
> 
> 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.
  
[..]
  
>>
>> 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