[Qemu-devel] [PATCH 0/3] unrestrict cssids related patches

[Qemu-devel] [PATCH 0/3] unrestrict cssids related patches

[Halil Pasic]

Hi all!

This serises is a follow up of:
* [RFC PATCH v2 1/1] s390x/css: unrestrict cssids
* [RFC PATCH 1/1] s390x: deprecate s390-squash-mcss

See the commit messages and cover letters of the individual
patches for more details.

Halil Pasic (3):
  s390x/css: unrestrict cssids
  s390x/css: advertise unrestricted cssids
  s390x: deprecate s390-squash-mcss machine prop

 hw/s390x/3270-ccw.c        |  2 +-
 hw/s390x/css-bridge.c      | 11 +++++++++++
 hw/s390x/css.c             | 28 ++++------------------------
 hw/s390x/s390-ccw.c        |  2 +-
 hw/s390x/s390-virtio-ccw.c |  7 +++++--
 hw/s390x/virtio-ccw.c      |  2 +-
 include/hw/s390x/css.h     | 12 ++++--------
 qemu-doc.texi              |  8 ++++++++
 qemu-options.hx            |  8 +++++++-
 9 files changed, 42 insertions(+), 38 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/3] s390x/css: unrestrict cssids

[Halil Pasic]

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

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

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

[Cornelia Huck]

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.

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

[Christian Borntraeger]

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>

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

[Halil Pasic]

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.

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

[Cornelia Huck]

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>

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

[Dong Jia Shi]

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

-- 
Dong Jia Shi

[Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Halil Pasic]

Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
to the management software (so it can tell are cssids unrestricted or
restricted).

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---

Boris says having the property on the virtual-css-bridge is good form
Libvirt PoV. @Shalini: could you verify that things work out fine
(provided we get at least a preliminary blessing from Connie).

Consider squashing into "s390x/css: unrestrict cssids".
---
 hw/s390x/css-bridge.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
index c4a9735d71..c7e8998680 100644
--- a/hw/s390x/css-bridge.c
+++ b/hw/s390x/css-bridge.c
@@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static bool prop_get_true(Object *obj, Error **errp)
+{
+    return true;
+}
+
 static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
 {
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
@@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
     hc->unplug = ccw_device_unplug;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->props = virtual_css_bridge_properties;
+    object_class_property_add_bool(klass, "cssid-unrestricted",
+                                   prop_get_true, NULL, NULL);
+    object_class_property_set_description(klass, "cssid-unrestricted",
+            "A css device can use any  cssid, regardless whether virtual"
+            " or not (read only, always true)",
+            NULL);
 }
 
 static const TypeInfo virtual_css_bridge_info = {
-- 
2.13.5

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Christian Borntraeger]

On 12/01/2017 03:31 PM, Halil Pasic wrote:
> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> to the management software (so it can tell are cssids unrestricted or
> restricted).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> Boris says having the property on the virtual-css-bridge is good form
> Libvirt PoV. @Shalini: could you verify that things work out fine
> (provided we get at least a preliminary blessing from Connie).
> 
> Consider squashing into "s390x/css: unrestrict cssids".



FWIW, a property on the  virtual-css-bridge is
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

So if Conny is also ok with this we might have found our solution.

> ---
>  hw/s390x/css-bridge.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index c4a9735d71..c7e8998680 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
> 
> +static bool prop_get_true(Object *obj, Error **errp)
> +{
> +    return true;
> +}
> +
>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>  {
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>      hc->unplug = ccw_device_unplug;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->props = virtual_css_bridge_properties;
> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> +                                   prop_get_true, NULL, NULL);
> +    object_class_property_set_description(klass, "cssid-unrestricted",
> +            "A css device can use any  cssid, regardless whether virtual"
> +            " or not (read only, always true)",
> +            NULL);
>  }
> 
>  static const TypeInfo virtual_css_bridge_info = {
> 

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Cornelia Huck]

On Fri,  1 Dec 2017 15:31:35 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> to the management software (so it can tell are cssids unrestricted or
> restricted).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> Boris says having the property on the virtual-css-bridge is good form
> Libvirt PoV. @Shalini: could you verify that things work out fine
> (provided we get at least a preliminary blessing from Connie).
> 
> Consider squashing into "s390x/css: unrestrict cssids".
> ---
>  hw/s390x/css-bridge.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index c4a9735d71..c7e8998680 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static bool prop_get_true(Object *obj, Error **errp)
> +{
> +    return true;
> +}
> +
>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>  {
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>      hc->unplug = ccw_device_unplug;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->props = virtual_css_bridge_properties;
> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> +                                   prop_get_true, NULL, NULL);
> +    object_class_property_set_description(klass, "cssid-unrestricted",
> +            "A css device can use any  cssid, regardless whether virtual"

extra space -----------------------------^

> +            " or not (read only, always true)",
> +            NULL);
>  }
>  
>  static const TypeInfo virtual_css_bridge_info = {

Looks reasonable. If this works as expected, I'll squash it into the
previous patch.

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Halil Pasic]

On 12/04/2017 12:15 PM, Cornelia Huck wrote:
> On Fri,  1 Dec 2017 15:31:35 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
>> to the management software (so it can tell are cssids unrestricted or
>> restricted).
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> Boris says having the property on the virtual-css-bridge is good form
>> Libvirt PoV. @Shalini: could you verify that things work out fine
>> (provided we get at least a preliminary blessing from Connie).
>>
>> Consider squashing into "s390x/css: unrestrict cssids".
>> ---
>>  hw/s390x/css-bridge.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>> index c4a9735d71..c7e8998680 100644
>> --- a/hw/s390x/css-bridge.c
>> +++ b/hw/s390x/css-bridge.c
>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> +static bool prop_get_true(Object *obj, Error **errp)
>> +{
>> +    return true;
>> +}
>> +
>>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>  {
>>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>      hc->unplug = ccw_device_unplug;
>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>      dc->props = virtual_css_bridge_properties;
>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>> +                                   prop_get_true, NULL, NULL);
>> +    object_class_property_set_description(klass, "cssid-unrestricted",
>> +            "A css device can use any  cssid, regardless whether virtual"
> 
> extra space -----------------------------^
> 

Nod.

>> +            " or not (read only, always true)",

Do we need "." here ----------------------------^ ?

>> +            NULL);
>>  }
>>  
>>  static const TypeInfo virtual_css_bridge_info = {
> 
> Looks reasonable. If this works as expected, I'll squash it into the
> previous patch.
> 

I've just asked Shalini to verify the libvirt perspective.

Supposed we verify this works as expected, I read I don't have to spin
a v2 and you are going to fix the issues found yourself. Right?

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Cornelia Huck]

