On 07/27/2017 01:59 PM, Cornelia Huck wrote: > On Thu, 27 Jul 2017 03:54:18 +0200 > Dong Jia Shi <bjsdj...@linux.vnet.ibm.com> wrote: > >> When a channel path is hot plugged into a CSS, we should generate >> a channel path initialized CRW (channel report word). The current >> code does not do that, instead it puts a stub function with a TODO >> reminder there. >> >> This implements the css_generate_chp_crws() function by: >> 1. refactor the existing code. >> 2. add an @add parameter to provide future callers with the >> capability of generating channel path permanent error with >> facility not initialized CRW. >> 3. add a @hotplugged parameter, so to opt out generating initialized >> CRWs for predefined channel paths. > > I'm not 100% sure whether the logic is correct here. Let me elaborate: > > The current code flow when hotplugging a device is: > - Generate the schib. > - Check if any of the chpids refers to a not yet existing channel path; > generate it if that is the case. > - Post a crw for the subchannel. > > The second step is where the current code seems to be not quite correct > already. It is fine for coldplugged devices, but I really think we need > to make sure that all referenced channel paths are in place before we > hotplug a new device. It was not really relevant when we just had one > very virtual channel path, and 3270 is experimental so it is not a > problem in practice.
What do you mean by not quite correct? Are we somewhere in conflict with the AR (if yes could you give me a pointer)? Or is it a modeling concern? Or is it about the user interface design? Or any combination of these? > > This, of course, implies we need deeper changes. We need to create the > channel paths before the subchannel is created and refuse hotplug of a > device if not all channel paths it needs are defined. This means we > need some things before we can claim real channel path support: > - Have a way to specify channel paths on the command line resp. when > hotplugging. This implies they need to be real objects. > - Have a way to specify which channel paths belong to a subchannel in > the same context. Keep existing device types working with the current > method. > - Give channel paths states: Defined, configured. The right time for a > CRW is the transition between those states. > - Only queue a 'device come' CRW for a subchannel if at least one of > its channel paths is in the configured state. Detach or make not > operational a subchannel if all of its paths are deconfigured. > AFAIU in your opinion our model is to simple and needs to get more complex. What benefits do we expect from the added complexity? I mean our current model works (and I'm not sure what limitations do we want to get rid of, or even what are the relevant limitations we have.) > Something along those lines also matches better what I've seen on z/VM > or LPAR. I realize that it's not easy :( > > tl;dr: I don't think we want chp crws until after we have a good chp > model. I fully agree with this point. Regards, Halil > >> >> Signed-off-by: Dong Jia Shi <bjsdj...@linux.vnet.ibm.com> >> --- >> hw/s390x/3270-ccw.c | 3 ++- >> hw/s390x/css.c | 55 >> ++++++++++++++++++++++++++++++++++++----------- >> hw/s390x/s390-ccw.c | 2 +- >> hw/s390x/virtio-ccw.c | 3 ++- >> include/hw/s390x/css.h | 8 ++++--- >> include/hw/s390x/ioinst.h | 1 + >> 6 files changed, 53 insertions(+), 19 deletions(-) >