Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck <address@hidden> [2017-07-31 13:13:02 +0200]:

> On Mon, 31 Jul 2017 11:51:37 +0800
> Dong Jia Shi <address@hidden> wrote:
> 
> > * Cornelia Huck <address@hidden> [2017-07-27 13:59:10 +0200]:
> > 
> > > On Thu, 27 Jul 2017 03:54:18 +0200
> > > Dong Jia Shi <address@hidden> 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.  
> > vfio-ccw hotplug could also live with the current mechanism - just
> > generate the chp according to its CHPIDs information. What the problem
> > in practice for it then? Channel path status change could be synchronize
> > by adding more MMIO regions and eventfd irq for vfio-ccw.
> 
> I'm not sure that there is a problem in practice (I suppose there
> isn't), but it feels weird. The user plugs a device, magically the
> paths are created.
Understand.

> 
> > 
> > > 
> > > 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.  
> > If we want to adopt the unified modelling for all kinds of devices, then
> > we require the user to define chps before define devices.
> 
> Yes.
> 
> > 
> > We could defaulty always have a virtio reserved chp 0 defined on each
> > css, so we do not need to touch the current virtio devices command line.
> 
> I think that's even mandatory, or we break backwards compatibility.
Nod.

> 
> > Defining more chps or changing chpid for virtio devices does not provide
> > added values.
> 
> Agreed.
> 
> > 
> > For emulated device, we can define chpids for use. E.g.:
> > -device chp,cssid=fe,chpid=11 \
> > -device chp,cssid=fe,chpid=22 \
> > -chardev socket,id=terminal0,host=0.0.0.0,port=23,nowait,server,tn3270 \
> > -device 
> > x-terminal3270,chardev=terminal0,id=terminal3270_0,devno=fe.0.000a,chpids=1122000000000000
> 
> If we go that route (which I'm not too sure of), we should rather
> reference the chp objects by id instead of providing a to-be-parsed
> parameter.
Got this and Nod.

> 
> > 
> > Or, I think, we could let Qemu automatically find a free chp for them.
> > Sine, the same as the virtio devices, defining more chps or changing
> > chpid for emulated devices does provide added values either. In this
> > case, we do not need to touch the emualted device command line too.
> 
> We should keep this working for compat (even if it's an x- device).
Yes. Finding a free chp when needed, is what the emulated device
currently does. So this will be compatable with the current stuff.

> 
> > 
> > When defining a vfio-ccw device, since the real subchannel implicitly
> > indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> > my current work, we could even retrieve these information from a new
> > added MMIO region). In this case, defining some channel path devices
> > separately does not make sense to me.
> 
> We might want to pass only a subset of the channel paths to guest. This
> can only work if we can define individual chp objects.
Why would we want this?

We can add, for example, a "chpids" parameter for the "vfio-ccw" device
to limit its chpids to a subset that we want it to have? E.g.:

For this mdev:
MDEV                                  Subchan.  PIM PAM POM  CHPIDs
------------------------------------------------------------------------------
6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000

We could use this command line:
-device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245000000000000
                                                              ^^^^

> 
> > 
> > After thinking quite a while, if we do want to add a real device object
> > for a channel path, the most intractable problem (but not the only one)
> > for me is to find a good way to map the real path with the virtual one.
> > How would we retrieve the information from the real one? We'd need the
> > host kernel to provide totally new interfaces for channel path
> > information synchronization and notification machenism. I don't think in
> > this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd
> > could be a better choice. I think, this is like we are trying to
> > passthru a channel path. So we'd need to have a new vfio device physical
> > driver (e.g. vfio-chp) to handle this...
> 
> And that would run into the problem of (1) the chp objects not using a
> bus on Linux, and (2) implying dedicating the chpids, which we likely
> do not want.
Yes. Tough problem.

> 
> > 
> > And, if we finnaly find a way to solve the above problem, we may have
> > some commandline as the follows, and there is still other problems. E.g.:
> > 
> > lscss:
> > MDEV                                  Subchan.  PIM PAM POM  CHPIDs
> > ------------------------------------------------------------------------------
> > 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 
> > 00000000
> > 
> > lschp:
> > CHPID  Vary  Cfg.  Type  Cmg  Shared  PCHID
> > ============================================
> > 0.42   1     1     1b    2    1       0158 
> > 0.43   1     1     1b    2    1       0159 
> > 0.44   1     1     1b    2    1       01a0 
> > 0.45   1     1     1b    2    1       01a1
> > 
> > Suppose we want to pass through the above mdev ($MDEV_CCW013f), we could
> > have the following command line:
> > -device vfio-chp,sysfsdev=$MDEV_CHP42,cssid=0,chpid=42 \
> > -device vfio-chp,sysfsdev=$MDEV_CHP43,cssid=0,chpid=43 \
> > -device vfio-chp,sysfsdev=$MDEV_CHP44,cssid=0,chpid=44 \
> > -device vfio-chp,sysfsdev=$MDEV_CHP45,cssid=0,chpid=45 \
> > -device 
> > vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4243444500000000
> > 
> > The above vfio-ccw devices can not use any other CHP besides the above
> > 4. Defining only some of the 4 CHPs for the vfio-ccw device does not
> > sounds reasonable. So isn't it redundant to explicitly define all of the
> > 4 chps in command line for the vfio-ccw device? Since, itself already
> > provides the CHPIDs information...
> 
> See my comments above.
Done.

> 
> > 
> > > - Give channel paths states: Defined, configured. The right time for a
> > >   CRW is the transition between those states.  
> > Sounds reasonable.
> > 
> > > - 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.  
> > Sounds reasonable too.
> > 
> > > 
> > > Something along those lines also matches better what I've seen on z/VM
> > > or LPAR. I realize that it's not easy :(  
> > Yes... I don't find out a way that can satisfy all of the above
> > requirements for now...
> 
> Even if you can, it sounds like a lot of work :(
Sounds like that will take a year to accomplish...

> 
> > 
> > > 
> > > tl;dr: I don't think we want chp crws until after we have a good chp
> > > model.  
> > I have to agree.
> 
> I'm wondering whether we should just defer this to later. We can live
> with no chp crw being created (except for rchp), as due to the crw
> generation being unreliable the guest OS has to handle path changes
> without crws anyway.
I know this. But how couldn't I get the idea to defer the crw
generation?! :D

> 
> We just need to make sure that pno and friends are appropriately
> reported to the guest.
> 
Will try!

-- 
Dong Jia Shi