On Mon, 4 Dec 2017 16:07:27 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/04/2017 12:15 PM, Cornelia Huck wrote:
> > On Fri,  1 Dec 2017 15:31:35 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> >> to the management software (so it can tell are cssids unrestricted or
> >> restricted).
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>
> >> Boris says having the property on the virtual-css-bridge is good form
> >> Libvirt PoV. @Shalini: could you verify that things work out fine
> >> (provided we get at least a preliminary blessing from Connie).
> >>
> >> Consider squashing into "s390x/css: unrestrict cssids".
> >> ---
> >>  hw/s390x/css-bridge.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> >> index c4a9735d71..c7e8998680 100644
> >> --- a/hw/s390x/css-bridge.c
> >> +++ b/hw/s390x/css-bridge.c
> >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +    return true;
> >> +}
> >> +
> >>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >>      hc->unplug = ccw_device_unplug;
> >>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >>      dc->props = virtual_css_bridge_properties;
> >> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >> +                                   prop_get_true, NULL, NULL);
> >> +    object_class_property_set_description(klass, "cssid-unrestricted",
> >> +            "A css device can use any  cssid, regardless whether virtual"  
> > 
> > extra space -----------------------------^
> >   
> 
> Nod.
> 
> >> +            " or not (read only, always true)",  
> 
> Do we need "." here ----------------------------^ ?

None of the other descs do that.

> 
> >> +            NULL);
> >>  }
> >>  
> >>  static const TypeInfo virtual_css_bridge_info = {  
> > 
> > Looks reasonable. If this works as expected, I'll squash it into the
> > previous patch.
> >   
> 
> I've just asked Shalini to verify the libvirt perspective.
> 
> Supposed we verify this works as expected, I read I don't have to spin
> a v2 and you are going to fix the issues found yourself. Right?

I'd prefer a v2 that I can simply apply. Let's wait for some more
acks/r-bs.

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Halil Pasic]

On 12/04/2017 05:07 PM, Cornelia Huck wrote:
> On Mon, 4 Dec 2017 16:07:27 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 12/04/2017 12:15 PM, Cornelia Huck wrote:
>>> On Fri,  1 Dec 2017 15:31:35 +0100
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
>>>> to the management software (so it can tell are cssids unrestricted or
>>>> restricted).
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> ---
>>>>
>>>> Boris says having the property on the virtual-css-bridge is good form
>>>> Libvirt PoV. @Shalini: could you verify that things work out fine
>>>> (provided we get at least a preliminary blessing from Connie).
>>>>
>>>> Consider squashing into "s390x/css: unrestrict cssids".
>>>> ---
>>>>  hw/s390x/css-bridge.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>>>> index c4a9735d71..c7e8998680 100644
>>>> --- a/hw/s390x/css-bridge.c
>>>> +++ b/hw/s390x/css-bridge.c
>>>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>  };
>>>>  
>>>> +static bool prop_get_true(Object *obj, Error **errp)
>>>> +{
>>>> +    return true;
>>>> +}
>>>> +
>>>>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>>  {
>>>>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>>      hc->unplug = ccw_device_unplug;
>>>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>>      dc->props = virtual_css_bridge_properties;
>>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>>> +                                   prop_get_true, NULL, NULL);
>>>> +    object_class_property_set_description(klass, "cssid-unrestricted",
>>>> +            "A css device can use any  cssid, regardless whether virtual"  
>>>
>>> extra space -----------------------------^
>>>   
>>
>> Nod.
>>
>>>> +            " or not (read only, always true)",  
>>
>> Do we need "." here ----------------------------^ ?
> 
> None of the other descs do that.
> 
>>
>>>> +            NULL);
>>>>  }
>>>>  
>>>>  static const TypeInfo virtual_css_bridge_info = {  
>>>
>>> Looks reasonable. If this works as expected, I'll squash it into the
>>> previous patch.
>>>   
>>
>> I've just asked Shalini to verify the libvirt perspective.
>>
>> Supposed we verify this works as expected, I read I don't have to spin
>> a v2 and you are going to fix the issues found yourself. Right?
> 
> I'd prefer a v2 that I can simply apply. Let's wait for some more
> acks/r-bs.
> 

Cool with me ;)!

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-12-04 16:07:27 +0100]:

> 
> 
> On 12/04/2017 12:15 PM, Cornelia Huck wrote:
> > On Fri,  1 Dec 2017 15:31:35 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > 
> >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> >> to the management software (so it can tell are cssids unrestricted or
> >> restricted).
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>
> >> Boris says having the property on the virtual-css-bridge is good form
> >> Libvirt PoV. @Shalini: could you verify that things work out fine
> >> (provided we get at least a preliminary blessing from Connie).
> >>
> >> Consider squashing into "s390x/css: unrestrict cssids".
> >> ---
> >>  hw/s390x/css-bridge.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> >> index c4a9735d71..c7e8998680 100644
> >> --- a/hw/s390x/css-bridge.c
> >> +++ b/hw/s390x/css-bridge.c
> >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +    return true;
> >> +}
> >> +
> >>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >>      hc->unplug = ccw_device_unplug;
> >>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >>      dc->props = virtual_css_bridge_properties;
> >> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >> +                                   prop_get_true, NULL, NULL);
> >> +    object_class_property_set_description(klass, "cssid-unrestricted",
> >> +            "A css device can use any  cssid, regardless whether virtual"
> > 
> > extra space -----------------------------^
> > 
> 
> Nod.
> 
> >> +            " or not (read only, always true)",
> 
> Do we need "." here ----------------------------^ ?
> 
> >> +            NULL);
> >>  }
> >>  
> >>  static const TypeInfo virtual_css_bridge_info = {
> > 
> > Looks reasonable. If this works as expected, I'll squash it into the
> > previous patch.
> > 
> 
> I've just asked Shalini to verify the libvirt perspective.
> 
> Supposed we verify this works as expected, I read I don't have to spin
> a v2 and you are going to fix the issues found yourself. Right?
Supposed this works as expected, you have my r-b. ;)

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Shalini Chellathurai Saroja]

