Re: [qemu-s390x] [PATCH 1/3] s390x/css: unrestrict cssids

[Cornelia Huck]

On Fri,  1 Dec 2017 15:31:34 +0100
Halil Pasic <address@hidden> wrote:

> The default css 0xfe is currently restricted to virtual subchannel
> devices. The hope when the decision was made was, that non-virtual
> subchannel devices will come around when guests can exploit multiple
> channel subsystems. Since current guests don't do that, the pain of the
> partitioned (cssid) namespace outweighs the gain.
> 
> The default css 0xfe is currently restricted to virtual subchannel
> devices. The hope when the decision was made was, that non-virtual
> subchannel devices will come around when guest can exploit multiple
> channel subsystems. Since the guests generally don't do, the pain
> of the partitioned (cssid) namespace outweighs the gain.

Doubled paragraph?

> 
> Let us remove the corresponding restrictions (virtual devices
> can be put only in 0xfe and non-virtual devices in any css except
> the 0xfe -- while s390-squash-mcss then remaps everything to cssid 0).
> 
> At the same time, change our schema for generating css bus ids to put
> both virtual and non-virtual devices into the default css (spilling over
> into other css images, if needed). The intention is to deprecate
> s390-squash-mcss. Whit this change devices without a specified devno

s/Whit/With/

> won't end up hidden to guests not supporting multiple channel subsystems,
> unless this can not be avoided (default css full).
> 
> Deprecaton of s390-squash-mcss and indicating the changes via QMP is
> expected to follow soon (as separate commits).

Let's drop this paragraph (the qmp interface should be squashed in, and
you mention the deprecation right above.)

> 
> The adverse effect of getting rid of the restriction on migration should
> not be too severe.  Vfio-ccw devices are not live-migratable yet, and for
> virtual devices using the extra freedom would only make sense with the
> aforementioned guest support in place.
> 
> The auto-generated bus ids are affected by both changes. We hope to not
> encounter any auto-generated bus ids in production as Libvirt is always
> explicit about the bus id.  Since 8ed179c937 ("s390x/css: catch section
> mismatch on load", 2017-05-18) the worst that can happen because the same
> device ended up having a different bus id is a cleanly failed migration.
> I find it hard to reason about the impact of changed auto-generated bus
> ids on migration for command line users as I don't know which rules is
> such an user supposed to follow.

Should we document somewhere that guests supposed to be migrated should
make sure that they use explicit devnos?

> 
> Another pain-point is down- or upgrade of QEMU for command line users.
> The old way and the new way of doing vfio-ccw are mutually incompatible.
> Libvirt is only going to support the new way, so for libvirt users, the
> possible problems at QEMU downgrade are the following. If a domain
> contains virtual devices placed into a css different than 0xfe the domain
> will refuse to start with a QEMU not having this patch. Putting devices
> into a css different that 0xfe however won't make much sense in the near
> future (guest support). Libvirt will refuse to do vfio-ccw with a QEMU
> not having this patch. This is business as usual.

My writing style would be to have this as a shorter, bulleted list -
but no need to rewrite this if this is understandable to the others on
cc:

> 
> Signed-off-by: Halil Pasic <address@hidden>
> 
> ---
> Hi!
> 
> I've factored out the announcing via QMP interface stuff to ease review.
> I would not mind the two being squashed together before this hits main,
> as I would much prefer having the two as one (atomic) change. But the
> second part turned out so controversial, that splitting is expected to
> benefit the review process.
> 
> v2 -> v3:
> * factored out announcing into a separate patch
> * reworded commit message
> * removed outdated comment about squash
> 
> v1 -> v2:
> * changed ccw bus id generation too (see commit message)
> * moved the property to the machine (see cover letter)
> * added a description to the property
> ---
>  hw/s390x/3270-ccw.c        |  2 +-
>  hw/s390x/css.c             | 28 ++++------------------------
>  hw/s390x/s390-ccw.c        |  2 +-
>  hw/s390x/s390-virtio-ccw.c |  1 -
>  hw/s390x/virtio-ccw.c      |  2 +-
>  include/hw/s390x/css.h     | 12 ++++--------
>  6 files changed, 11 insertions(+), 36 deletions(-)
> 

> @@ -2396,19 +2386,8 @@ SubchDev *css_create_sch(CssDevId bus_id, bool 
> is_virtual, bool squash_mcss,
>                                             bus_id.devid, &schid, errp)) {
>              return NULL;
>          }
> -    } else if (squash_mcss || is_virtual) {
> -        bus_id.cssid = channel_subsys.default_cssid;
> -
> -        if (!css_find_free_subch_and_devno(bus_id.cssid, &bus_id.ssid,
> -                                           &bus_id.devid, &schid, errp)) {
> -            return NULL;
> -        }
>      } else {
> -        for (bus_id.cssid = 0; bus_id.cssid < MAX_CSSID; ++bus_id.cssid) {
> -            if (bus_id.cssid == VIRTUAL_CSSID) {
> -                continue;
> -            }
> -
> +        for (bus_id.cssid = channel_subsys.default_cssid;;) {

This looks a bit ugly, but I don't see another compact way to do this.

>              if (!channel_subsys.css[bus_id.cssid]) {
>                  css_create_css_image(bus_id.cssid, false);
>              }
> @@ -2418,7 +2397,8 @@ SubchDev *css_create_sch(CssDevId bus_id, bool 
> is_virtual, bool squash_mcss,
>                                                  NULL)) {
>                  break;
>              }
> -            if (bus_id.cssid == MAX_CSSID) {
> +            bus_id.cssid = (bus_id.cssid + 1) % MAX_CSSID;
> +            if (bus_id.cssid == channel_subsys.default_cssid) {
>                  error_setg(errp, "Virtual channel subsystem is full!");
>                  return NULL;
>              }

The interface exposing this change definitely needs to be squashed into
this patch, but else looks good.