On 12/04/2017 12:10 PM, Cornelia Huck wrote:
> On Fri, 1 Dec 2017 15:31:34 +0100
> Halil Pasic <pa...@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 <pa...@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.