On 12/04/2017 04:07 PM, Halil Pasic wrote:
>
> On 12/04/2017 12:15 PM, Cornelia Huck wrote:
>> On Fri,  1 Dec 2017 15:31:35 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
>>> to the management software (so it can tell are cssids unrestricted or
>>> restricted).
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>
>>> Boris says having the property on the virtual-css-bridge is good form
>>> Libvirt PoV. @Shalini: could you verify that things work out fine
>>> (provided we get at least a preliminary blessing from Connie).
>>>
>>> Consider squashing into "s390x/css: unrestrict cssids".
>>> ---
>>>   hw/s390x/css-bridge.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>>> index c4a9735d71..c7e8998680 100644
>>> --- a/hw/s390x/css-bridge.c
>>> +++ b/hw/s390x/css-bridge.c
>>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>   
>>> +static bool prop_get_true(Object *obj, Error **errp)
>>> +{
>>> +    return true;
>>> +}
>>> +
>>>   static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>   {
>>>       HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>       hc->unplug = ccw_device_unplug;
>>>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>       dc->props = virtual_css_bridge_properties;
>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>> +                                   prop_get_true, NULL, NULL);
>>> +    object_class_property_set_description(klass, "cssid-unrestricted",
>>> +            "A css device can use any  cssid, regardless whether virtual"
>> extra space -----------------------------^
>>
> Nod.
>
>>> +            " or not (read only, always true)",
> Do we need "." here ----------------------------^ ?
>
>>> +            NULL);
>>>   }
>>>   
>>>   static const TypeInfo virtual_css_bridge_info = {
>> Looks reasonable. If this works as expected, I'll squash it into the
>> previous patch.
>>
> I've just asked Shalini to verify the libvirt perspective.

Verified. The patch works as expected.

>
> Supposed we verify this works as expected, I read I don't have to spin
> a v2 and you are going to fix the issues found yourself. Right?

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Cornelia Huck]

On Tue, 5 Dec 2017 18:28:47 +0100
Shalini Chellathurai Saroja <shalini@linux.vnet.ibm.com> wrote:

> On 12/04/2017 04:07 PM, Halil Pasic wrote:
> >
> > On 12/04/2017 12:15 PM, Cornelia Huck wrote:  

> >> Looks reasonable. If this works as expected, I'll squash it into the
> >> previous patch.
> >>  
> > I've just asked Shalini to verify the libvirt perspective.  
> 
> Verified. The patch works as expected.

Cool, thanks! It seems we have a winner here.

Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Halil Pasic]

On 12/05/2017 06:28 PM, Shalini Chellathurai Saroja wrote:
> 
> 
> On 12/04/2017 04:07 PM, Halil Pasic wrote:
>>
>> On 12/04/2017 12:15 PM, Cornelia Huck wrote:
>>> On Fri,  1 Dec 2017 15:31:35 +0100
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>
>>>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
>>>> to the management software (so it can tell are cssids unrestricted or
>>>> restricted).
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> ---
>>>>
>>>> Boris says having the property on the virtual-css-bridge is good form
>>>> Libvirt PoV. @Shalini: could you verify that things work out fine
>>>> (provided we get at least a preliminary blessing from Connie).
>>>>
>>>> Consider squashing into "s390x/css: unrestrict cssids".
>>>> ---
>>>>   hw/s390x/css-bridge.c | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>>>> index c4a9735d71..c7e8998680 100644
>>>> --- a/hw/s390x/css-bridge.c
>>>> +++ b/hw/s390x/css-bridge.c
>>>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>>>>       DEFINE_PROP_END_OF_LIST(),
>>>>   };
>>>>   +static bool prop_get_true(Object *obj, Error **errp)
>>>> +{
>>>> +    return true;
>>>> +}
>>>> +
>>>>   static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>>   {
>>>>       HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>>       hc->unplug = ccw_device_unplug;
>>>>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>>       dc->props = virtual_css_bridge_properties;
>>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>>> +                                   prop_get_true, NULL, NULL);
>>>> +    object_class_property_set_description(klass, "cssid-unrestricted",
>>>> +            "A css device can use any  cssid, regardless whether virtual"
>>> extra space -----------------------------^
>>>
>> Nod.
>>
>>>> +            " or not (read only, always true)",
>> Do we need "." here ----------------------------^ ?
>>
>>>> +            NULL);
>>>>   }
>>>>     static const TypeInfo virtual_css_bridge_info = {
>>> Looks reasonable. If this works as expected, I'll squash it into the
>>> previous patch.
>>>
>> I've just asked Shalini to verify the libvirt perspective.
> 
> Verified. The patch works as expected.
> 

Many thanks! I intend to address the remaining issues of the series and
spin a v2 today.

Halil

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Thomas Huth]

On 01.12.2017 15:31, Halil Pasic wrote:
> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> to the management software (so it can tell are cssids unrestricted or
> restricted).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> Boris says having the property on the virtual-css-bridge is good form
> Libvirt PoV. @Shalini: could you verify that things work out fine
> (provided we get at least a preliminary blessing from Connie).
> 
> Consider squashing into "s390x/css: unrestrict cssids".
> ---
>  hw/s390x/css-bridge.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index c4a9735d71..c7e8998680 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static bool prop_get_true(Object *obj, Error **errp)
> +{
> +    return true;
> +}
> +
>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>  {
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>      hc->unplug = ccw_device_unplug;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->props = virtual_css_bridge_properties;
> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> +                                   prop_get_true, NULL, NULL);
> +    object_class_property_set_description(klass, "cssid-unrestricted",
> +            "A css device can use any  cssid, regardless whether virtual"
> +            " or not (read only, always true)",

I'd maybe remove the "always true" in the description here, since that
might create wrong assumptions with regards to future versions or lead
to bad code in the upper layers. If someone reads "always true", they
simply might omit the check of the value of this property. If we then
ever want to change it to "false" again, we're in trouble (sure, we
could simply completely remove the property again, but we have to
remember to do that instead of setting it to false ... so let's better
play safe right now already, ok?)

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Halil Pasic]

