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

[Halil Pasic]

On 05/07/2018 12:12 PM, Cornelia Huck wrote:
> On Fri, 4 May 2018 17:02:07 +0200
> Halil Pasic <address@hidden> 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 <address@hidden>
> > > Signed-off-by: Cornelia Huck <address@hidden>
> >
> > Overall I agree with the patch.
> >
> > Acked-by: Halil Pasic <address@hidden>
> >
> > > ---
> > >    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