| Message ID | 20171201143136.62497-2-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 1 Dec 2017 15:31:34 +0100 Halil Pasic <pasic@linux.vnet.ibm.com> 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 <pasic@linux.vnet.ibm.com> > > --- > 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.
On 12/04/2017 12:10 PM, Cornelia Huck wrote: [..] > >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> squashed or non-squashed: Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
On 12/04/2017 12:10 PM, Cornelia Huck wrote: > On Fri, 1 Dec 2017 15:31:34 +0100 > Halil Pasic <pasic@linux.vnet.ibm.com> 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? > Yep. Copy paste mistake. >> >> 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/ Nod > >> 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 s/Deprecaton/Deprecation/ >> 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? > I think having a document collecting such migration rules and best practices for command line users (and implicitly also for implementers of management software) would be a good idea. Maybe there is such a documentation, but I don't know where. The devnos should be a part of it for sure. But I'm not volunteering for creating this kind of documentation. Natural languages aren't my forte. >> >> 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: > If you want, we can iterate on the description. My primary concern was to agree on how to advertise this change. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> >> --- >> 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. > Agree.
On Mon, 4 Dec 2017 16:02:14 +0100 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 12/04/2017 12:10 PM, Cornelia Huck wrote: > > On Fri, 1 Dec 2017 15:31:34 +0100 > > Halil Pasic <pasic@linux.vnet.ibm.com> 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? > > > > Yep. Copy paste mistake. > > >> > >> 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/ > > Nod > > > > >> 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 > > s/Deprecaton/Deprecation/ > > >> 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? > > > > I think having a document collecting such migration rules and best practices > for command line users (and implicitly also for implementers of management > software) would be a good idea. Maybe there is such a documentation, but > I don't know where. The devnos should be a part of it for sure. But I'm > not volunteering for creating this kind of documentation. Natural languages > aren't my forte. I would not mind someone else doing this. > > >> > >> 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: > > > > If you want, we can iterate on the description. My primary concern was > to agree on how to advertise this change. Let's skip that. > > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-12-01 15:31:34 +0100]: [...] No comment for the message part. The code looks good to me. So after squashing with patch #2: Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > --- > 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(-) > > diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c > index 081e3ef6f4..3af13ea027 100644 > --- a/hw/s390x/3270-ccw.c > +++ b/hw/s390x/3270-ccw.c > @@ -104,7 +104,7 @@ static void emulated_ccw_3270_realize(DeviceState *ds, Error **errp) > SubchDev *sch; > Error *err = NULL; > > - sch = css_create_sch(cdev->devno, true, cbus->squash_mcss, errp); > + sch = css_create_sch(cdev->devno, cbus->squash_mcss, errp); > if (!sch) { > return; > } > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index f6b5c807cd..cd26f32050 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -2370,22 +2370,12 @@ const PropertyInfo css_devid_ro_propinfo = { > .get = get_css_devid, > }; > > -SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss, > - Error **errp) > +SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp) > { > uint16_t schid = 0; > SubchDev *sch; > > if (bus_id.valid) { > - if (is_virtual != (bus_id.cssid == VIRTUAL_CSSID)) { > - error_setg(errp, "cssid %hhx not valid for %s devices", > - bus_id.cssid, > - (is_virtual ? "virtual" : "non-virtual")); > - return NULL; > - } > - } > - > - if (bus_id.valid) { > if (squash_mcss) { > bus_id.cssid = channel_subsys.default_cssid; > } else if (!channel_subsys.css[bus_id.cssid]) { > @@ -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;;) { > 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; > } > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index 0ef232ec27..4a9d4d2534 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -77,7 +77,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp) > goto out_err_propagate; > } > > - sch = css_create_sch(ccw_dev->devno, false, cbus->squash_mcss, &err); > + sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, &err); > if (!sch) { > goto out_mdevid_free; > } > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 6a57f94197..4d65a50334 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -302,7 +302,6 @@ static void ccw_init(MachineState *machine) > /* > * Non mcss-e enabled guests only see the devices from the default > * css, which is determined by the value of the squash_mcss property. > - * Note: we must not squash non virtual devices to css 0xFE. > */ > if (css_bus->squash_mcss) { > ret = css_create_css_image(0, true); > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 184515ce94..3dd902a664 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -701,7 +701,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp) > SubchDev *sch; > Error *err = NULL; > > - sch = css_create_sch(ccw_dev->devno, true, cbus->squash_mcss, errp); > + sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, errp); > if (!sch) { > return; > } > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index ab6ebe66b5..53c270a216 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -272,12 +272,9 @@ extern const PropertyInfo css_devid_ro_propinfo; > * default css image for it. > * If @p bus_id is valid, and @p squash_mcss is false, verify that it is > * not already in use, and find a free devno for it. > - * If @p bus_id is not valid, and if either @p squash_mcss or @p is_virtual > - * is true, find a free subchannel id and device number across all > - * subchannel sets from the default css image. > - * If @p bus_id is not valid, and if both @p squash_mcss and @p is_virtual > - * are false, find a non-full css image and find a free subchannel id and > - * device number across all subchannel sets from it. > + * If @p bus_id is not valid find a free subchannel id and device number > + * across all subchannel sets and all css images starting from the default > + * css image. > * > * If either of the former actions succeed, allocate a subchannel structure, > * initialise it with the bus id, subchannel id and device number, register > @@ -286,8 +283,7 @@ extern const PropertyInfo css_devid_ro_propinfo; > * The caller becomes owner of the returned subchannel structure and > * is responsible for unregistering and freeing it. > */ > -SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss, > - Error **errp); > +SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp); > > /** Turn on css migration */ > void css_register_vmstate(void); > -- > 2.13.5 >
diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c index 081e3ef6f4..3af13ea027 100644 --- a/hw/s390x/3270-ccw.c +++ b/hw/s390x/3270-ccw.c @@ -104,7 +104,7 @@ static void emulated_ccw_3270_realize(DeviceState *ds, Error **errp) SubchDev *sch; Error *err = NULL; - sch = css_create_sch(cdev->devno, true, cbus->squash_mcss, errp); + sch = css_create_sch(cdev->devno, cbus->squash_mcss, errp); if (!sch) { return; } diff --git a/hw/s390x/css.c b/hw/s390x/css.c index f6b5c807cd..cd26f32050 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -2370,22 +2370,12 @@ const PropertyInfo css_devid_ro_propinfo = { .get = get_css_devid, }; -SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss, - Error **errp) +SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp) { uint16_t schid = 0; SubchDev *sch; if (bus_id.valid) { - if (is_virtual != (bus_id.cssid == VIRTUAL_CSSID)) { - error_setg(errp, "cssid %hhx not valid for %s devices", - bus_id.cssid, - (is_virtual ? "virtual" : "non-virtual")); - return NULL; - } - } - - if (bus_id.valid) { if (squash_mcss) { bus_id.cssid = channel_subsys.default_cssid; } else if (!channel_subsys.css[bus_id.cssid]) { @@ -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;;) { 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; } diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c index 0ef232ec27..4a9d4d2534 100644 --- a/hw/s390x/s390-ccw.c +++ b/hw/s390x/s390-ccw.c @@ -77,7 +77,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp) goto out_err_propagate; } - sch = css_create_sch(ccw_dev->devno, false, cbus->squash_mcss, &err); + sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, &err); if (!sch) { goto out_mdevid_free; } diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 6a57f94197..4d65a50334 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -302,7 +302,6 @@ static void ccw_init(MachineState *machine) /* * Non mcss-e enabled guests only see the devices from the default * css, which is determined by the value of the squash_mcss property. - * Note: we must not squash non virtual devices to css 0xFE. */ if (css_bus->squash_mcss) { ret = css_create_css_image(0, true); diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 184515ce94..3dd902a664 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -701,7 +701,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp) SubchDev *sch; Error *err = NULL; - sch = css_create_sch(ccw_dev->devno, true, cbus->squash_mcss, errp); + sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, errp); if (!sch) { return; } diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index ab6ebe66b5..53c270a216 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -272,12 +272,9 @@ extern const PropertyInfo css_devid_ro_propinfo; * default css image for it. * If @p bus_id is valid, and @p squash_mcss is false, verify that it is * not already in use, and find a free devno for it. - * If @p bus_id is not valid, and if either @p squash_mcss or @p is_virtual - * is true, find a free subchannel id and device number across all - * subchannel sets from the default css image. - * If @p bus_id is not valid, and if both @p squash_mcss and @p is_virtual - * are false, find a non-full css image and find a free subchannel id and - * device number across all subchannel sets from it. + * If @p bus_id is not valid find a free subchannel id and device number + * across all subchannel sets and all css images starting from the default + * css image. * * If either of the former actions succeed, allocate a subchannel structure, * initialise it with the bus id, subchannel id and device number, register @@ -286,8 +283,7 @@ extern const PropertyInfo css_devid_ro_propinfo; * The caller becomes owner of the returned subchannel structure and * is responsible for unregistering and freeing it. */ -SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss, - Error **errp); +SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp); /** Turn on css migration */ void css_register_vmstate(void);
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. 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 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). 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. 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. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- 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(-)