On 12/05/2017 09:28 AM, Thomas Huth wrote:
> On 01.12.2017 15:31, Halil Pasic wrote:
>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
>> to the management software (so it can tell are cssids unrestricted or
>> restricted).
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> Boris says having the property on the virtual-css-bridge is good form
>> Libvirt PoV. @Shalini: could you verify that things work out fine
>> (provided we get at least a preliminary blessing from Connie).
>>
>> Consider squashing into "s390x/css: unrestrict cssids".
>> ---
>>  hw/s390x/css-bridge.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>> index c4a9735d71..c7e8998680 100644
>> --- a/hw/s390x/css-bridge.c
>> +++ b/hw/s390x/css-bridge.c
>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> +static bool prop_get_true(Object *obj, Error **errp)
>> +{
>> +    return true;
>> +}
>> +
>>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>  {
>>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>      hc->unplug = ccw_device_unplug;
>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>      dc->props = virtual_css_bridge_properties;
>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>> +                                   prop_get_true, NULL, NULL);
>> +    object_class_property_set_description(klass, "cssid-unrestricted",
>> +            "A css device can use any  cssid, regardless whether virtual"
>> +            " or not (read only, always true)",
> 
> I'd maybe remove the "always true" in the description here, since that
> might create wrong assumptions with regards to future versions or lead
> to bad code in the upper layers. If someone reads "always true", they
> simply might omit the check of the value of this property. If we then
> ever want to change it to "false" again, we're in trouble (sure, we
> could simply completely remove the property again, but we have to
> remember to do that instead of setting it to false ... so let's better
> play safe right now already, ok?)
> 
>  Thomas
> 

Libvirt intends to check for the existence of the property and ignore
it's value. I've been told in Libvirt capabilities are usually tied
to existence. For inspecting the value one would have to work on an
instance. I don't think that would work with Libvirt's capability
probing scheme well.

So what you describe is basically like intended. I was not to document
always true though, but then Connie had a version with always true
and I got convinced it ain't a bad idea. If both Connie and you agree
that 'always true' is to be dropped I'm fine with dropping it.

Thanks for the review!

Halil

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Cornelia Huck]

On Tue, 5 Dec 2017 11:08:18 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/05/2017 09:28 AM, Thomas Huth wrote:
> > On 01.12.2017 15:31, Halil Pasic wrote:  
> >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> >> to the management software (so it can tell are cssids unrestricted or
> >> restricted).
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>
> >> Boris says having the property on the virtual-css-bridge is good form
> >> Libvirt PoV. @Shalini: could you verify that things work out fine
> >> (provided we get at least a preliminary blessing from Connie).
> >>
> >> Consider squashing into "s390x/css: unrestrict cssids".
> >> ---
> >>  hw/s390x/css-bridge.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> >> index c4a9735d71..c7e8998680 100644
> >> --- a/hw/s390x/css-bridge.c
> >> +++ b/hw/s390x/css-bridge.c
> >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +    return true;
> >> +}
> >> +
> >>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >>      hc->unplug = ccw_device_unplug;
> >>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >>      dc->props = virtual_css_bridge_properties;
> >> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >> +                                   prop_get_true, NULL, NULL);
> >> +    object_class_property_set_description(klass, "cssid-unrestricted",
> >> +            "A css device can use any  cssid, regardless whether virtual"
> >> +            " or not (read only, always true)",  
> > 
> > I'd maybe remove the "always true" in the description here, since that
> > might create wrong assumptions with regards to future versions or lead
> > to bad code in the upper layers. If someone reads "always true", they
> > simply might omit the check of the value of this property. If we then
> > ever want to change it to "false" again, we're in trouble (sure, we
> > could simply completely remove the property again, but we have to
> > remember to do that instead of setting it to false ... so let's better
> > play safe right now already, ok?)
> > 
> >  Thomas
> >   
> 
> Libvirt intends to check for the existence of the property and ignore
> it's value. I've been told in Libvirt capabilities are usually tied
> to existence. For inspecting the value one would have to work on an
> instance. I don't think that would work with Libvirt's capability
> probing scheme well.
> 
> So what you describe is basically like intended. I was not to document
> always true though, but then Connie had a version with always true
> and I got convinced it ain't a bad idea. If both Connie and you agree
> that 'always true' is to be dropped I'm fine with dropping it.

I highly doubt that we will want to switch it to false again,
especially considering libvirt usage. So I'd prefer to document this.

Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/css: advertise unrestricted cssids

[Thomas Huth]

On 05.12.2017 11:08, Halil Pasic wrote:
> 
> 
> On 12/05/2017 09:28 AM, Thomas Huth wrote:
>> On 01.12.2017 15:31, Halil Pasic wrote:
>>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
>>> to the management software (so it can tell are cssids unrestricted or
>>> restricted).
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>
>>> Boris says having the property on the virtual-css-bridge is good form
>>> Libvirt PoV. @Shalini: could you verify that things work out fine
>>> (provided we get at least a preliminary blessing from Connie).
>>>
>>> Consider squashing into "s390x/css: unrestrict cssids".
>>> ---
>>>  hw/s390x/css-bridge.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>>> index c4a9735d71..c7e8998680 100644
>>> --- a/hw/s390x/css-bridge.c
>>> +++ b/hw/s390x/css-bridge.c
>>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>  
>>> +static bool prop_get_true(Object *obj, Error **errp)
>>> +{
>>> +    return true;
>>> +}
>>> +
>>>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>>>      hc->unplug = ccw_device_unplug;
>>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>      dc->props = virtual_css_bridge_properties;
>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>> +                                   prop_get_true, NULL, NULL);
>>> +    object_class_property_set_description(klass, "cssid-unrestricted",
>>> +            "A css device can use any  cssid, regardless whether virtual"
>>> +            " or not (read only, always true)",
>>
>> I'd maybe remove the "always true" in the description here, since that
>> might create wrong assumptions with regards to future versions or lead
>> to bad code in the upper layers. If someone reads "always true", they
>> simply might omit the check of the value of this property. If we then
>> ever want to change it to "false" again, we're in trouble (sure, we
>> could simply completely remove the property again, but we have to
>> remember to do that instead of setting it to false ... so let's better
>> play safe right now already, ok?)
>>
>>  Thomas
>>
> 
> Libvirt intends to check for the existence of the property and ignore
> it's value.
OK, I wasn't aware of the fact that libvirt normally only checks for the
presence, but not for the value. Then yes, please keep the "always true"
in the comment.

 Thomas

[Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Halil Pasic]

