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

[Cornelia Huck]

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.

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

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

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

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

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

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

> 
> > - 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 :(

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

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