Re: [qemu-s390x] [PATCH 0/2] s390x: reset handling for ccw devices

[Halil Pasic]

On 05/08/2018 03:55 PM, Cornelia Huck wrote:
> On Tue, 8 May 2018 15:29:50 +0200
> Halil Pasic <address@hidden> wrote:
>
> > On 05/07/2018 05:51 PM, Cornelia Huck wrote:
> > > On Friday, Thomas noticed some problems with 3270 devices. One result
> > > was "s390x/css: disabled subchannels cannot be status pending", but
> > > a reboot did not cure the previous broken status. Turns out that 3270
> > > devices are missing a reset handler.
> > >
> > > This series cleans up reset handling a bit and makes sure that the base
> > > ccw device class provides a subchannel reset handler. I'm currently
> > > not sure what we should do with vfio-ccw, so the behaviour there is
> > > left unchanged.
> >
> > Had a look, and LGTM (will tag separately) modulo what follows here. I'm
> > a bit concerned about vfio-ccw not being made compliant to this new
> > he reset of CCWDeviceClass is taking care of resetting the subchannel data
> > structures. This feels like introducing a common abstraction
> > to me, but then some things bother me.
>
> We are having a common abstraction that can be overwritten by any
> specialized implementation - and this is what vfio-ccw is doing,
> therefore nothing changes for it.

Sorry my OO background is making me overthink things. I was on
the separation of concerns line: base class takes care of its
own class invariants and the the derived of its own. Considering
what your statements below, I think we are in agreement.

> > In particular the the pim, the lpm and the pam set in css_reset_sch
> > seems to suit only devices that use the virtual chp. That
> > is it ain't suits any CCWDevice instance.
>
> Yes, we need to revisit this code and split out what makes sense and
> what doesn't. For now, we only have virtual devices and vfio-ccw, so
> we're fine. It even might make sense to keep them separate in the
> future, as having a virtual device and one only mirroring some state in
> QEMU sound like they want to be handled differently.

I agree. The last sentence probably means that resetting the in QEMU
state may not be sufficient.

Another to me somewhat strange thing I noticed is this disabled_cb
used by virtio. It's is I guess the way it it is (specified in
the OASIS spec and everything), but I don't really understand how
this aligns with what the PoP says about MSCH. I mean AFAIU
MSCH does not trigger any communication between the channel subsystem
and the CU and or the device. I read the PoP as nothing is supposed
to change expect the things specified where MSCH is described. I guess
it is just another strange thing we have to live with -- for historical
reasons.

Regards,
Halil