With the cssids unrestricted (commit <COMMIT HERE> "s390x/css: unrestrict
cssids") the s390-squash-mcss machine property should not be used.
Actually libvirt never supported this, so the expectation is that
removing it should be pretty painless.  But let's play nice and deprecate
it first.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---

I would like to reference a commit for "s390x/css: unrestrict cssids"
which is currently in RFC status on the list.
---
 hw/s390x/s390-virtio-ccw.c | 6 +++++-
 qemu-doc.texi              | 8 ++++++++
 qemu-options.hx            | 8 +++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4d65a50334..3796f666e6 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -553,6 +553,10 @@ static inline void machine_set_squash_mcss(Object *obj, bool value,
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
 
     ms->s390_squash_mcss = value;
+    if (ms->s390_squash_mcss) {
+        warn_report("The machine property 's390-squash-mcss' is deprecated"
+                    " (obsoleted by lifting the cssid restrictions).");
+    }
 }
 
 static inline void s390_machine_initfn(Object *obj)
@@ -582,7 +586,7 @@ static inline void s390_machine_initfn(Object *obj)
     object_property_add_bool(obj, "s390-squash-mcss",
                              machine_get_squash_mcss,
                              machine_set_squash_mcss, NULL);
-    object_property_set_description(obj, "s390-squash-mcss",
+    object_property_set_description(obj, "s390-squash-mcss", "(deprecated) "
             "enable/disable squashing subchannels into the default css",
             NULL);
     object_property_set_bool(obj, false, "s390-squash-mcss", NULL);
diff --git a/qemu-doc.texi b/qemu-doc.texi
index db2351c746..874432d87c 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2501,6 +2501,14 @@ enabled via the ``-machine usb=on'' argument.
 
 The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
 
+@subsection -machine virtio-ccw,s390-squash-mcss=on|off (since 2.12.0)
+
+The ``s390-squash-mcss=on`` property has been obsoleted by allowing the
+cssid to be chosen freely. Instead of squashing subchannels into the
+default channel subsystem image for guests that do not support multiple
+channel subsystems, all devices can be put into the default channel
+subsystem image.
+
 @section qemu-img command line arguments
 
 @subsection convert -s (since 2.0.0)
diff --git a/qemu-options.hx b/qemu-options.hx
index f11c4ac960..fe0c29271f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -43,7 +43,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
     "                nvdimm=on|off controls NVDIMM support (default=off)\n"
     "                enforce-config-section=on|off enforce configuration section migration (default=off)\n"
-    "                s390-squash-mcss=on|off controls support for squashing into default css (default=off)\n",
+    "                s390-squash-mcss=on|off (deprecated) controls support for squashing into default css (default=off)\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
@@ -98,6 +98,12 @@ Enables or disables NVDIMM support. The default is off.
 @item s390-squash-mcss=on|off
 Enables or disables squashing subchannels into the default css.
 The default is off.
+NOTE: This property is deprecated and will be removed in future releases.
+The ``s390-squash-mcss=on`` property has been obsoleted by allowing the
+cssid to be chosen freely. Instead of squashing subchannels into the
+default channel subsystem image for guests that do not support multiple
+channel subsystems, all devices can be put into the default channel
+subsystem image.
 @item enforce-config-section=on|off
 If @option{enforce-config-section} is set to @var{on}, force migration
 code to send configuration section even if the machine-type sets the
-- 
2.13.5

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Cornelia Huck]

On Fri,  1 Dec 2017 15:31:36 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> With the cssids unrestricted (commit <COMMIT HERE> "s390x/css: unrestrict

I won't add the commit id, as I intend to merge this in one go and I
don't want to edit this every time I rebase prior to pull.

> cssids") the s390-squash-mcss machine property should not be used.
> Actually libvirt never supported this, so the expectation is that
> removing it should be pretty painless.  But let's play nice and deprecate
> it first.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> I would like to reference a commit for "s390x/css: unrestrict cssids"
> which is currently in RFC status on the list.
> ---
>  hw/s390x/s390-virtio-ccw.c | 6 +++++-
>  qemu-doc.texi              | 8 ++++++++
>  qemu-options.hx            | 8 +++++++-
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4d65a50334..3796f666e6 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -553,6 +553,10 @@ static inline void machine_set_squash_mcss(Object *obj, bool value,
>      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>  
>      ms->s390_squash_mcss = value;
> +    if (ms->s390_squash_mcss) {
> +        warn_report("The machine property 's390-squash-mcss' is deprecated"
> +                    " (obsoleted by lifting the cssid restrictions).");

Too bad that we can't warn when this is explicitly set to false - OTOH
I don't really expect anyone doing that.

> +    }
>  }
>  
>  static inline void s390_machine_initfn(Object *obj)
> @@ -582,7 +586,7 @@ static inline void s390_machine_initfn(Object *obj)
>      object_property_add_bool(obj, "s390-squash-mcss",
>                               machine_get_squash_mcss,
>                               machine_set_squash_mcss, NULL);
> -    object_property_set_description(obj, "s390-squash-mcss",
> +    object_property_set_description(obj, "s390-squash-mcss", "(deprecated) "
>              "enable/disable squashing subchannels into the default css",
>              NULL);
>      object_property_set_bool(obj, false, "s390-squash-mcss", NULL);
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index db2351c746..874432d87c 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2501,6 +2501,14 @@ enabled via the ``-machine usb=on'' argument.
>  
>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>  
> +@subsection -machine virtio-ccw,s390-squash-mcss=on|off (since 2.12.0)
> +
> +The ``s390-squash-mcss=on`` property has been obsoleted by allowing the
> +cssid to be chosen freely. Instead of squashing subchannels into the
> +default channel subsystem image for guests that do not support multiple
> +channel subsystems, all devices can be put into the default channel
> +subsystem image.
> +
>  @section qemu-img command line arguments
>  
>  @subsection convert -s (since 2.0.0)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index f11c4ac960..fe0c29271f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -43,7 +43,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>      "                nvdimm=on|off controls NVDIMM support (default=off)\n"
>      "                enforce-config-section=on|off enforce configuration section migration (default=off)\n"
> -    "                s390-squash-mcss=on|off controls support for squashing into default css (default=off)\n",
> +    "                s390-squash-mcss=on|off (deprecated) controls support for squashing into default css (default=off)\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
> @@ -98,6 +98,12 @@ Enables or disables NVDIMM support. The default is off.
>  @item s390-squash-mcss=on|off
>  Enables or disables squashing subchannels into the default css.
>  The default is off.
> +NOTE: This property is deprecated and will be removed in future releases.
> +The ``s390-squash-mcss=on`` property has been obsoleted by allowing the
> +cssid to be chosen freely. Instead of squashing subchannels into the
> +default channel subsystem image for guests that do not support multiple
> +channel subsystems, all devices can be put into the default channel
> +subsystem image.
>  @item enforce-config-section=on|off
>  If @option{enforce-config-section} is set to @var{on}, force migration
>  code to send configuration section even if the machine-type sets the

Looks sane. We should put a note into the 2.12 changelog as well.

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Halil Pasic]

On 12/04/2017 12:28 PM, Cornelia Huck wrote:
> On Fri,  1 Dec 2017 15:31:36 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> With the cssids unrestricted (commit <COMMIT HERE> "s390x/css: unrestrict
> 
> I won't add the commit id, as I intend to merge this in one go and I
> don't want to edit this every time I rebase prior to pull.
> 

I'm fine with dropping the commit id.

>> cssids") the s390-squash-mcss machine property should not be used.
>> Actually libvirt never supported this, so the expectation is that
>> removing it should be pretty painless.  But let's play nice and deprecate
>> it first.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> I would like to reference a commit for "s390x/css: unrestrict cssids"
>> which is currently in RFC status on the list.
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 6 +++++-
>>  qemu-doc.texi              | 8 ++++++++
>>  qemu-options.hx            | 8 +++++++-
>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 4d65a50334..3796f666e6 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -553,6 +553,10 @@ static inline void machine_set_squash_mcss(Object *obj, bool value,
>>      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>>  
>>      ms->s390_squash_mcss = value;
>> +    if (ms->s390_squash_mcss) {
>> +        warn_report("The machine property 's390-squash-mcss' is deprecated"
>> +                    " (obsoleted by lifting the cssid restrictions).");
> 
> Too bad that we can't warn when this is explicitly set to false - OTOH
> I don't really expect anyone doing that.
> 

I did not really dig deep. It may be possible, but I had other priorities,
and was not sure if it's worth (as I also don't expect it being set to false
explicit in the wild).

>> +    }
>>  }
>>  
>>  static inline void s390_machine_initfn(Object *obj)
>> @@ -582,7 +586,7 @@ static inline void s390_machine_initfn(Object *obj)
>>      object_property_add_bool(obj, "s390-squash-mcss",
>>                               machine_get_squash_mcss,
>>                               machine_set_squash_mcss, NULL);
>> -    object_property_set_description(obj, "s390-squash-mcss",
>> +    object_property_set_description(obj, "s390-squash-mcss", "(deprecated) "
>>              "enable/disable squashing subchannels into the default css",
>>              NULL);
>>      object_property_set_bool(obj, false, "s390-squash-mcss", NULL);
>> diff --git a/qemu-doc.texi b/qemu-doc.texi
>> index db2351c746..874432d87c 100644
>> --- a/qemu-doc.texi
>> +++ b/qemu-doc.texi
>> @@ -2501,6 +2501,14 @@ enabled via the ``-machine usb=on'' argument.
>>  
>>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>>  
>> +@subsection -machine virtio-ccw,s390-squash-mcss=on|off (since 2.12.0)
>> +
>> +The ``s390-squash-mcss=on`` property has been obsoleted by allowing the
>> +cssid to be chosen freely. Instead of squashing subchannels into the
>> +default channel subsystem image for guests that do not support multiple
>> +channel subsystems, all devices can be put into the default channel
>> +subsystem image.
>> +
>>  @section qemu-img command line arguments
>>  
>>  @subsection convert -s (since 2.0.0)
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index f11c4ac960..fe0c29271f 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -43,7 +43,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>>      "                nvdimm=on|off controls NVDIMM support (default=off)\n"
>>      "                enforce-config-section=on|off enforce configuration section migration (default=off)\n"
>> -    "                s390-squash-mcss=on|off controls support for squashing into default css (default=off)\n",
>> +    "                s390-squash-mcss=on|off (deprecated) controls support for squashing into default css (default=off)\n",
>>      QEMU_ARCH_ALL)
>>  STEXI
>>  @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>> @@ -98,6 +98,12 @@ Enables or disables NVDIMM support. The default is off.
>>  @item s390-squash-mcss=on|off
>>  Enables or disables squashing subchannels into the default css.
>>  The default is off.
>> +NOTE: This property is deprecated and will be removed in future releases.
>> +The ``s390-squash-mcss=on`` property has been obsoleted by allowing the
>> +cssid to be chosen freely. Instead of squashing subchannels into the
>> +default channel subsystem image for guests that do not support multiple
>> +channel subsystems, all devices can be put into the default channel
>> +subsystem image.
>>  @item enforce-config-section=on|off
>>  If @option{enforce-config-section} is set to @var{on}, force migration
>>  code to send configuration section even if the machine-type sets the
> 
> Looks sane. We should put a note into the 2.12 changelog as well.
> 

I agree. Who would be responsible for updating the changelog. I'm not
familiar with that process yet. To be honest, I wouldn't mind having
the changelog notice in your writing style. Guess would be better for
everyone ;).

There are also other things we identified as TODOs:
* Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough
(@Dong Jia, could you take this one)
* In tree and/or on wiki documentation which is up-to-date and
more verbose than commit messages are supposed to be (and Dong
Jia's write-up could be incorporated to). I see this one as
lower prio though. Any volunteers?

Many thanks for the guidance!

Halil

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Cornelia Huck]

On Mon, 4 Dec 2017 16:32:16 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/04/2017 12:28 PM, Cornelia Huck wrote:
> > On Fri,  1 Dec 2017 15:31:36 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 4d65a50334..3796f666e6 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -553,6 +553,10 @@ static inline void machine_set_squash_mcss(Object *obj, bool value,
> >>      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> >>  
> >>      ms->s390_squash_mcss = value;
> >> +    if (ms->s390_squash_mcss) {
> >> +        warn_report("The machine property 's390-squash-mcss' is deprecated"
> >> +                    " (obsoleted by lifting the cssid restrictions).");  
> > 
> > Too bad that we can't warn when this is explicitly set to false - OTOH
> > I don't really expect anyone doing that.
> >   
> 
> I did not really dig deep. It may be possible, but I had other priorities,
> and was not sure if it's worth (as I also don't expect it being set to false
> explicit in the wild).

It's not worth spending any more time on this.

> 
> >> +    }
> >>  }
> >>  
> >>  static inline void s390_machine_initfn(Object *obj)

