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-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.

> 
> 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.

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.
Defining more chps or changing chpid for virtio devices does not provide
added values.

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

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.

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.

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, 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...

> - 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...

> 
> tl;dr: I don't think we want chp crws until after we have a good chp
> model.
I have to agree.

> 
> > 
> > Signed-off-by: Dong Jia Shi <address@hidden>
> > ---
> >  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(-)
> 

-- 
Dong Jia Shi