(...)

> > Looks sane. We should put a note into the 2.12 changelog as well.
> >   
> 
> I agree. Who would be responsible for updating the changelog. I'm not
> familiar with that process yet. To be honest, I wouldn't mind having
> the changelog notice in your writing style. Guess would be better for
> everyone ;).

Just a one-liner once we have the 2.12 changelog page.

> 
> There are also other things we identified as TODOs:
> * Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough
> (@Dong Jia, could you take this one)
> * In tree and/or on wiki documentation which is up-to-date and
> more verbose than commit messages are supposed to be (and Dong
> Jia's write-up could be incorporated to). I see this one as
> lower prio though. Any volunteers?

Long-ish things with links etc. should probably go into the wiki.

Anyway, feel free to go ahead (it's a wiki :) We can always change
things later on.

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-12-04 17:11:24 +0100]:

[...]

This one looks good to me too, so:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

> 
> (...)
> 
> > > Looks sane. We should put a note into the 2.12 changelog as well.
> > >   
> > 
> > I agree. Who would be responsible for updating the changelog. I'm not
> > familiar with that process yet. To be honest, I wouldn't mind having
> > the changelog notice in your writing style. Guess would be better for
> > everyone ;).
> 
> Just a one-liner once we have the 2.12 changelog page.
> 
> > 
> > There are also other things we identified as TODOs:
> > * Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough
> > (@Dong Jia, could you take this one)
Ok. Will do soon.

> > * In tree and/or on wiki documentation which is up-to-date and
> > more verbose than commit messages are supposed to be (and Dong
> > Jia's write-up could be incorporated to). I see this one as
> > lower prio though. Any volunteers?
> 
> Long-ish things with links etc. should probably go into the wiki.
> 
> Anyway, feel free to go ahead (it's a wiki :) We can always change
> things later on.
> 
Let me update the wiki based on my understanding (and style :) firstly,
then you can login and improve it as you wish.

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Dong Jia Shi]

* Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-12-05 15:43:00 +0800]:

> * Cornelia Huck <cohuck@redhat.com> [2017-12-04 17:11:24 +0100]:
> 
> [...]
> 
> This one looks good to me too, so:
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 
BTW, since we are deprecating s390-squash-mcss, I think any more
improment on it (e.g. more accurate and helpful hint message) is not
worthy. So I will not send such pathes in future, if nobody disagrees.

[...]

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Halil Pasic]

On 12/05/2017 08:48 AM, Dong Jia Shi wrote:
> * Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-12-05 15:43:00 +0800]:
> 
>> * Cornelia Huck <cohuck@redhat.com> [2017-12-04 17:11:24 +0100]:
>>
>> [...]
>>
>> This one looks good to me too, so:
>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>
> BTW, since we are deprecating s390-squash-mcss, I think any more
> improment on it (e.g. more accurate and helpful hint message) is not
> worthy. So I will not send such pathes in future, if nobody disagrees.
> 

Makes sense to me.

Halil

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Dong Jia Shi]

* Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-12-05 15:43:00 +0800]:

> * Cornelia Huck <cohuck@redhat.com> [2017-12-04 17:11:24 +0100]:
> 
> [...]
> 
> This one looks good to me too, so:
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 
> > 
> > (...)
> > 
> > > > Looks sane. We should put a note into the 2.12 changelog as well.
> > > >   
> > > 
> > > I agree. Who would be responsible for updating the changelog. I'm not
> > > familiar with that process yet. To be honest, I wouldn't mind having
> > > the changelog notice in your writing style. Guess would be better for
> > > everyone ;).
> > 
> > Just a one-liner once we have the 2.12 changelog page.
> > 
> > > 
> > > There are also other things we identified as TODOs:
> > > * Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough
> > > (@Dong Jia, could you take this one)
> Ok. Will do soon.
> 
> > > * In tree and/or on wiki documentation which is up-to-date and
> > > more verbose than commit messages are supposed to be (and Dong
> > > Jia's write-up could be incorporated to). I see this one as
> > > lower prio though. Any volunteers?
> > 
> > Long-ish things with links etc. should probably go into the wiki.
> > 
> > Anyway, feel free to go ahead (it's a wiki :) We can always change
> > things later on.
> > 
> Let me update the wiki based on my understanding (and style :) firstly,
> then you can login and improve it as you wish.
The following wiki is just updated. Please feel free to make additional
correction:
https://wiki.qemu.org/Features/Channel_I/O_Passthrough

BTW, Conny mentioned before that we should also update this one:
https://wiki.qemu.org/Features/LegacyRemoval

Should it be done now?

@Halil, if you would like to do the update operation, I will leave it to
you. If you want to delegate, I can do it.

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Cornelia Huck]

On Tue, 12 Dec 2017 22:05:24 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-12-05 15:43:00 +0800]:
> 
> > * Cornelia Huck <cohuck@redhat.com> [2017-12-04 17:11:24 +0100]:
> > 
> > [...]
> > 
> > This one looks good to me too, so:
> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> >   
> > > 
> > > (...)
> > >   
> > > > > Looks sane. We should put a note into the 2.12 changelog as well.
> > > > >     
> > > > 
> > > > I agree. Who would be responsible for updating the changelog. I'm not
> > > > familiar with that process yet. To be honest, I wouldn't mind having
> > > > the changelog notice in your writing style. Guess would be better for
> > > > everyone ;).  
> > > 
> > > Just a one-liner once we have the 2.12 changelog page.
> > >   
> > > > 
> > > > There are also other things we identified as TODOs:
> > > > * Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough
> > > > (@Dong Jia, could you take this one)  
> > Ok. Will do soon.
> >   
> > > > * In tree and/or on wiki documentation which is up-to-date and
> > > > more verbose than commit messages are supposed to be (and Dong
> > > > Jia's write-up could be incorporated to). I see this one as
> > > > lower prio though. Any volunteers?  
> > > 
> > > Long-ish things with links etc. should probably go into the wiki.
> > > 
> > > Anyway, feel free to go ahead (it's a wiki :) We can always change
> > > things later on.
> > >   
> > Let me update the wiki based on my understanding (and style :) firstly,
> > then you can login and improve it as you wish.  
> The following wiki is just updated. Please feel free to make additional
> correction:
> https://wiki.qemu.org/Features/Channel_I/O_Passthrough

Looks good, fixed minor typos.

> 
> BTW, Conny mentioned before that we should also update this one:
> https://wiki.qemu.org/Features/LegacyRemoval
> 
> Should it be done now?

Already done :)

Re: [Qemu-devel] [qemu-s390x] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Thomas Huth]

On 01.12.2017 15:31, Halil Pasic wrote:
> With the cssids unrestricted (commit <COMMIT HERE> "s390x/css: unrestrict
> cssids") the s390-squash-mcss machine property should not be used.
> Actually libvirt never supported this, so the expectation is that
> removing it should be pretty painless.  But let's play nice and deprecate
> it first.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
[...]
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index db2351c746..874432d87c 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2501,6 +2501,14 @@ enabled via the ``-machine usb=on'' argument.
>  
>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>  
> +@subsection -machine virtio-ccw,s390-squash-mcss=on|off (since 2.12.0)

The machine is called "s390-ccw-virtio", not "virtio-ccw". Anyway, I
think you could also rather omit the machine name here (since it is the
default) and just write "-machine s390-squash-mcss=on|off".

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Halil Pasic]

On 12/05/2017 09:41 AM, Thomas Huth wrote:
> On 01.12.2017 15:31, Halil Pasic wrote:
>> With the cssids unrestricted (commit <COMMIT HERE> "s390x/css: unrestrict
>> cssids") the s390-squash-mcss machine property should not be used.
>> Actually libvirt never supported this, so the expectation is that
>> removing it should be pretty painless.  But let's play nice and deprecate
>> it first.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
> [...]
>> diff --git a/qemu-doc.texi b/qemu-doc.texi
>> index db2351c746..874432d87c 100644
>> --- a/qemu-doc.texi
>> +++ b/qemu-doc.texi
>> @@ -2501,6 +2501,14 @@ enabled via the ``-machine usb=on'' argument.
>>  
>>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>>  
>> +@subsection -machine virtio-ccw,s390-squash-mcss=on|off (since 2.12.0)
> 
> The machine is called "s390-ccw-virtio", not "virtio-ccw". Anyway, I
> think you could also rather omit the machine name here (since it is the
> default) and just write "-machine s390-squash-mcss=on|off".
> 
>  Thomas
> 

Good catch! Regarding omitting the machine name, I was surprised that
it works, but it does. Our documentation is a bit strange on this: we
document the machine name as non-optional:
"-machine [type=]name[,prop=value[,...]]"
but then use -machine prop=value in the deprecation section (that's
with no name).

Since the documentation doesn't make it clear that the property is
existent only for the s390-ccw-virtio family of the machines I think
being your suggestion has a lot of charm. @Connie: do you agree to
removing the machine name (type)?

Thanks!
Halil

Re: [Qemu-devel] [qemu-s390x] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Cornelia Huck]

On Tue, 5 Dec 2017 13:05:11 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/05/2017 09:41 AM, Thomas Huth wrote:
> > On 01.12.2017 15:31, Halil Pasic wrote:  
> >> With the cssids unrestricted (commit <COMMIT HERE> "s390x/css: unrestrict
> >> cssids") the s390-squash-mcss machine property should not be used.
> >> Actually libvirt never supported this, so the expectation is that
> >> removing it should be pretty painless.  But let's play nice and deprecate
> >> it first.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---  
> > [...]  
> >> diff --git a/qemu-doc.texi b/qemu-doc.texi
> >> index db2351c746..874432d87c 100644
> >> --- a/qemu-doc.texi
> >> +++ b/qemu-doc.texi
> >> @@ -2501,6 +2501,14 @@ enabled via the ``-machine usb=on'' argument.
> >>  
> >>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
> >>  
> >> +@subsection -machine virtio-ccw,s390-squash-mcss=on|off (since 2.12.0)  
> > 
> > The machine is called "s390-ccw-virtio", not "virtio-ccw". Anyway, I
> > think you could also rather omit the machine name here (since it is the
> > default) and just write "-machine s390-squash-mcss=on|off".
> > 
> >  Thomas
> >   
> 
> Good catch! Regarding omitting the machine name, I was surprised that
> it works, but it does. Our documentation is a bit strange on this: we
> document the machine name as non-optional:
> "-machine [type=]name[,prop=value[,...]]"
> but then use -machine prop=value in the deprecation section (that's
> with no name).
> 
> Since the documentation doesn't make it clear that the property is
> existent only for the s390-ccw-virtio family of the machines I think
> being your suggestion has a lot of charm. @Connie: do you agree to
> removing the machine name (type)?

Let's drop the machine name.

Re: [Qemu-devel] [qemu-s390x] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On 12/05/2017 06:05 AM, Halil Pasic wrote:

> Good catch! Regarding omitting the machine name, I was surprised that
> it works, but it does. Our documentation is a bit strange on this: we
> document the machine name as non-optional:
> "-machine [type=]name[,prop=value[,...]]"
> but then use -machine prop=value in the deprecation section (that's
> with no name).

QemuOpts at their "finest".  If it sees the first hunk as a value
without an =, then that value is fed to type=name; but you are also free
to make the first hunk be an explicit name=value (whether type=name or
prop=value), at which point it is up to the backend whether type can be
omitted.  So in this case, '-machine prop=value' manages to work just
fine, and it's a question of how you would convey that information in
the --help documentation.

> 
> Since the documentation doesn't make it clear that the property is
> existent only for the s390-ccw-virtio family of the machines I think
> being your suggestion has a lot of charm. @Connie: do you agree to
> removing the machine name (type)?
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [qemu-s390x] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]



On 12/05/2017 03:54 PM, Eric Blake wrote:
> On 12/05/2017 06:05 AM, Halil Pasic wrote:
> 
>> Good catch! Regarding omitting the machine name, I was surprised that
>> it works, but it does. Our documentation is a bit strange on this: we
>> document the machine name as non-optional:
>> "-machine [type=]name[,prop=value[,...]]"
>> but then use -machine prop=value in the deprecation section (that's
>> with no name).
> 
> QemuOpts at their "finest".  If it sees the first hunk as a value
> without an =, then that value is fed to type=name; but you are also free
> to make the first hunk be an explicit name=value (whether type=name or
> prop=value), at which point it is up to the backend whether type can be
> omitted.  So in this case, '-machine prop=value' manages to work just
> fine, and it's a question of how you would convey that information in
> the --help documentation.
> 

I would have spontaneously said something like 
-machine [[type=]name | prop=value][,prop=value,[,...]]

It is kind of complicated and it does not precisely reflect what is possible
(one can specify the machine multiple times, and it does not have to be
a prefix of the list for example) but that ain't necessarily a problem.

I'm not saying we should change this. I just wanted to say: wow, this was
new to me.

>>
>> Since the documentation doesn't make it clear that the property is
>> existent only for the s390-ccw-virtio family of the machines I think
>> being your suggestion has a lot of charm. @Connie: do you agree to
>> removing the machine name (type)?
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]