[Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict 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 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), and inform management software property on all ccw
devices.

The adverse effect on migration should not be too severe as
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.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

---
Hi!

About indicating this at the ccw devices instead of, e.g. as a machine
property (or otherwise globally), was requested by our libvirt guys. I
have no strong opinion regarding in this matter.

This patch is intended as a discussion starter. I would at least like to
get a Tested-by by Shalini before promoting it to non-RFC (provided the
discussion goes well).

TODOs:
* Consider adding a description for the new property.
* Consider renaming VIRTUAL_CSSID.
* Consider changing the bus-id generation scheme (when
devno is not specified by the user). his patch keep the current scheme in
place: they won't go into the default css (but slots are filled up
starting at cssid 0). This is theoretically good for migration
compatibility same command line same addresses.  Practically since there
is no migratable non-virtual ccw device, we should consider using the
same bus-id generation scheme for virtual and non-virtual devices.

---
 hw/s390x/ccw-device.c | 6 ++++++
 hw/s390x/css.c        | 9 ---------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index f9bfa154d6..2167ccea5d 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static bool prop_get_true(Object *obj, Error **errp)
+{
+    return true;
+}
 static void ccw_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
     k->realize = ccw_device_realize;
     k->refill_ids = ccw_device_refill_ids;
     dc->props = ccw_device_properties;
+    object_class_property_add_bool(klass, "cssid-unrestricted",
+                                   prop_get_true, NULL, NULL);
 }
 
 const VMStateDescription vmstate_ccw_dev = {
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index f6b5c807cd..957cb9ec90 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
     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]) {
-- 
2.13.5

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 21 Nov 2017 12:18:25 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

Subject: s/unresrict/unrestrict/

> 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), and inform management software property on all ccw
> devices.

I do not really like dropping the restrictions while still keeping the
default cssid as 0xfe. Putting virtual devices into css 0 seems
completely fine, but putting non-virtual devices into css 0xfe still
feels a bit wrong. What about:

- Add a property to specify the default cssid. Compat machines use a
  default cssid of 0xfe.
- Default to a cssid of 0.
- (optional) Warn when putting a non-virtual device into css 0xfe,
  unless it is the default css.

> 
> The adverse effect on migration should not be too severe as
> 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.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 
> ---
> Hi!
> 
> About indicating this at the ccw devices instead of, e.g. as a machine
> property (or otherwise globally), was requested by our libvirt guys. I
> have no strong opinion regarding in this matter.

I would like to understand why. It feels very odd.

> 
> This patch is intended as a discussion starter. I would at least like to
> get a Tested-by by Shalini before promoting it to non-RFC (provided the
> discussion goes well).
> 
> TODOs:
> * Consider adding a description for the new property.

As it is, it is rather incomprehensible. But see below.

> * Consider renaming VIRTUAL_CSSID.

Why? This is still reserved for virtual devices in the architecture.
You just change qemu policy (and possibly what the default cssid is).

> * Consider changing the bus-id generation scheme (when
> devno is not specified by the user). his patch keep the current scheme in
> place: they won't go into the default css (but slots are filled up
> starting at cssid 0). This is theoretically good for migration
> compatibility same command line same addresses.  Practically since there
> is no migratable non-virtual ccw device, we should consider using the
> same bus-id generation scheme for virtual and non-virtual devices.

How does this interact with the squash parameter?

If we force the default css to 0xfe for compat machines, we should be
fine if we autogenerate to the default css. If you start at css 0
regardless of the default css, you might end up with a configuration
that the user did not expect at all.

> 
> ---
>  hw/s390x/ccw-device.c | 6 ++++++
>  hw/s390x/css.c        | 9 ---------
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index f9bfa154d6..2167ccea5d 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static bool prop_get_true(Object *obj, Error **errp)
> +{
> +    return true;
> +}
>  static void ccw_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>      k->realize = ccw_device_realize;
>      k->refill_ids = ccw_device_refill_ids;
>      dc->props = ccw_device_properties;
> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> +                                   prop_get_true, NULL, NULL);

This looks really, really strange. This is a property that is always
true if it exists.

What about compat machines? This qemu won't have the restriction, but
old qemus will.

Also, I'd consider this a property of the machine, not of the
individual devices. Or of the ChannelSubsystem structure, which is not
qom'ified.

As an alternative, I think providing a machine default_cssid parameter
makes more sense: It is understandable, and you keep compatibility. I
think we want this in the long run anyway.

>  }
>  
>  const VMStateDescription vmstate_ccw_dev = {
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index f6b5c807cd..957cb9ec90 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>      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;
> -        }
> -    }

This allows building a commandline in a compat machine that will not
work with old qemus, no?

> -
> -    if (bus_id.valid) {
>          if (squash_mcss) {
>              bus_id.cssid = channel_subsys.default_cssid;
>          } else if (!channel_subsys.css[bus_id.cssid]) {

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/21/2017 02:44 PM, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 12:18:25 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> Subject: s/unresrict/unrestrict/
> 
>> 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), and inform management software property on all ccw
>> devices.
> 
> I do not really like dropping the restrictions while still keeping the


> default cssid as 0xfe. Putting virtual devices into css 0 seems
> completely fine, but putting non-virtual devices into css 0xfe still
> feels a bit wrong. What about:

I see no fundamental reason to not allow any device everywhere. And yes,
if we would start today, we would not use fe but 0. But we already have 
code outside (libvirt), we have exisiting guest xmls and we have documentation
outside and we have  communicated that via  a lot of channels. So if we want
to stay compatible we should really keep css 0xfe as the default css (that
is seen as css 0 anyway for non-mcss-e guests).

The nice thing with Halils approach is that it will continue to work
if future guests enable MCSS-E and it is the most minimal change by just
lifting a restriction in terms of flexibility. 

> 
> - Add a property to specify the default cssid. Compat machines use a
>   default cssid of 0xfe.
> - Default to a cssid of 0.
> - (optional) Warn when putting a non-virtual device into css 0xfe,
>   unless it is the default css.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/21/2017 02:44 PM, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 12:18:25 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> Subject: s/unresrict/unrestrict/
> 
>> 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), and inform management software property on all ccw
>> devices.
> 
> I do not really like dropping the restrictions while still keeping the
> default cssid as 0xfe. Putting virtual devices into css 0 seems
> completely fine, but putting non-virtual devices into css 0xfe still
> feels a bit wrong. What about:
> 
> - Add a property to specify the default cssid. Compat machines use a
>   default cssid of 0xfe.
> - Default to a cssid of 0.
> - (optional) Warn when putting a non-virtual device into css 0xfe,
>   unless it is the default css.

To make it more clear, I think the most compatible solution would be to allow
vfio-ccw also in FE. (But continue to force virtual devices in FE as well).
This was more or less the first proposal that we had. Then we asked ourselves
why not also allow virtual devices in 0?

I think your proposal of specifying a default css (and then allowing
all devices in that) is actually pretty close to Halils proposal. The only
difference is that Halils variant keeps fe as default css. 
So I think we could even combine both approaches. Use Halils patch as a base
and in addition provide a way to change the default css away from fe. Have to
think about that.


[...]
>>
>> ---
>>  hw/s390x/ccw-device.c | 6 ++++++
>>  hw/s390x/css.c        | 9 ---------
>>  2 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
>> index f9bfa154d6..2167ccea5d 100644
>> --- a/hw/s390x/ccw-device.c
>> +++ b/hw/s390x/ccw-device.c
>> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> +static bool prop_get_true(Object *obj, Error **errp)
>> +{
>> +    return true;
>> +}
>>  static void ccw_device_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>>      k->realize = ccw_device_realize;
>>      k->refill_ids = ccw_device_refill_ids;
>>      dc->props = ccw_device_properties;
>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>> +                                   prop_get_true, NULL, NULL);
> 
> This looks really, really strange. This is a property that is always
> true if it exists.
> 
> What about compat machines? This qemu won't have the restriction, but
> old qemus will.
> 
> Also, I'd consider this a property of the machine, not of the
> individual devices. Or of the ChannelSubsystem structure, which is not
> qom'ified.

A property per device allow to put restrictions on specific devices if necessary.
Not sure if we need it. I think for libvirt several variants would work out
(a property as seen here, a new object, the default_css machine option)

> 
> As an alternative, I think providing a machine default_cssid parameter
> makes more sense: It is understandable, and you keep compatibility. I
> think we want this in the long run anyway.
> 
>>  }
>>  
>>  const VMStateDescription vmstate_ccw_dev = {
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index f6b5c807cd..957cb9ec90 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>>      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;
>> -        }
>> -    }
> 
> This allows building a commandline in a compat machine that will not
> work with old qemus, no?

I think this is pretty common to have new devices and things that will not
work on old QEMUs but are allowed on compat machines, e.g. virtio-gpu was
added in 2.4 but

[cborntra@oc7330422307 qemu]$ build/i386-softmmu/qemu-system-i386 -device virtio-gpu-pci -M pc-i440fx-2.2
VNC server running on ::1:5900

works fine

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/21/2017 02:44 PM, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 12:18:25 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> Subject: s/unresrict/unrestrict/

Sure!

> 
>> 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), and inform management software property on all ccw
>> devices.
> 
> I do not really like dropping the restrictions while still keeping the
> default cssid as 0xfe. Putting virtual devices into css 0 seems
> completely fine, but putting non-virtual devices into css 0xfe still
> feels a bit wrong. What about:
> 
> - Add a property to specify the default cssid. Compat machines use a
>   default cssid of 0xfe.
> - Default to a cssid of 0.
> - (optional) Warn when putting a non-virtual device into css 0xfe,
>   unless it is the default css.
> 

Please see Christians response. IMHO the libvirt perspective, and
especially keeping the domain xmls as they are today viable is the
key to a good solution.

AFAIU one should probably use vfio-ccw as devices having their own
xml managed via attach-device and detach-device, as they are not
migratable (thus need to be removed before migrating). If we want
to be able to attach such devices to domains especially created
with vfio-ccw in mind putting all the devices into 0xfe seems to
be the only sane way.

Something like mcsse-squash isn't really a good solution, because
doing it behind of the back of the user in libvirt feels wrong,
and if we have to make the user put it in the domain xml then
we are back at special domain definitions problem.

>>
>> The adverse effect on migration should not be too severe as
>> 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.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>
>> ---
>> Hi!
>>
>> About indicating this at the ccw devices instead of, e.g. as a machine
>> property (or otherwise globally), was requested by our libvirt guys. I
>> have no strong opinion regarding in this matter.
> 
> I would like to understand why. It feels very odd.
> 

@Boris: I would like to delegate explaining to you. I did understand
your arguments, but I'm not confident about being able to reproduce them
authentically.

>>
>> This patch is intended as a discussion starter. I would at least like to
>> get a Tested-by by Shalini before promoting it to non-RFC (provided the
>> discussion goes well).
>>
>> TODOs:
>> * Consider adding a description for the new property.
> 
> As it is, it is rather incomprehensible. But see below.
> 

OK. The idea is that this property should be used for libvirt.

Comprehensibility is a general problem as the user should not
really have to care about mcss-e at all (see PoP). How should we
explain mcss-e to the user? AFAIR this is what triggered the usability
discussion.

>> * Consider renaming VIRTUAL_CSSID.
> 
> Why? This is still reserved for virtual devices in the architecture.
> You just change qemu policy (and possibly what the default cssid is).
>

I don't think so. Where is it reserved in the architecture? The
only reference I've found is our VSM book. Sorry I really can't
find it.
 
>> * Consider changing the bus-id generation scheme (when
>> devno is not specified by the user). his patch keep the current scheme in
>> place: they won't go into the default css (but slots are filled up
>> starting at cssid 0). This is theoretically good for migration
>> compatibility same command line same addresses.  Practically since there
>> is no migratable non-virtual ccw device, we should consider using the
>> same bus-id generation scheme for virtual and non-virtual devices.
> 
> How does this interact with the squash parameter?


With squash it would not change anything: we would start at default cssid which is 0
with squash. Without squash it would have the effect that we first
fill the default css and then proceed to the next one. Would change
behavior. The hope is that nobody used non-virtual devices without
squash on, as they are useless that way since there is no mcss-e
guest around.

The expected benefit is that the devices would show up in the guest
instead of being effectively inaccessible -- sane defaults.

I forgot to write, but I would actually like to deprecate the squash.
I see it as something on top though.

> 
> If we force the default css to 0xfe for compat machines, we should be
> fine if we autogenerate to the default css. If you start at css 0
> regardless of the default css, you might end up with a configuration
> that the user did not expect at all.
> 

I don't force anything in the compat machines here. So I don't understand
your question.


>>
>> ---
>>  hw/s390x/ccw-device.c | 6 ++++++
>>  hw/s390x/css.c        | 9 ---------
>>  2 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
>> index f9bfa154d6..2167ccea5d 100644
>> --- a/hw/s390x/ccw-device.c
>> +++ b/hw/s390x/ccw-device.c
>> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> +static bool prop_get_true(Object *obj, Error **errp)
>> +{
>> +    return true;
>> +}
>>  static void ccw_device_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>>      k->realize = ccw_device_realize;
>>      k->refill_ids = ccw_device_refill_ids;
>>      dc->props = ccw_device_properties;
>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>> +                                   prop_get_true, NULL, NULL);
> 
> This looks really, really strange. This is a property that is always
> true if it exists.
> 

Won't argue about that. The libvirt guys are actually not interested
int he value at all: only that there is such a property.

> What about compat machines? This qemu won't have the restriction, but
> old qemus will.
> 

Very true. But as the commit message implies it should not be a problem.

> Also, I'd consider this a property of the machine, not of the
> individual devices. Or of the ChannelSubsystem structure, which is not
> qom'ified.
> 

I've said the exact same thing to Boris. He said from libvirt perspective
a device property is better.

> As an alternative, I think providing a machine default_cssid parameter
> makes more sense: It is understandable, and you keep compatibility. I
> think we want this in the long run anyway.
> 

I think most of us had the idea. I support this idea fully (expose default
cssid to the user (rw)). I see it as something that can be done on top
and is not a pressing issue, but rather a nice to have.

>>  }
>>  
>>  const VMStateDescription vmstate_ccw_dev = {
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index f6b5c807cd..957cb9ec90 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>>      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;
>> -        }
>> -    }
> 
> This allows building a commandline in a compat machine that will not
> work with old qemus, no?
> 

Yes. We have considered this. I was convinced by Christian that
it ain't too bad. In the end, if we don't want non-virtual device
aware domains (see above), then there is no way to make this work
with libvirt. Actually to make the migration work with libvirt with
old qemus the only way would be to use sqash. But libvirt does not
want that.

Also consider that vfio-ccw (AFAIK the only non-virtual css device
type) is not migratable. So having them on the command line and live
migrating is a lost case already.

Yes, having a pre- 2.12 binary version and a post 2.12 binary version
way to use vfio-ccw is ugly to some extent. The recommendation would
be don't use it with pre 2.12 (libvirt is going to plainly refuse).

And yes with this one is going to be able to write a 2.12 command
line which does not work with 2.11 but that is normal.

This patch keeps the squash so the 2.10 definition will still be
viable for 2.12. Should we sometime get rid of the squash, then
that would be a breaking change.

Regards,
Halil

>> -
>> -    if (bus_id.valid) {
>>          if (squash_mcss) {
>>              bus_id.cssid = channel_subsys.default_cssid;
>>          } else if (!channel_subsys.css[bus_id.cssid]) {
> 

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 21 Nov 2017 15:45:17 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/21/2017 02:44 PM, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 12:18:25 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > 
> > Subject: s/unresrict/unrestrict/
> >   
> >> 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), and inform management software property on all ccw
> >> devices.  
> > 
> > I do not really like dropping the restrictions while still keeping the
> > default cssid as 0xfe. Putting virtual devices into css 0 seems
> > completely fine, but putting non-virtual devices into css 0xfe still
> > feels a bit wrong. What about:
> > 
> > - Add a property to specify the default cssid. Compat machines use a
> >   default cssid of 0xfe.
> > - Default to a cssid of 0.
> > - (optional) Warn when putting a non-virtual device into css 0xfe,
> >   unless it is the default css.  
> 
> To make it more clear, I think the most compatible solution would be to allow
> vfio-ccw also in FE. (But continue to force virtual devices in FE as well).
> This was more or less the first proposal that we had. Then we asked ourselves
> why not also allow virtual devices in 0?
> 
> I think your proposal of specifying a default css (and then allowing
> all devices in that) is actually pretty close to Halils proposal. The only
> difference is that Halils variant keeps fe as default css. 
> So I think we could even combine both approaches. Use Halils patch as a base
> and in addition provide a way to change the default css away from fe. Have to
> think about that.

If keeping the default of 0xfe makes the life of management software
easier, sure. As it is, we seem to be far more entrenched with
paravirtual devices than I had expected when I first wrote this, so
0xfe might be the sensible choice even if mcss-e is available in the
future. In this case, I think we should lift all cssid restrictions.

> 
> 
> [...]
> >>
> >> ---
> >>  hw/s390x/ccw-device.c | 6 ++++++
> >>  hw/s390x/css.c        | 9 ---------
> >>  2 files changed, 6 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> >> index f9bfa154d6..2167ccea5d 100644
> >> --- a/hw/s390x/ccw-device.c
> >> +++ b/hw/s390x/ccw-device.c
> >> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +    return true;
> >> +}
> >>  static void ccw_device_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
> >>      k->realize = ccw_device_realize;
> >>      k->refill_ids = ccw_device_refill_ids;
> >>      dc->props = ccw_device_properties;
> >> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >> +                                   prop_get_true, NULL, NULL);  
> > 
> > This looks really, really strange. This is a property that is always
> > true if it exists.
> > 
> > What about compat machines? This qemu won't have the restriction, but
> > old qemus will.
> > 
> > Also, I'd consider this a property of the machine, not of the
> > individual devices. Or of the ChannelSubsystem structure, which is not
> > qom'ified.  
> 
> A property per device allow to put restrictions on specific devices if necessary.
> Not sure if we need it.

If we would need restrictions, putting in the valid cssids (or
forbidding ids) would make more sense. But frankly, I don't really see
a use case for that.

> I think for libvirt several variants would work out
> (a property as seen here, a new object, the default_css machine option)

If so, I'd vote for either a new css object or a machine option.

> 
> > 
> > As an alternative, I think providing a machine default_cssid parameter
> > makes more sense: It is understandable, and you keep compatibility. I
> > think we want this in the long run anyway.
> >   
> >>  }
> >>  
> >>  const VMStateDescription vmstate_ccw_dev = {
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index f6b5c807cd..957cb9ec90 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
> >>      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;
> >> -        }
> >> -    }  
> > 
> > This allows building a commandline in a compat machine that will not
> > work with old qemus, no?  
> 
> I think this is pretty common to have new devices and things that will not
> work on old QEMUs but are allowed on compat machines, e.g. virtio-gpu was
> added in 2.4 but
> 
> [cborntra@oc7330422307 qemu]$ build/i386-softmmu/qemu-system-i386 -device virtio-gpu-pci -M pc-i440fx-2.2
> VNC server running on ::1:5900
> 
> works fine

It seems a bit more unexpected, though.

(And I'm still not quite sure how all of this interacts with the squash
option.)

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 21 Nov 2017 16:47:29 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/21/2017 02:44 PM, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 12:18:25 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > 
> > Subject: s/unresrict/unrestrict/  
> 
> Sure!
> 
> >   
> >> 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), and inform management software property on all ccw
> >> devices.  
> > 
> > I do not really like dropping the restrictions while still keeping the
> > default cssid as 0xfe. Putting virtual devices into css 0 seems
> > completely fine, but putting non-virtual devices into css 0xfe still
> > feels a bit wrong. What about:
> > 
> > - Add a property to specify the default cssid. Compat machines use a
> >   default cssid of 0xfe.
> > - Default to a cssid of 0.
> > - (optional) Warn when putting a non-virtual device into css 0xfe,
> >   unless it is the default css.
> >   
> 
> Please see Christians response. IMHO the libvirt perspective, and
> especially keeping the domain xmls as they are today viable is the
> key to a good solution.
> 
> AFAIU one should probably use vfio-ccw as devices having their own
> xml managed via attach-device and detach-device, as they are not
> migratable (thus need to be removed before migrating). If we want
> to be able to attach such devices to domains especially created
> with vfio-ccw in mind putting all the devices into 0xfe seems to
> be the only sane way.
> 
> Something like mcsse-squash isn't really a good solution, because
> doing it behind of the back of the user in libvirt feels wrong,
> and if we have to make the user put it in the domain xml then
> we are back at special domain definitions problem.

See my response in the other thread as well. I'm not really opposed to
keeping 0xfe as the default.

> 
> >>
> >> The adverse effect on migration should not be too severe as
> >> 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.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> >>
> >> ---
> >> Hi!
> >>
> >> About indicating this at the ccw devices instead of, e.g. as a machine
> >> property (or otherwise globally), was requested by our libvirt guys. I
> >> have no strong opinion regarding in this matter.  
> > 
> > I would like to understand why. It feels very odd.
> >   
> 
> @Boris: I would like to delegate explaining to you. I did understand
> your arguments, but I'm not confident about being able to reproduce them
> authentically.
> 
> >>
> >> This patch is intended as a discussion starter. I would at least like to
> >> get a Tested-by by Shalini before promoting it to non-RFC (provided the
> >> discussion goes well).
> >>
> >> TODOs:
> >> * Consider adding a description for the new property.  
> > 
> > As it is, it is rather incomprehensible. But see below.
> >   
> 
> OK. The idea is that this property should be used for libvirt.

Yes, but what for? That was my problem.

> 
> Comprehensibility is a general problem as the user should not
> really have to care about mcss-e at all (see PoP). How should we
> explain mcss-e to the user? AFAIR this is what triggered the usability
> discussion.

I think we can expect an admin wanting to set up machines understand
the fact that there are multiple cssids, but only the default one can
be seen by most guests.

> 
> >> * Consider renaming VIRTUAL_CSSID.  
> > 
> > Why? This is still reserved for virtual devices in the architecture.
> > You just change qemu policy (and possibly what the default cssid is).
> >  
> 
> I don't think so. Where is it reserved in the architecture? The
> only reference I've found is our VSM book. Sorry I really can't
> find it.

It was reserved with some architecture folks; Christian should know (I
obviously don't have access to anything anymore).

>  
> >> * Consider changing the bus-id generation scheme (when
> >> devno is not specified by the user). his patch keep the current scheme in
> >> place: they won't go into the default css (but slots are filled up
> >> starting at cssid 0). This is theoretically good for migration
> >> compatibility same command line same addresses.  Practically since there
> >> is no migratable non-virtual ccw device, we should consider using the
> >> same bus-id generation scheme for virtual and non-virtual devices.  
> > 
> > How does this interact with the squash parameter?  
> 
> 
> With squash it would not change anything: we would start at default cssid which is 0
> with squash. Without squash it would have the effect that we first
> fill the default css and then proceed to the next one. Would change
> behavior. The hope is that nobody used non-virtual devices without
> squash on, as they are useless that way since there is no mcss-e
> guest around.
> 
> The expected benefit is that the devices would show up in the guest
> instead of being effectively inaccessible -- sane defaults.
> 
> I forgot to write, but I would actually like to deprecate the squash.
> I see it as something on top though.

I'd vote for getting rid of it as soon as possible.

> 
> > 
> > If we force the default css to 0xfe for compat machines, we should be
> > fine if we autogenerate to the default css. If you start at css 0
> > regardless of the default css, you might end up with a configuration
> > that the user did not expect at all.
> >   
> 
> I don't force anything in the compat machines here. So I don't understand
> your question.

It refers to my suggestion above (specifying a default css).

> 
> 
> >>
> >> ---
> >>  hw/s390x/ccw-device.c | 6 ++++++
> >>  hw/s390x/css.c        | 9 ---------
> >>  2 files changed, 6 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> >> index f9bfa154d6..2167ccea5d 100644
> >> --- a/hw/s390x/ccw-device.c
> >> +++ b/hw/s390x/ccw-device.c
> >> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +    return true;
> >> +}
> >>  static void ccw_device_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
> >>      k->realize = ccw_device_realize;
> >>      k->refill_ids = ccw_device_refill_ids;
> >>      dc->props = ccw_device_properties;
> >> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >> +                                   prop_get_true, NULL, NULL);  
> > 
> > This looks really, really strange. This is a property that is always
> > true if it exists.
> >   
> 
> Won't argue about that. The libvirt guys are actually not interested
> int he value at all: only that there is such a property.

So what about a machine property? Wouldn't that help as well?

Or a css object?

> 
> > What about compat machines? This qemu won't have the restriction, but
> > old qemus will.
> >   
> 
> Very true. But as the commit message implies it should not be a problem.

How is that supposed to play with libvirt detection, then?

> 
> > Also, I'd consider this a property of the machine, not of the
> > individual devices. Or of the ChannelSubsystem structure, which is not
> > qom'ified.
> >   
> 
> I've said the exact same thing to Boris. He said from libvirt perspective
> a device property is better.
> 
> > As an alternative, I think providing a machine default_cssid parameter
> > makes more sense: It is understandable, and you keep compatibility. I
> > think we want this in the long run anyway.
> >   
> 
> I think most of us had the idea. I support this idea fully (expose default
> cssid to the user (rw)). I see it as something that can be done on top
> and is not a pressing issue, but rather a nice to have.

TBH, this weird property is what I like least about this patch.

> 
> >>  }
> >>  
> >>  const VMStateDescription vmstate_ccw_dev = {
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index f6b5c807cd..957cb9ec90 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
> >>      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;
> >> -        }
> >> -    }  
> > 
> > This allows building a commandline in a compat machine that will not
> > work with old qemus, no?
> >   
> 
> Yes. We have considered this. I was convinced by Christian that
> it ain't too bad. In the end, if we don't want non-virtual device
> aware domains (see above), then there is no way to make this work
> with libvirt. Actually to make the migration work with libvirt with
> old qemus the only way would be to use sqash. But libvirt does not
> want that.
> 
> Also consider that vfio-ccw (AFAIK the only non-virtual css device
> type) is not migratable. So having them on the command line and live
> migrating is a lost case already.
> 
> Yes, having a pre- 2.12 binary version and a post 2.12 binary version
> way to use vfio-ccw is ugly to some extent. The recommendation would
> be don't use it with pre 2.12 (libvirt is going to plainly refuse).
> 
> And yes with this one is going to be able to write a 2.12 command
> line which does not work with 2.11 but that is normal.
> 
> This patch keeps the squash so the 2.10 definition will still be
> viable for 2.12. Should we sometime get rid of the squash, then
> that would be a breaking change.

Removing squash is easy to detect. I'm a bit worried about
not-obvious-at-the-start breakage.

> 
> Regards,
> Halil
> 
> >> -
> >> -    if (bus_id.valid) {
> >>          if (squash_mcss) {
> >>              bus_id.cssid = channel_subsys.default_cssid;
> >>          } else if (!channel_subsys.css[bus_id.cssid]) {  
> >   
> 

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/21/2017 05:20 PM, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 16:47:29 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/21/2017 02:44 PM, Cornelia Huck wrote:
>>> On Tue, 21 Nov 2017 12:18:25 +0100
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
[..]
>>>
>>> - Add a property to specify the default cssid. Compat machines use a
>>>   default cssid of 0xfe.
>>> - Default to a cssid of 0.
>>> - (optional) Warn when putting a non-virtual device into css 0xfe,
>>>   unless it is the default css.
>>>   
>>
>> Please see Christians response. IMHO the libvirt perspective, and
>> especially keeping the domain xmls as they are today viable is the
>> key to a good solution.
>>
>> AFAIU one should probably use vfio-ccw as devices having their own
>> xml managed via attach-device and detach-device, as they are not
>> migratable (thus need to be removed before migrating). If we want
>> to be able to attach such devices to domains especially created
>> with vfio-ccw in mind putting all the devices into 0xfe seems to
>> be the only sane way.
>>
>> Something like mcsse-squash isn't really a good solution, because
>> doing it behind of the back of the user in libvirt feels wrong,
>> and if we have to make the user put it in the domain xml then
>> we are back at special domain definitions problem.
> 
> See my response in the other thread as well. I'm not really opposed to
> keeping 0xfe as the default.
> 

Sure. Done.

>>
>>>>
>>>> The adverse effect on migration should not be too severe as
>>>> 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.
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>>
>>>> ---
>>>> Hi!
>>>>
>>>> About indicating this at the ccw devices instead of, e.g. as a machine
>>>> property (or otherwise globally), was requested by our libvirt guys. I
>>>> have no strong opinion regarding in this matter.  
>>>
>>> I would like to understand why. It feels very odd.
>>>   
>>
>> @Boris: I would like to delegate explaining to you. I did understand
>> your arguments, but I'm not confident about being able to reproduce them
>> authentically.
>>
>>>>
>>>> This patch is intended as a discussion starter. I would at least like to
>>>> get a Tested-by by Shalini before promoting it to non-RFC (provided the
>>>> discussion goes well).
>>>>
>>>> TODOs:
>>>> * Consider adding a description for the new property.  
>>>
>>> As it is, it is rather incomprehensible. But see below.
>>>   
>>
>> OK. The idea is that this property should be used for libvirt.
> 
> Yes, but what for? That was my problem.
> 

Libvirt is going to refuse using vfio-ccw devices if the
property is not defined (existent). I hope Boris is going to provide
his perspective tomorrow.

>>
>> Comprehensibility is a general problem as the user should not
>> really have to care about mcss-e at all (see PoP). How should we
>> explain mcss-e to the user? AFAIR this is what triggered the usability
>> discussion.
> 
> I think we can expect an admin wanting to set up machines understand
> the fact that there are multiple cssids, but only the default one can
> be seen by most guests.
> 

That much at least probably. But I think the less the admin/user is
forced to know abut multiple channel subsystems the better. For various
reasons.

>>
>>>> * Consider renaming VIRTUAL_CSSID.  
>>>
>>> Why? This is still reserved for virtual devices in the architecture.
>>> You just change qemu policy (and possibly what the default cssid is).
>>>  
>>
>> I don't think so. Where is it reserved in the architecture? The
>> only reference I've found is our VSM book. Sorry I really can't
>> find it.
> 
> It was reserved with some architecture folks; Christian should know (I
> obviously don't have access to anything anymore).
> 

@Christian: AFAIU you this wont be a problem. Let's have a chat about this
tomorrow.
 
>>  
>>>> * Consider changing the bus-id generation scheme (when
>>>> devno is not specified by the user). his patch keep the current scheme in
>>>> place: they won't go into the default css (but slots are filled up
>>>> starting at cssid 0). This is theoretically good for migration
>>>> compatibility same command line same addresses.  Practically since there
>>>> is no migratable non-virtual ccw device, we should consider using the
>>>> same bus-id generation scheme for virtual and non-virtual devices.  
>>>
>>> How does this interact with the squash parameter?  
>>
>>
>> With squash it would not change anything: we would start at default cssid which is 0
>> with squash. Without squash it would have the effect that we first
>> fill the default css and then proceed to the next one. Would change
>> behavior. The hope is that nobody used non-virtual devices without
>> squash on, as they are useless that way since there is no mcss-e
>> guest around.
>>
>> The expected benefit is that the devices would show up in the guest
>> instead of being effectively inaccessible -- sane defaults.
>>
>> I forgot to write, but I would actually like to deprecate the squash.
>> I see it as something on top though.
> 
> I'd vote for getting rid of it as soon as possible.
>

Me to. I've already got my patch for deprecation half baked ;).
 
The original question was about weather keep the start putting
non-virtual devices into (the non-guest-visible) 0 if no devno is
specified, or rather fill the default first and only then spill
to the next css.

>>
>>>
>>> If we force the default css to 0xfe for compat machines, we should be
>>> fine if we autogenerate to the default css. If you start at css 0
>>> regardless of the default css, you might end up with a configuration
>>> that the user did not expect at all.
>>>   
>>
>> I don't force anything in the compat machines here. So I don't understand
>> your question.
> 
> It refers to my suggestion above (specifying a default css).
> 

Was my understanding too, so I did not want to go too deep.

[..]

>>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>>> +                                   prop_get_true, NULL, NULL);  
>>>
>>> This looks really, really strange. This is a property that is always
>>> true if it exists.
>>>   
>>
>> Won't argue about that. The libvirt guys are actually not interested
>> int he value at all: only that there is such a property.
> 
> So what about a machine property? Wouldn't that help as well?
> 

Technically it is doable. The property would be still a weird
one, but from QEMU perspective in a less weird place. I was also
arguing that from OO perspective this kind of a don't care about
it's value property is weird, but AFAIK not having the info if
we can do something with a device at the device is weird from
Libvirt perspective. I'm really uncomforble with speaking for
Libvirt/Boris. Hope he will make his point tomorrow.

> Or a css object?
> 

My first internal-only version used this approach, but
I was asked to do it like this.

>>
>>> What about compat machines? This qemu won't have the restriction, but
>>> old qemus will.
>>>   
>>
>> Very true. But as the commit message implies it should not be a problem.
> 
> How is that supposed to play with libvirt detection, then?
> 

As written above. Libvirt will simply refuse to use vfio-ccw if the property
is not defined. This results in no prior history to care about for libvirt
users: vfio-ccw effectively becomes available with qemu 2.12.

>>
>>> Also, I'd consider this a property of the machine, not of the
>>> individual devices. Or of the ChannelSubsystem structure, which is not
>>> qom'ified.
>>>   
>>
>> I've said the exact same thing to Boris. He said from libvirt perspective
>> a device property is better.
>>
>>> As an alternative, I think providing a machine default_cssid parameter
>>> makes more sense: It is understandable, and you keep compatibility. I
>>> think we want this in the long run anyway.
>>>   
>>
>> I think most of us had the idea. I support this idea fully (expose default
>> cssid to the user (rw)). I see it as something that can be done on top
>> and is not a pressing issue, but rather a nice to have.
> 
> TBH, this weird property is what I like least about this patch.
> 

I'm fine with any of the 3 alternatives (as is, new type, new machine prop).

I do agree the property is weird, but a machine property would in my opinion
also be weird (especially form libvirt perspective, but also from qemu
perspective e.g. compat handling and machine type none).

I used to think that using a trait type is the cleanest, but one advantage
of the approach taken in this patch is that it can be introspected via command
line (it is quite weird though).

>>>>  }
>>>>  
>>>>  const VMStateDescription vmstate_ccw_dev = {
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index f6b5c807cd..957cb9ec90 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>>>>      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;
>>>> -        }
>>>> -    }  
>>>
>>> This allows building a commandline in a compat machine that will not
>>> work with old qemus, no?
>>>   
>>
>> Yes. We have considered this. I was convinced by Christian that
>> it ain't too bad. In the end, if we don't want non-virtual device
>> aware domains (see above), then there is no way to make this work
>> with libvirt. Actually to make the migration work with libvirt with
>> old qemus the only way would be to use sqash. But libvirt does not
>> want that.
>>
>> Also consider that vfio-ccw (AFAIK the only non-virtual css device
>> type) is not migratable. So having them on the command line and live
>> migrating is a lost case already.
>>
>> Yes, having a pre- 2.12 binary version and a post 2.12 binary version
>> way to use vfio-ccw is ugly to some extent. The recommendation would
>> be don't use it with pre 2.12 (libvirt is going to plainly refuse).
>>
>> And yes with this one is going to be able to write a 2.12 command
>> line which does not work with 2.11 but that is normal.
>>
>> This patch keeps the squash so the 2.10 definition will still be
>> viable for 2.12. Should we sometime get rid of the squash, then
>> that would be a breaking change.
> 
> Removing squash is easy to detect. I'm a bit worried about
> not-obvious-at-the-start breakage.
> 

Again won't happen with libvirt. With a direct command line user
downgrading or moving to a qemu 2.10 or 2.11 it is a risk we can
take IMHO.

Also I can't find anything about vfio-ccw in the upstream users
manual for 2.10.91. So I would argue using vfio-ccw is using an
undocumented feature: if undocumented features changing in a non
compatible way hits you, it's partly your own fault.

To sum it up. It is not nice, I agree but I don't see better alternatives.

I hope you and Boris will reach consensus on what is the best
way to expose the change.  I really don't have any strong opinion
on this (maybe except finding the machine property solution the least
favorable one). If it turns out Boris does not want to lead this
discussion, I will take responsibility as the author of course.

Regards,
Halil

[..]

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/21/2017 05:06 PM, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 15:45:17 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 11/21/2017 02:44 PM, Cornelia Huck wrote:
>>> On Tue, 21 Nov 2017 12:18:25 +0100
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>
>>> Subject: s/unresrict/unrestrict/
>>>   
>>>> 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), and inform management software property on all ccw
>>>> devices.  
>>>
>>> I do not really like dropping the restrictions while still keeping the
>>> default cssid as 0xfe. Putting virtual devices into css 0 seems
>>> completely fine, but putting non-virtual devices into css 0xfe still
>>> feels a bit wrong. What about:
>>>
>>> - Add a property to specify the default cssid. Compat machines use a
>>>   default cssid of 0xfe.
>>> - Default to a cssid of 0.
>>> - (optional) Warn when putting a non-virtual device into css 0xfe,
>>>   unless it is the default css.  
>>
>> To make it more clear, I think the most compatible solution would be to allow
>> vfio-ccw also in FE. (But continue to force virtual devices in FE as well).
>> This was more or less the first proposal that we had. Then we asked ourselves
>> why not also allow virtual devices in 0?
>>
>> I think your proposal of specifying a default css (and then allowing
>> all devices in that) is actually pretty close to Halils proposal. The only
>> difference is that Halils variant keeps fe as default css. 
>> So I think we could even combine both approaches. Use Halils patch as a base
>> and in addition provide a way to change the default css away from fe. Have to
>> think about that.
> 
> If keeping the default of 0xfe makes the life of management software
> easier, sure. As it is, we seem to be far more entrenched with
> paravirtual devices than I had expected when I first wrote this, so
> 0xfe might be the sensible choice even if mcss-e is available in the
> future. In this case, I think we should lift all cssid restrictions.

So basically agree with the restriction lifting, but you want to have a
different method of telling libvirt about it?
[...]
>>> This allows building a commandline in a compat machine that will not
>>> work with old qemus, no?  
>>
>> I think this is pretty common to have new devices and things that will not
>> work on old QEMUs but are allowed on compat machines, e.g. virtio-gpu was
>> added in 2.4 but
>>
>> [cborntra@oc7330422307 qemu]$ build/i386-softmmu/qemu-system-i386 -device virtio-gpu-pci -M pc-i440fx-2.2
>> VNC server running on ::1:5900
>>
>> works fine
> 
> It seems a bit more unexpected, though.

What I understood from the CPU model discussion is, that the machine version basically sets the migration
stream format and any "invisible" setting that the user cannot influence. But it is not there to the fence
the user from specifying new devices or new CPU types (that the old QEMU does not know) or different 
command lines. You get a compatible machine if you specify an identical command line that will work for
both.
> 
> (And I'm still not quite sure how all of this interacts with the squash
> option.)

I think we should deprecate the squash option and not use it to simplify things.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Shalini Chellathurai Saroja]

On 11/21/2017 12:18 PM, Halil Pasic 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 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), and inform management software property on all ccw
> devices.
>
> The adverse effect on migration should not be too severe as
> 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.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>
> ---
> Hi!
>
> About indicating this at the ccw devices instead of, e.g. as a machine
> property (or otherwise globally), was requested by our libvirt guys. I
> have no strong opinion regarding in this matter.
>
> This patch is intended as a discussion starter. I would at least like to
> get a Tested-by by Shalini before promoting it to non-RFC (provided the
> discussion goes well).
Tested the patch in libvirt. It works as expected (allows to specify 
cssid of vfio devices as 0xfe).
Thank you.
> TODOs:
> * Consider adding a description for the new property.
> * Consider renaming VIRTUAL_CSSID.
> * Consider changing the bus-id generation scheme (when
> devno is not specified by the user). his patch keep the current scheme in
> place: they won't go into the default css (but slots are filled up
> starting at cssid 0). This is theoretically good for migration
> compatibility same command line same addresses.  Practically since there
> is no migratable non-virtual ccw device, we should consider using the
> same bus-id generation scheme for virtual and non-virtual devices.
>
> ---
>   hw/s390x/ccw-device.c | 6 ++++++
>   hw/s390x/css.c        | 9 ---------
>   2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index f9bfa154d6..2167ccea5d 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> +static bool prop_get_true(Object *obj, Error **errp)
> +{
> +    return true;
> +}
>   static void ccw_device_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
>       k->realize = ccw_device_realize;
>       k->refill_ids = ccw_device_refill_ids;
>       dc->props = ccw_device_properties;
> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> +                                   prop_get_true, NULL, NULL);
>   }
>
>   const VMStateDescription vmstate_ccw_dev = {
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index f6b5c807cd..957cb9ec90 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>       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]) {

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 21 Nov 2017 18:05:46 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/21/2017 05:20 PM, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 16:47:29 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 11/21/2017 02:44 PM, Cornelia Huck wrote:  
> >>> On Tue, 21 Nov 2017 12:18:25 +0100
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:  

> >>>> * Consider changing the bus-id generation scheme (when
> >>>> devno is not specified by the user). his patch keep the current scheme in
> >>>> place: they won't go into the default css (but slots are filled up
> >>>> starting at cssid 0). This is theoretically good for migration
> >>>> compatibility same command line same addresses.  Practically since there
> >>>> is no migratable non-virtual ccw device, we should consider using the
> >>>> same bus-id generation scheme for virtual and non-virtual devices.    
> >>>
> >>> How does this interact with the squash parameter?    
> >>
> >>
> >> With squash it would not change anything: we would start at default cssid which is 0
> >> with squash. Without squash it would have the effect that we first
> >> fill the default css and then proceed to the next one. Would change
> >> behavior. The hope is that nobody used non-virtual devices without
> >> squash on, as they are useless that way since there is no mcss-e
> >> guest around.
> >>
> >> The expected benefit is that the devices would show up in the guest
> >> instead of being effectively inaccessible -- sane defaults.
> >>
> >> I forgot to write, but I would actually like to deprecate the squash.
> >> I see it as something on top though.  
> > 
> > I'd vote for getting rid of it as soon as possible.
> >  
> 
> Me to. I've already got my patch for deprecation half baked ;).
>  
> The original question was about weather keep the start putting
> non-virtual devices into (the non-guest-visible) 0 if no devno is
> specified, or rather fill the default first and only then spill
> to the next css.

Combined with what I said right below, I think we should be fine
autogenerating into the default css. Anything else will just generate
unusable configurations when the squash parameter is gone.

> 
> >>  
> >>>
> >>> If we force the default css to 0xfe for compat machines, we should be
> >>> fine if we autogenerate to the default css. If you start at css 0
> >>> regardless of the default css, you might end up with a configuration
> >>> that the user did not expect at all.

> >>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >>>> +                                   prop_get_true, NULL, NULL);    
> >>>
> >>> This looks really, really strange. This is a property that is always
> >>> true if it exists.
> >>>     
> >>
> >> Won't argue about that. The libvirt guys are actually not interested
> >> int he value at all: only that there is such a property.  
> > 
> > So what about a machine property? Wouldn't that help as well?
> >   
> 
> Technically it is doable. The property would be still a weird
> one, but from QEMU perspective in a less weird place. I was also
> arguing that from OO perspective this kind of a don't care about
> it's value property is weird, but AFAIK not having the info if
> we can do something with a device at the device is weird from
> Libvirt perspective. I'm really uncomforble with speaking for
> Libvirt/Boris. Hope he will make his point tomorrow.
> 
> > Or a css object?
> >   
> 
> My first internal-only version used this approach, but
> I was asked to do it like this.

If we formulate this rather as "we now expose the default css", we (a)
have something that really makes the most sense at a channel subsystem
or machine level, and (b) can be detected by libvirt as an indicator of
"no restriction for virtual vs. non-virtual".

> 
> >>  
> >>> What about compat machines? This qemu won't have the restriction, but
> >>> old qemus will.
> >>>     
> >>
> >> Very true. But as the commit message implies it should not be a problem.  
> > 
> > How is that supposed to play with libvirt detection, then?
> >   
> 
> As written above. Libvirt will simply refuse to use vfio-ccw if the property
> is not defined. This results in no prior history to care about for libvirt
> users: vfio-ccw effectively becomes available with qemu 2.12.

I'm not worried about vfio-ccw. As you said, this should work. We just
need to make sure that we don't break existing setups. (I don't think
we will.)

> 
> >>  
> >>> Also, I'd consider this a property of the machine, not of the
> >>> individual devices. Or of the ChannelSubsystem structure, which is not
> >>> qom'ified.
> >>>     
> >>
> >> I've said the exact same thing to Boris. He said from libvirt perspective
> >> a device property is better.
> >>  
> >>> As an alternative, I think providing a machine default_cssid parameter
> >>> makes more sense: It is understandable, and you keep compatibility. I
> >>> think we want this in the long run anyway.
> >>>     
> >>
> >> I think most of us had the idea. I support this idea fully (expose default
> >> cssid to the user (rw)). I see it as something that can be done on top
> >> and is not a pressing issue, but rather a nice to have.  
> > 
> > TBH, this weird property is what I like least about this patch.
> >   
> 
> I'm fine with any of the 3 alternatives (as is, new type, new machine prop).
> 
> I do agree the property is weird, but a machine property would in my opinion
> also be weird (especially form libvirt perspective, but also from qemu
> perspective e.g. compat handling and machine type none).
> 
> I used to think that using a trait type is the cleanest, but one advantage

What is a "trait type"?

> of the approach taken in this patch is that it can be introspected via command
> line (it is quite weird though).

Any property should be introspectable, no?

> 
> >>>>  }
> >>>>  
> >>>>  const VMStateDescription vmstate_ccw_dev = {
> >>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>>> index f6b5c807cd..957cb9ec90 100644
> >>>> --- a/hw/s390x/css.c
> >>>> +++ b/hw/s390x/css.c
> >>>> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
> >>>>      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;
> >>>> -        }
> >>>> -    }    
> >>>
> >>> This allows building a commandline in a compat machine that will not
> >>> work with old qemus, no?
> >>>     
> >>
> >> Yes. We have considered this. I was convinced by Christian that
> >> it ain't too bad. In the end, if we don't want non-virtual device
> >> aware domains (see above), then there is no way to make this work
> >> with libvirt. Actually to make the migration work with libvirt with
> >> old qemus the only way would be to use sqash. But libvirt does not
> >> want that.
> >>
> >> Also consider that vfio-ccw (AFAIK the only non-virtual css device
> >> type) is not migratable. So having them on the command line and live
> >> migrating is a lost case already.
> >>
> >> Yes, having a pre- 2.12 binary version and a post 2.12 binary version
> >> way to use vfio-ccw is ugly to some extent. The recommendation would
> >> be don't use it with pre 2.12 (libvirt is going to plainly refuse).
> >>
> >> And yes with this one is going to be able to write a 2.12 command
> >> line which does not work with 2.11 but that is normal.
> >>
> >> This patch keeps the squash so the 2.10 definition will still be
> >> viable for 2.12. Should we sometime get rid of the squash, then
> >> that would be a breaking change.  
> > 
> > Removing squash is easy to detect. I'm a bit worried about
> > not-obvious-at-the-start breakage.
> >   
> 
> Again won't happen with libvirt. With a direct command line user
> downgrading or moving to a qemu 2.10 or 2.11 it is a risk we can
> take IMHO.

I want to avoid setting landmines, though.

> 
> Also I can't find anything about vfio-ccw in the upstream users
> manual for 2.10.91. 

We have an "upstream users manual"?

> So I would argue using vfio-ccw is using an
> undocumented feature: if undocumented features changing in a non
> compatible way hits you, it's partly your own fault.

If it's an x- interface, it is preliminary. If not, we should avoid
breaking it.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 21 Nov 2017 19:10:15 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/21/2017 05:06 PM, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 15:45:17 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 11/21/2017 02:44 PM, Cornelia Huck wrote:  
> >>> On Tue, 21 Nov 2017 12:18:25 +0100
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>
> >>> Subject: s/unresrict/unrestrict/
> >>>     
> >>>> 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), and inform management software property on all ccw
> >>>> devices.    
> >>>
> >>> I do not really like dropping the restrictions while still keeping the
> >>> default cssid as 0xfe. Putting virtual devices into css 0 seems
> >>> completely fine, but putting non-virtual devices into css 0xfe still
> >>> feels a bit wrong. What about:
> >>>
> >>> - Add a property to specify the default cssid. Compat machines use a
> >>>   default cssid of 0xfe.
> >>> - Default to a cssid of 0.
> >>> - (optional) Warn when putting a non-virtual device into css 0xfe,
> >>>   unless it is the default css.    
> >>
> >> To make it more clear, I think the most compatible solution would be to allow
> >> vfio-ccw also in FE. (But continue to force virtual devices in FE as well).
> >> This was more or less the first proposal that we had. Then we asked ourselves
> >> why not also allow virtual devices in 0?
> >>
> >> I think your proposal of specifying a default css (and then allowing
> >> all devices in that) is actually pretty close to Halils proposal. The only
> >> difference is that Halils variant keeps fe as default css. 
> >> So I think we could even combine both approaches. Use Halils patch as a base
> >> and in addition provide a way to change the default css away from fe. Have to
> >> think about that.  
> > 
> > If keeping the default of 0xfe makes the life of management software
> > easier, sure. As it is, we seem to be far more entrenched with
> > paravirtual devices than I had expected when I first wrote this, so
> > 0xfe might be the sensible choice even if mcss-e is available in the
> > future. In this case, I think we should lift all cssid restrictions.  
> 
> So basically agree with the restriction lifting, but you want to have a
> different method of telling libvirt about it?

Yes.

> [...]
> >>> This allows building a commandline in a compat machine that will not
> >>> work with old qemus, no?    
> >>
> >> I think this is pretty common to have new devices and things that will not
> >> work on old QEMUs but are allowed on compat machines, e.g. virtio-gpu was
> >> added in 2.4 but
> >>
> >> [cborntra@oc7330422307 qemu]$ build/i386-softmmu/qemu-system-i386 -device virtio-gpu-pci -M pc-i440fx-2.2
> >> VNC server running on ::1:5900
> >>
> >> works fine  
> > 
> > It seems a bit more unexpected, though.  
> 
> What I understood from the CPU model discussion is, that the machine version basically sets the migration
> stream format and any "invisible" setting that the user cannot influence. But it is not there to the fence
> the user from specifying new devices or new CPU types (that the old QEMU does not know) or different 
> command lines. You get a compatible machine if you specify an identical command line that will work for
> both.

So we really need a good changelog entry for that one. Mind you, I'm
not really opposed; but if a trap is there, it would be good to have at
least some signage.

> > 
> > (And I'm still not quite sure how all of this interacts with the squash
> > option.)  
> 
> I think we should deprecate the squash option and not use it to simplify things.

I certainly want the squash option to go out when we do this. I just
was a bit unsure about any interactions, but it seems it is not really
problematic.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Boris Fiuczynski]

On 11/22/2017 01:13 PM, Cornelia Huck wrote:
>>>>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>>>>> +                                   prop_get_true, NULL, NULL);
>>>>> This looks really, really strange. This is a property that is always
>>>>> true if it exists.
>>>>>      
>>>> Won't argue about that. The libvirt guys are actually not interested
>>>> int he value at all: only that there is such a property.
>>> So what about a machine property? Wouldn't that help as well?
>>>    
>> Technically it is doable. The property would be still a weird
>> one, but from QEMU perspective in a less weird place. I was also
>> arguing that from OO perspective this kind of a don't care about
>> it's value property is weird, but AFAIK not having the info if
>> we can do something with a device at the device is weird from
>> Libvirt perspective. I'm really uncomforble with speaking for
>> Libvirt/Boris. Hope he will make his point tomorrow.
>>
>>> Or a css object?
>>>    
>> My first internal-only version used this approach, but
>> I was asked to do it like this.
> If we formulate this rather as "we now expose the default css", we (a)
> have something that really makes the most sense at a channel subsystem
> or machine level, and (b) can be detected by libvirt as an indicator of
> "no restriction for virtual vs. non-virtual".

I think that there are good reasons for using a device property as well 
as for using a machine property. Technically both options are possible 
(even for libvirt :) without too much rope...). At first my personal 
choice was to express the changed behavior/capability on the device 
level since that is what a user defines on the command line and also 
where he gets restricted to use a css matching his device type.
 From the libvirt perspective we would have supported vfio-ccw devices 
only if the vfio-ccw would be allowed to be defined with unrestricted 
cssids.
As I wrote before I also understand the reasoning to express the channel 
subsystem wide changed behavior as a machine option. In that case 
libvirt would need to check that both capabilities (vfio-ccw and machine 
option cssid-unrestricted) are set and not just 
vfio-cww.cssid-unrestricted. All doable. A qemu command line user would 
obviously need to know the correlation of the machine option and the ccw 
devices but that certainly is also nothing new.
When talking to Christian and Halil I started to favor the idea of a new 
css object especially when thinking about the future in which the user 
might want to specify the default css. It for sure would also be able to 
be set with the use of a machine option but a css object would at that 
point be much a nicer and more capable approach. What do you think?

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Wed, 22 Nov 2017 15:45:56 +0100
Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:

> On 11/22/2017 01:13 PM, Cornelia Huck wrote:
> >>>>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >>>>>> +                                   prop_get_true, NULL, NULL);  
> >>>>> This looks really, really strange. This is a property that is always
> >>>>> true if it exists.
> >>>>>        
> >>>> Won't argue about that. The libvirt guys are actually not interested
> >>>> int he value at all: only that there is such a property.  
> >>> So what about a machine property? Wouldn't that help as well?
> >>>      
> >> Technically it is doable. The property would be still a weird
> >> one, but from QEMU perspective in a less weird place. I was also
> >> arguing that from OO perspective this kind of a don't care about
> >> it's value property is weird, but AFAIK not having the info if
> >> we can do something with a device at the device is weird from
> >> Libvirt perspective. I'm really uncomforble with speaking for
> >> Libvirt/Boris. Hope he will make his point tomorrow.
> >>  
> >>> Or a css object?
> >>>      
> >> My first internal-only version used this approach, but
> >> I was asked to do it like this.  
> > If we formulate this rather as "we now expose the default css", we (a)
> > have something that really makes the most sense at a channel subsystem
> > or machine level, and (b) can be detected by libvirt as an indicator of
> > "no restriction for virtual vs. non-virtual".  
> 
> I think that there are good reasons for using a device property as well 
> as for using a machine property. Technically both options are possible 
> (even for libvirt :) without too much rope...). At first my personal 
> choice was to express the changed behavior/capability on the device 
> level since that is what a user defines on the command line and also 
> where he gets restricted to use a css matching his device type.
>  From the libvirt perspective we would have supported vfio-ccw devices 
> only if the vfio-ccw would be allowed to be defined with unrestricted 
> cssids.

Thanks, I can now understand where that property came from.

> As I wrote before I also understand the reasoning to express the channel 
> subsystem wide changed behavior as a machine option. In that case 
> libvirt would need to check that both capabilities (vfio-ccw and machine 
> option cssid-unrestricted) are set and not just 
> vfio-cww.cssid-unrestricted. All doable. A qemu command line user would 
> obviously need to know the correlation of the machine option and the ccw 
> devices but that certainly is also nothing new.
> When talking to Christian and Halil I started to favor the idea of a new 
> css object especially when thinking about the future in which the user 
> might want to specify the default css. It for sure would also be able to 
> be set with the use of a machine option but a css object would at that 
> point be much a nicer and more capable approach. What do you think?

I think exposing the css object and making the default_css property
available is the cleanest solution (especially if we want more css
properties in the future). The only drawback I see is that it needs
more coding (and probably care to keep it backwards compatible.)

Halil, do you think you can come up with something?

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/22/2017 05:25 PM, Cornelia Huck wrote:
> On Wed, 22 Nov 2017 15:45:56 +0100
> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
> 
>> On 11/22/2017 01:13 PM, Cornelia Huck wrote:
>>>>>>>> +    object_class_property_add_bool(klass, "cssid-unrestricted",
>>>>>>>> +                                   prop_get_true, NULL, NULL);  
>>>>>>> This looks really, really strange. This is a property that is always
>>>>>>> true if it exists.
>>>>>>>        
>>>>>> Won't argue about that. The libvirt guys are actually not interested
>>>>>> int he value at all: only that there is such a property.  
>>>>> So what about a machine property? Wouldn't that help as well?
>>>>>      
>>>> Technically it is doable. The property would be still a weird
>>>> one, but from QEMU perspective in a less weird place. I was also
>>>> arguing that from OO perspective this kind of a don't care about
>>>> it's value property is weird, but AFAIK not having the info if
>>>> we can do something with a device at the device is weird from
>>>> Libvirt perspective. I'm really uncomforble with speaking for
>>>> Libvirt/Boris. Hope he will make his point tomorrow.
>>>>  
>>>>> Or a css object?
>>>>>      
>>>> My first internal-only version used this approach, but
>>>> I was asked to do it like this.  
>>> If we formulate this rather as "we now expose the default css", we (a)
>>> have something that really makes the most sense at a channel subsystem
>>> or machine level, and (b) can be detected by libvirt as an indicator of
>>> "no restriction for virtual vs. non-virtual".  
>>
>> I think that there are good reasons for using a device property as well 
>> as for using a machine property. Technically both options are possible 
>> (even for libvirt :) without too much rope...). At first my personal 
>> choice was to express the changed behavior/capability on the device 
>> level since that is what a user defines on the command line and also 
>> where he gets restricted to use a css matching his device type.
>>  From the libvirt perspective we would have supported vfio-ccw devices 
>> only if the vfio-ccw would be allowed to be defined with unrestricted 
>> cssids.
> 
> Thanks, I can now understand where that property came from.
> 
>> As I wrote before I also understand the reasoning to express the channel 
>> subsystem wide changed behavior as a machine option. In that case 
>> libvirt would need to check that both capabilities (vfio-ccw and machine 
>> option cssid-unrestricted) are set and not just 
>> vfio-cww.cssid-unrestricted. All doable. A qemu command line user would 
>> obviously need to know the correlation of the machine option and the ccw 
>> devices but that certainly is also nothing new.
>> When talking to Christian and Halil I started to favor the idea of a new 
>> css object especially when thinking about the future in which the user 
>> might want to specify the default css. It for sure would also be able to 
>> be set with the use of a machine option but a css object would at that 
>> point be much a nicer and more capable approach. What do you think?
> 
> I think exposing the css object and making the default_css property
> available is the cleanest solution (especially if we want more css
> properties in the future). The only drawback I see is that it needs
> more coding (and probably care to keep it backwards compatible.)
> 
> Halil, do you think you can come up with something?
> 
Having an adequate representation for the css in QOM would be certainly
interesting, but at the same time (IMHO) is somewhat challenging. Let me
make some observations, which should some of my concerns. 

We already have virtual-css-bridge which is  the QOM type/object that
kind of conceptually stands for the channel subsystem as subsystem (not
for the images but for the big whole).

The virtual-css-bridge is however non-user-creatable (inherited this from
sysbus-device-type), so I doubt it can be used for specifying properties
on the command line. That is no good for something like specifying the
default css (image, not the big whole). What we usually do is pull up the
property to the machine level. But this comes at a cost.

A small digression, Christian pointed me to the -set cmd line option.
I've done a quick PoC (assigned an id in code and did the command line)
but I failed.

Furthermore the (canonical) QOM path of virtual-css-bridge is
/machine/unattached/device[2] (there is a link from sysbus but the child
property is connecting it to unattached, see [1]). I would prefer seeing
and working wit something like /machine/css/default_css.

Under the virtual-css-bridge there is a virtual-css-bus called
QOM-path-ly virtual-css, so it's canonical path is
/machine/unattached/device[2]/virtual-css. The virtual-css-bus is in qdev
a bus however. AFAIU buses are not very suitable for hosting properties
(as one can't attach/add buses directly only via devices.

Another interesting fact is that virtio-ccw devices are QOMly only linked
to their qdev parent bus, the virtual-ccs-bus, and are a QOMly children
of /machine/peripheral (see [2]).

To sum it up: I'm pretty confused with the QOM view of things.

My intuition would be that the object/device has to be somewhere between
/machine and the vrtio-ccw devices in the children graph.  And since we
want to do a command line controllable properties on the css object, the
object probably needs to be a device.

Given what we currently have I don't see a good place for this css
object.

Furthermore I'm currently under the impression that every non-abstract
device is user creatable (but some can be created only implicitly). Some
seem to go even further and say should workwith device-add [3].
 
In my opinion having a something like is_singleton (bool) with the
meaning exactly one instance of this device type is required, instead of
user_creatabe would have been more useful, and would actually be
something worth exposing to the user.

Some of our devices are such, that neither more than one, nor none makes
any sense (flic, and I think the channel subsystem (big)). With something
like this we could say QEMU is free to fill in with defaults if some of
these singletons is not specified on the command line (but can be
deduced), but the user is still in control regarding properties.

Summa summarum, coming up with a really nice solution does not appear to
be trivial if even possible.

If we think less than nice, then having an object object under /objects
(something like iothread) which could be called css_config is something
that pops up in my mind. The way to specify the default css on the
command line would look something like --object
ccs-config,default-css=0xfe I guess. I'm not sure it's less weird then
then a machine property, and the same goes for the QOM path. Furthermore,
we would probably have to make this a singleton (in a sense described
above) if we want to have a consistent interface for querying.

Is what I described the 'css object' you have been asking for or did
I misunderstand? If it is, I really don't have the feeling it would be
nicer nor simpler than what we have in this patch.

I'm not that familiar with how object-objects work in qemu, and honestly
I got curious, so I will explore this. Even if just for the sake of
learning.  But frankly, right now this does not appear simpler or more
elegant than the current solution plus a machine property for the default
css id.

My overall impression is: I'm pretty confused by our QOM. That's why I
tried to avoid getting too deeply involved with it. Currently, the
approach taken in this patch seems to be the least painful option.
Especially if deeper-going changes (qom paths, singletons, whatever) are
not desirable.

And since I'm not convinced we need to set the default css I think
worrying about setting css properties when the need arises is viable at
this moment.

If you can help me getting rid of some of the confusion, and gain
a better understanding of our QOM modeling, that would be highly
appreciated.


Halil

References:
[1]
(qemu) qom-list /machine/unattached/
io[0] (child<qemu:memory-region>)
system[0] (child<qemu:memory-region>)
device[2] (child<virtual-css-bridge>)

[2]
(qemu) qom-list /machine/unattached/device[2]/virtual-css
type (string)
child[1] (link<virtio-blk-ccw>)
child[0] (link<virtio-blk-ccw>)
hotplug-handler (link<hotplug-handler>)
realized (bool)


[3]
   /*
     * Can this device be instantiated with -device / device_add?
     * All devices should support instantiation with device_add, and
     * this flag should not exist.  But we're not there, yet.  Some
     * devices fail to instantiate with cryptic error messages.
     * Others instantiate, but don't work.  Exposing users to such
     * behavior would be cruel; clearing this flag will protect them.
     * It should never be cleared without a comment explaining why it
     * is cleared.
     * TODO remove once we're there
     */
    bool user_creatable;

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/22/2017 01:13 PM, Cornelia Huck wrote:
[..]

>> The original question was about weather keep the start putting
>> non-virtual devices into (the non-guest-visible) 0 if no devno is
>> specified, or rather fill the default first and only then spill
>> to the next css.
> 
> Combined with what I said right below, I think we should be fine
> autogenerating into the default css. Anything else will just generate
> unusable configurations when the squash parameter is gone.
> 

OK. Will think about how to address this for v2 (separate patch or
part of this).

[..]

>>>
>>> So what about a machine property? Wouldn't that help as well?
>>>   
>>
>> Technically it is doable. The property would be still a weird
>> one, but from QEMU perspective in a less weird place. I was also
>> arguing that from OO perspective this kind of a don't care about
>> it's value property is weird, but AFAIK not having the info if
>> we can do something with a device at the device is weird from
>> Libvirt perspective. I'm really uncomforble with speaking for
>> Libvirt/Boris. Hope he will make his point tomorrow.
>>
>>> Or a css object?

This seems to be a concurrent discussion. Please see my other email.

>>>   
>>
>> My first internal-only version used this approach, but
>> I was asked to do it like this.
> 
> If we formulate this rather as "we now expose the default css", we (a)
> have something that really makes the most sense at a channel subsystem
> or machine level, and (b) can be detected by libvirt as an indicator of
> "no restriction for virtual vs. non-virtual".
> 

My previous statement was not entirely correct. I used a QOM type
but not to represent the css but this particular (cssids unrestricred
trait of the css).


>>
>>>>  
>>>>> What about compat machines? This qemu won't have the restriction, but
>>>>> old qemus will.
>>>>>     
>>>>
>>>> Very true. But as the commit message implies it should not be a problem.  
>>>
>>> How is that supposed to play with libvirt detection, then?
>>>   
>>
>> As written above. Libvirt will simply refuse to use vfio-ccw if the property
>> is not defined. This results in no prior history to care about for libvirt
>> users: vfio-ccw effectively becomes available with qemu 2.12.
> 
> I'm not worried about vfio-ccw. As you said, this should work. We just
> need to make sure that we don't break existing setups. (I don't think
> we will.)
> 

OK.

>>
>>>>  
>>>>> Also, I'd consider this a property of the machine, not of the
>>>>> individual devices. Or of the ChannelSubsystem structure, which is not
>>>>> qom'ified.
>>>>>     
>>>>
>>>> I've said the exact same thing to Boris. He said from libvirt perspective
>>>> a device property is better.
>>>>  
>>>>> As an alternative, I think providing a machine default_cssid parameter
>>>>> makes more sense: It is understandable, and you keep compatibility. I
>>>>> think we want this in the long run anyway.
>>>>>     
>>>>
>>>> I think most of us had the idea. I support this idea fully (expose default
>>>> cssid to the user (rw)). I see it as something that can be done on top
>>>> and is not a pressing issue, but rather a nice to have.  
>>>
>>> TBH, this weird property is what I like least about this patch.
>>>   
>>
>> I'm fine with any of the 3 alternatives (as is, new type, new machine prop).
>>
>> I do agree the property is weird, but a machine property would in my opinion
>> also be weird (especially form libvirt perspective, but also from qemu
>> perspective e.g. compat handling and machine type none).
>>
>> I used to think that using a trait type is the cleanest, but one advantage
> 
> What is a "trait type"?
> 

In code I did something like this:
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -961,3 +961,24 @@ static void ccw_machine_register_types(void)
 }

 type_init(ccw_machine_register_types)
+
+static const TypeInfo  s390_trait = {
+    .name =  TYPE_S390_TRAIT,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+};
+
+
+static const TypeInfo  cssids_unrestricted_info = {
+    .name = "s390-cssids-unresticted",
+    .parent = TYPE_S390_TRAIT,
+    .abstract = true,
+};
+
+static void css_register_traits(void)
+{
+    type_register_static(&s390_trait);
+    type_register_static(&cssids_unrestricted_info);
+}
+
+type_init(css_register_traits);

So management software could do something like
{"execute": "qom-list-types", "arguments" : {"abstract": true, "implements": "s390-trait"}

To figure out the traits (on binary basis). If "s390-cssids-unresticted"
pops up it means the restriction is no more (for the binary).

Since it is just a type there is no interaction with machine type 'none'
used for probing, no need to instantiate an object to read a property,
or just assume it's value is true, and no confusion about why does the
property pop up at multiple places (all virtio-ccw machine derived machines
or all ccw-device derived devices).

It's just basically tagging the qemu binary with a type. Since we
are doing something non-standard (kind of breaking compatibility)
I figured expressing it in a non-standard way may not be that bad.

I was eventually convinced that it is too strange. And command line
users would be completely unaware, which isn't necessarily a good thing.

(About trait: https://www.merriam-webster.com/dictionary/trait
and more specifically in CS it's a pretty commonly used idiom
in c++ template meta-programming for doing compile time conditional stuff
based on type parameters and their properties. )

>> of the approach taken in this patch is that it can be introspected via command
>> line (it is quite weird though).
> 
> Any property should be introspectable, no?
> 

Yes, the existence. The value AFAIK not via the command line.

>>
>>>>>>  }
>>>>>>  
>>>>>>  const VMStateDescription vmstate_ccw_dev = {
>>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>>>> index f6b5c807cd..957cb9ec90 100644
>>>>>> --- a/hw/s390x/css.c
>>>>>> +++ b/hw/s390x/css.c
>>>>>> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>>>>>>      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;
>>>>>> -        }
>>>>>> -    }    
>>>>>
>>>>> This allows building a commandline in a compat machine that will not
>>>>> work with old qemus, no?
>>>>>     
>>>>
>>>> Yes. We have considered this. I was convinced by Christian that
>>>> it ain't too bad. In the end, if we don't want non-virtual device
>>>> aware domains (see above), then there is no way to make this work
>>>> with libvirt. Actually to make the migration work with libvirt with
>>>> old qemus the only way would be to use sqash. But libvirt does not
>>>> want that.
>>>>
>>>> Also consider that vfio-ccw (AFAIK the only non-virtual css device
>>>> type) is not migratable. So having them on the command line and live
>>>> migrating is a lost case already.
>>>>
>>>> Yes, having a pre- 2.12 binary version and a post 2.12 binary version
>>>> way to use vfio-ccw is ugly to some extent. The recommendation would
>>>> be don't use it with pre 2.12 (libvirt is going to plainly refuse).
>>>>
>>>> And yes with this one is going to be able to write a 2.12 command
>>>> line which does not work with 2.11 but that is normal.
>>>>
>>>> This patch keeps the squash so the 2.10 definition will still be
>>>> viable for 2.12. Should we sometime get rid of the squash, then
>>>> that would be a breaking change.  
>>>
>>> Removing squash is easy to detect. I'm a bit worried about
>>> not-obvious-at-the-start breakage.
>>>   
>>
>> Again won't happen with libvirt. With a direct command line user
>> downgrading or moving to a qemu 2.10 or 2.11 it is a risk we can
>> take IMHO.
> 
> I want to avoid setting landmines, though.
> 

That's humane. I think doing full compat handling for the unrestrict
is also doable: Introduce a machine property that is going to be
false for less equal 2.11 and true for greater. And make the check
conditional on that property. The question is what do we gain by
that except having more code around that can makes only sense if
one knows the history behind it.

When I wrote my internal RFC the idea was maybe we can pretend
nobody ever used either vfio-ccw nor a ccsid other than 0xfe. The
idea seemed to resonate with the people.

Do you think we can get around full compat handling? Because if
we have to do that, then IMHO the property needs to be at the
machine. And is probably best implemented as an user r/w property.

My current status is that you and Christian agreed that we don't
want compat handling, but better sure than sorry.

>>
>> Also I can't find anything about vfio-ccw in the upstream users
>> manual for 2.10.91. 
> 
> We have an "upstream users manual"?
> 
This is what I mean:
https://qemu.weilnetz.de/doc/qemu-doc.html

>> So I would argue using vfio-ccw is using an
>> undocumented feature: if undocumented features changing in a non
>> compatible way hits you, it's partly your own fault.
> 
> If it's an x- interface, it is preliminary. If not, we should avoid
> breaking it.
> 

I don't see this documented in the users manual? Shouldn't the users
know what is preliminary?

Where is this documented?

Regards,
Halil

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Thu, 23 Nov 2017 17:09:16 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

[I need time to process your other remarks.]

> On 11/22/2017 01:13 PM, Cornelia Huck wrote:

> >> Also I can't find anything about vfio-ccw in the upstream users
> >> manual for 2.10.91.   
> > 
> > We have an "upstream users manual"?
> >   
> This is what I mean:
> https://qemu.weilnetz.de/doc/qemu-doc.html

This is really hit-and-miss. Some things are documented in there, some
are not. I usually consider the output of the binary the canonical list
of supported devices (in combination with the x- notation.)

> 
> >> So I would argue using vfio-ccw is using an
> >> undocumented feature: if undocumented features changing in a non
> >> compatible way hits you, it's partly your own fault.  
> > 
> > If it's an x- interface, it is preliminary. If not, we should avoid
> > breaking it.
> >   
> 
> I don't see this documented in the users manual? Shouldn't the users
> know what is preliminary?
> 
> Where is this documented?

It is long standing practice, but I cannot point to documentation
(maybe someone else can?)

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Thu, 23 Nov 2017 14:33:56 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Having an adequate representation for the css in QOM would be certainly
> interesting, but at the same time (IMHO) is somewhat challenging. Let me
> make some observations, which should some of my concerns. 
> 
> We already have virtual-css-bridge which is  the QOM type/object that
> kind of conceptually stands for the channel subsystem as subsystem (not
> for the images but for the big whole).
> 
> The virtual-css-bridge is however non-user-creatable (inherited this from
> sysbus-device-type), so I doubt it can be used for specifying properties
> on the command line. That is no good for something like specifying the
> default css (image, not the big whole). What we usually do is pull up the
> property to the machine level. But this comes at a cost.

It might be better to not have inherit this from sysbus (at the price
of doing some stuff like reset handling ourselves.) Not sure if that
helps for this concrete issue, though.

> 
> A small digression, Christian pointed me to the -set cmd line option.
> I've done a quick PoC (assigned an id in code and did the command line)
> but I failed.
> 
> Furthermore the (canonical) QOM path of virtual-css-bridge is
> /machine/unattached/device[2] (there is a link from sysbus but the child
> property is connecting it to unattached, see [1]). I would prefer seeing
> and working wit something like /machine/css/default_css.

That's because the parent reference is not set up before registering
the device. It should do a object_property_add_child() (as e.g. the
flic does) so that it will show up as attached to the machine.

> 
> Under the virtual-css-bridge there is a virtual-css-bus called
> QOM-path-ly virtual-css, so it's canonical path is
> /machine/unattached/device[2]/virtual-css. The virtual-css-bus is in qdev
> a bus however. AFAIU buses are not very suitable for hosting properties
> (as one can't attach/add buses directly only via devices.
> 
> Another interesting fact is that virtio-ccw devices are QOMly only linked
> to their qdev parent bus, the virtual-ccs-bus, and are a QOMly children
> of /machine/peripheral (see [2]).

That's true of anything added via qdev_device_add().

[As an aside, that's why the autogenerated net devices show up as
unattached as well. Not sure if it is worth doing anything about it.]

> 
> To sum it up: I'm pretty confused with the QOM view of things.
> 
> My intuition would be that the object/device has to be somewhere between
> /machine and the vrtio-ccw devices in the children graph.  And since we
> want to do a command line controllable properties on the css object, the
> object probably needs to be a device.
> 
> Given what we currently have I don't see a good place for this css
> object.

I don't think qom is supposed to map the hardware hierarchy (I might be
mistaken, though.) I don't see anything speaking against a css object
attached to the machine (if we can't use the css bridge for that
purpose.)

> 
> Furthermore I'm currently under the impression that every non-abstract
> device is user creatable (but some can be created only implicitly). Some
> seem to go even further and say should workwith device-add [3].

There are quite a number of devices for which being user-createable
does not make any sense at all.

What _could_ make sense is having a way to add css images manually and
having specify whether they are default. (Obviously not when they are
hotplugged.) For compatibility, we could create css image 0xfe
automatically as default if none has been defined. Not sure how easy it
would be to get this right, though.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/24/2017 01:46 PM, Cornelia Huck wrote:
> On Thu, 23 Nov 2017 14:33:56 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Having an adequate representation for the css in QOM would be certainly
>> interesting, but at the same time (IMHO) is somewhat challenging. Let me
>> make some observations, which should some of my concerns. 
>>
>> We already have virtual-css-bridge which is  the QOM type/object that
>> kind of conceptually stands for the channel subsystem as subsystem (not
>> for the images but for the big whole).
>>
>> The virtual-css-bridge is however non-user-creatable (inherited this from
>> sysbus-device-type), so I doubt it can be used for specifying properties
>> on the command line. That is no good for something like specifying the
>> default css (image, not the big whole). What we usually do is pull up the
>> property to the machine level. But this comes at a cost.
> 
> It might be better to not have inherit this from sysbus (at the price
> of doing some stuff like reset handling ourselves.) Not sure if that
> helps for this concrete issue, though.
> 
>>
>> A small digression, Christian pointed me to the -set cmd line option.
>> I've done a quick PoC (assigned an id in code and did the command line)
>> but I failed.
>>
>> Furthermore the (canonical) QOM path of virtual-css-bridge is
>> /machine/unattached/device[2] (there is a link from sysbus but the child
>> property is connecting it to unattached, see [1]). I would prefer seeing
>> and working wit something like /machine/css/default_css.
> 
> That's because the parent reference is not set up before registering
> the device. It should do a object_property_add_child() (as e.g. the
> flic does) so that it will show up as attached to the machine.
> 
>>
>> Under the virtual-css-bridge there is a virtual-css-bus called
>> QOM-path-ly virtual-css, so it's canonical path is
>> /machine/unattached/device[2]/virtual-css. The virtual-css-bus is in qdev
>> a bus however. AFAIU buses are not very suitable for hosting properties
>> (as one can't attach/add buses directly only via devices.
>>
>> Another interesting fact is that virtio-ccw devices are QOMly only linked
>> to their qdev parent bus, the virtual-ccs-bus, and are a QOMly children
>> of /machine/peripheral (see [2]).
> 
> That's true of anything added via qdev_device_add().
> 
> [As an aside, that's why the autogenerated net devices show up as
> unattached as well. Not sure if it is worth doing anything about it.]
> 
>>
>> To sum it up: I'm pretty confused with the QOM view of things.
>>
>> My intuition would be that the object/device has to be somewhere between
>> /machine and the vrtio-ccw devices in the children graph.  And since we
>> want to do a command line controllable properties on the css object, the
>> object probably needs to be a device.
>>
>> Given what we currently have I don't see a good place for this css
>> object.
> 
> I don't think qom is supposed to map the hardware hierarchy (I might be
> mistaken, though.) I don't see anything speaking against a css object
> attached to the machine (if we can't use the css bridge for that
> purpose.)
> 
>>
>> Furthermore I'm currently under the impression that every non-abstract
>> device is user creatable (but some can be created only implicitly). Some
>> seem to go even further and say should workwith device-add [3].
> 
> There are quite a number of devices for which being user-createable
> does not make any sense at all.
> 
> What _could_ make sense is having a way to add css images manually and
> having specify whether they are default. (Obviously not when they are
> hotplugged.) For compatibility, we could create css image 0xfe
> automatically as default if none has been defined. Not sure how easy it
> would be to get this right, though.

I first liked the idea to have it as a property of the css, but 
this is all pretty unclear how to do right. I start to think that going with
Halils first patch (a property per virtio device) is going to be the most
simple solution without causing any harm. After all as of today we only want 
to have a way to tell libvirt that devices can be everywhere. Specifying the
default css might be something that we want to have in the future, but here
future might even mean never.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Fri, 24 Nov 2017 14:01:20 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> I first liked the idea to have it as a property of the css, but 
> this is all pretty unclear how to do right. I start to think that going with
> Halils first patch (a property per virtio device) is going to be the most
> simple solution without causing any harm. After all as of today we only want 
> to have a way to tell libvirt that devices can be everywhere. Specifying the
> default css might be something that we want to have in the future, but here
> future might even mean never.

I still don't like the idea of a per-device property, but I agree that
adding a css property would need too much discussion to get to a
solution in the near future.

Is there anything that speaks against a machine property, though? While
not ideal, I like it better than the per-device one.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/24/2017 02:27 PM, Cornelia Huck wrote:
> On Fri, 24 Nov 2017 14:01:20 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> I first liked the idea to have it as a property of the css, but 
>> this is all pretty unclear how to do right. I start to think that going with
>> Halils first patch (a property per virtio device) is going to be the most
>> simple solution without causing any harm. After all as of today we only want 
>> to have a way to tell libvirt that devices can be everywhere. Specifying the
>> default css might be something that we want to have in the future, but here
>> future might even mean never.
> 
> I still don't like the idea of a per-device property, but I agree that
> adding a css property would need too much discussion to get to a
> solution in the near future.
> 
> Is there anything that speaks against a machine property, though? While
> not ideal, I like it better than the per-device one.

In theory this should work. 

In reality it seems more complicated. A per-device property is easy and can be
inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
machine property would require to change the qemu help output and qemu-options 
file (which makes it visible for all architectures). Not sure if there are
easier ways to do it. (e.g. QOM-only things, but then we have no command line
way of doing it)

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/24/2017 03:58 PM, Christian Borntraeger wrote:
> 
> 
> On 11/24/2017 02:27 PM, Cornelia Huck wrote:
>> On Fri, 24 Nov 2017 14:01:20 +0100
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> I first liked the idea to have it as a property of the css, but 
>>> this is all pretty unclear how to do right. I start to think that going with
>>> Halils first patch (a property per virtio device) is going to be the most
>>> simple solution without causing any harm. After all as of today we only want 
>>> to have a way to tell libvirt that devices can be everywhere. Specifying the
>>> default css might be something that we want to have in the future, but here
>>> future might even mean never.
>>
>> I still don't like the idea of a per-device property, but I agree that
>> adding a css property would need too much discussion to get to a
>> solution in the near future.
>>
>> Is there anything that speaks against a machine property, though? While
>> not ideal, I like it better than the per-device one.
> 
> In theory this should work. 
> 
> In reality it seems more complicated. A per-device property is easy and can be
> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
> machine property would require to change the qemu help output and qemu-options 
> file (which makes it visible for all architectures).

And then we have the fun of describing, that this property is weird, and can
not be set, and it's value does not matter.

BTW one can do -machine s390-ccw-virtio-2.11,help and get a list of properties,
so the command line introspection would work similar.

I've already brought some other arguments against the machine property.
Won't repeat them here.

> Not sure if there are
> easier ways to do it. (e.g. QOM-only things, but then we have no command line
> way of doing it)
> 
> 
 
If we don't care about if it can be introspected on the command line my
trait type approach is pretty minimal. But the least surprise is probably
still the device property (IMHO).

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Fri, 24 Nov 2017 16:30:24 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/24/2017 03:58 PM, Christian Borntraeger wrote:
> > 
> > 
> > On 11/24/2017 02:27 PM, Cornelia Huck wrote:  
> >> On Fri, 24 Nov 2017 14:01:20 +0100
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>  
> >>> I first liked the idea to have it as a property of the css, but 
> >>> this is all pretty unclear how to do right. I start to think that going with
> >>> Halils first patch (a property per virtio device) is going to be the most
> >>> simple solution without causing any harm. After all as of today we only want 
> >>> to have a way to tell libvirt that devices can be everywhere. Specifying the
> >>> default css might be something that we want to have in the future, but here
> >>> future might even mean never.  
> >>
> >> I still don't like the idea of a per-device property, but I agree that
> >> adding a css property would need too much discussion to get to a
> >> solution in the near future.
> >>
> >> Is there anything that speaks against a machine property, though? While
> >> not ideal, I like it better than the per-device one.  
> > 
> > In theory this should work. 
> > 
> > In reality it seems more complicated. A per-device property is easy and can be
> > inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
> > machine property would require to change the qemu help output and qemu-options 
> > file (which makes it visible for all architectures).  
> 
> And then we have the fun of describing, that this property is weird, and can
> not be set, and it's value does not matter.

Well, that's the case for both, no?

(Unless we simply make this a "default cssid" prop after all - then it
would be more than just a simple indication for libvirt...)

> 
> BTW one can do -machine s390-ccw-virtio-2.11,help and get a list of properties,
> so the command line introspection would work similar.
> 
> I've already brought some other arguments against the machine property.
> Won't repeat them here.

I have not read them yet.

> 
> > Not sure if there are
> > easier ways to do it. (e.g. QOM-only things, but then we have no command line
> > way of doing it)
> > 
> >   
>  
> If we don't care about if it can be introspected on the command line my
> trait type approach is pretty minimal. But the least surprise is probably
> still the device property (IMHO).

I think it's the other way around.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/24/2017 05:15 PM, Cornelia Huck wrote:
>>> In theory this should work. 
>>>
>>> In reality it seems more complicated. A per-device property is easy and can be
>>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
>>> machine property would require to change the qemu help output and qemu-options 
>>> file (which makes it visible for all architectures).  
>> And then we have the fun of describing, that this property is weird, and can
>> not be set, and it's value does not matter.
> Well, that's the case for both, no?


I don't think we have to document _device_ properites in qemu-options.hx
I don't see any documented neither for virtio-ccw nor for vfio-ccw. The
machine properties, on the contrary, are documented in this file.
> 
> (Unless we simply make this a "default cssid" prop after all - then it
> would be more than just a simple indication for libvirt...)
> 

We are now talking about the "cssid-unrestricted" property. The default
cssid is not something I would like to do any time soon.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-11-24 17:39:04 +0100]:

> 
> 
> On 11/24/2017 05:15 PM, Cornelia Huck wrote:
> >>> In theory this should work. 
> >>>
> >>> In reality it seems more complicated. A per-device property is easy and can be
> >>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
> >>> machine property would require to change the qemu help output and qemu-options 
> >>> file (which makes it visible for all architectures).  
> >> And then we have the fun of describing, that this property is weird, and can
> >> not be set, and it's value does not matter.
> > Well, that's the case for both, no?
> 
> 
> I don't think we have to document _device_ properites in qemu-options.hx
> I don't see any documented neither for virtio-ccw nor for vfio-ccw. The
> machine properties, on the contrary, are documented in this file.
Is it sane and possible to reuse the existing s390-squash-mcss property
to achieve the goal?  I mean, when it is false (which is the default
value), can we treat it as "we are allowed to put devices everywhere"?
Then we'd have the way to use a property of the -M to tell libvirt that
devices can be everywhere?

However then we can not drop it completely I guess, since Libvirt will
depends on it. But we can ignore the operation of setting it's value to
true.

> > 
> > (Unless we simply make this a "default cssid" prop after all - then it
> > would be more than just a simple indication for libvirt...)
> > 
> 
> We are now talking about the "cssid-unrestricted" property. The default
> cssid is not something I would like to do any time soon.

-- 
Dong Jia Shi

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Fri, 24 Nov 2017 17:39:04 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/24/2017 05:15 PM, Cornelia Huck wrote:
> >>> In theory this should work. 
> >>>
> >>> In reality it seems more complicated. A per-device property is easy and can be
> >>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
> >>> machine property would require to change the qemu help output and qemu-options 
> >>> file (which makes it visible for all architectures).    
> >> And then we have the fun of describing, that this property is weird, and can
> >> not be set, and it's value does not matter.  
> > Well, that's the case for both, no?  
> 
> 
> I don't think we have to document _device_ properites in qemu-options.hx
> I don't see any documented neither for virtio-ccw nor for vfio-ccw. The
> machine properties, on the contrary, are documented in this file.

But not having to describe it does not make it any less weird.

> > 
> > (Unless we simply make this a "default cssid" prop after all - then it
> > would be more than just a simple indication for libvirt...)
> >   
> 
> We are now talking about the "cssid-unrestricted" property. The default
> cssid is not something I would like to do any time soon.

What's so bad about this? As said above, I think it would be much more
useful. If libvirt can detect r/o vs. r/w for properties, we can simply
start out with a r/o variant now...

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Mon, 27 Nov 2017 10:20:56 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-11-24 17:39:04 +0100]:
> 
> > 
> > 
> > On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
> > >>> In theory this should work. 
> > >>>
> > >>> In reality it seems more complicated. A per-device property is easy and can be
> > >>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
> > >>> machine property would require to change the qemu help output and qemu-options 
> > >>> file (which makes it visible for all architectures).    
> > >> And then we have the fun of describing, that this property is weird, and can
> > >> not be set, and it's value does not matter.  
> > > Well, that's the case for both, no?  
> > 
> > 
> > I don't think we have to document _device_ properites in qemu-options.hx
> > I don't see any documented neither for virtio-ccw nor for vfio-ccw. The
> > machine properties, on the contrary, are documented in this file.  
> Is it sane and possible to reuse the existing s390-squash-mcss property
> to achieve the goal?  I mean, when it is false (which is the default
> value), can we treat it as "we are allowed to put devices everywhere"?
> Then we'd have the way to use a property of the -M to tell libvirt that
> devices can be everywhere?
> 
> However then we can not drop it completely I guess, since Libvirt will
> depends on it. But we can ignore the operation of setting it's value to
> true.

I don't think we should reuse it, as it would have rather confusing
semantics (which can't be easily sorted out unless you check for the
qemu version).

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/27/2017 01:56 PM, Cornelia Huck wrote:
> On Fri, 24 Nov 2017 17:39:04 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:
>>>>> In theory this should work. 
>>>>>
>>>>> In reality it seems more complicated. A per-device property is easy and can be
>>>>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
>>>>> machine property would require to change the qemu help output and qemu-options 
>>>>> file (which makes it visible for all architectures).    
>>>> And then we have the fun of describing, that this property is weird, and can
>>>> not be set, and it's value does not matter.  
>>> Well, that's the case for both, no?  
>>
>>
>> I don't think we have to document _device_ properites in qemu-options.hx
>> I don't see any documented neither for virtio-ccw nor for vfio-ccw. The
>> machine properties, on the contrary, are documented in this file.
> 
> But not having to describe it does not make it any less weird.
> 

The user won't make big eyes should she read the documentation. The
weirdness isn't less but is less exposed.

>>>
>>> (Unless we simply make this a "default cssid" prop after all - then it
>>> would be more than just a simple indication for libvirt...)
>>>   
>>
>> We are now talking about the "cssid-unrestricted" property. The default
>> cssid is not something I would like to do any time soon.
> 
> What's so bad about this? As said above, I think it would be much more
> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
> start out with a r/o variant now...
> 

I'm not sure I understand you. Are you proposing the following:
Drop the restriction, but don't indicate this via a read only
"cssid-unrestricted" device property but via a "default-css"
read only machine property.

Libvirt then should know that if "default-css" is present then
we don't have this virtual into 0xfe and non virtual into 0xfd
restriction any more.

Please confirm that I understood correctly or describe your
idea more verbosely.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Mon, 27 Nov 2017 14:11:57 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/27/2017 01:56 PM, Cornelia Huck wrote:
> > On Fri, 24 Nov 2017 17:39:04 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  

> >>> (Unless we simply make this a "default cssid" prop after all - then it
> >>> would be more than just a simple indication for libvirt...)
> >>>     
> >>
> >> We are now talking about the "cssid-unrestricted" property. The default
> >> cssid is not something I would like to do any time soon.  
> > 
> > What's so bad about this? As said above, I think it would be much more
> > useful. If libvirt can detect r/o vs. r/w for properties, we can simply
> > start out with a r/o variant now...
> >   
> 
> I'm not sure I understand you. Are you proposing the following:
> Drop the restriction, but don't indicate this via a read only
> "cssid-unrestricted" device property but via a "default-css"
> read only machine property.
> 
> Libvirt then should know that if "default-css" is present then
> we don't have this virtual into 0xfe and non virtual into 0xfd
> restriction any more.

Yes.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/27/2017 02:19 PM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 14:11:57 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:
>>> On Fri, 24 Nov 2017 17:39:04 +0100
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
> 
>>>>> (Unless we simply make this a "default cssid" prop after all - then it
>>>>> would be more than just a simple indication for libvirt...)
>>>>>     
>>>>
>>>> We are now talking about the "cssid-unrestricted" property. The default
>>>> cssid is not something I would like to do any time soon.  
>>>
>>> What's so bad about this? As said above, I think it would be much more
>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
>>> start out with a r/o variant now...
>>>   
>>
>> I'm not sure I understand you. Are you proposing the following:
>> Drop the restriction, but don't indicate this via a read only
>> "cssid-unrestricted" device property but via a "default-css"
>> read only machine property.
>>
>> Libvirt then should know that if "default-css" is present then
>> we don't have this virtual into 0xfe and non virtual into 0xfd
>> restriction any more.
> 
> Yes.

Unless we implement the ability to set the default css _NOW_, I would
prefer to not couple different things (ability to change default-css
and abiltity to move devices around). Otherwise we might need to change
things again if we find out that our plan does not work out.

So what about having
- a cssid-unrestricted machine property that shows that devices can move around
- later a default-css machine property when we actually implement that

?

Christian

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/27/2017 02:19 PM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 14:11:57 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:
>>> On Fri, 24 Nov 2017 17:39:04 +0100
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
> 
>>>>> (Unless we simply make this a "default cssid" prop after all - then it
>>>>> would be more than just a simple indication for libvirt...)
>>>>>     
>>>>
>>>> We are now talking about the "cssid-unrestricted" property. The default
>>>> cssid is not something I would like to do any time soon.  
>>>
>>> What's so bad about this? As said above, I think it would be much more
>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
>>> start out with a r/o variant now...
>>>   
>>
>> I'm not sure I understand you. Are you proposing the following:
>> Drop the restriction, but don't indicate this via a read only
>> "cssid-unrestricted" device property but via a "default-css"
>> read only machine property.
>>
>> Libvirt then should know that if "default-css" is present then
>> we don't have this virtual into 0xfe and non virtual into 0xfd
>> restriction any more.
> 
> Yes.
> 

@Boris:
Does this work for you? Technically it's equivalent to having
"cssid-unrestricted" at machine level.

I don't really like the idea of indicating unrestricted via
something mostly unrelated (at least for me it ain't obvious that
having the id of the default css exposed means we got rid of the
limitation.). But I'm tied of discussing this, so I'm willing to
compromise.

Halil

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/27/2017 03:03 PM, Christian Borntraeger wrote:
> 
> 
> On 11/27/2017 02:19 PM, Cornelia Huck wrote:
>> On Mon, 27 Nov 2017 14:11:57 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:
>>>> On Fri, 24 Nov 2017 17:39:04 +0100
>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>   
>>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
>>
>>>>>> (Unless we simply make this a "default cssid" prop after all - then it
>>>>>> would be more than just a simple indication for libvirt...)
>>>>>>     
>>>>>
>>>>> We are now talking about the "cssid-unrestricted" property. The default
>>>>> cssid is not something I would like to do any time soon.  
>>>>
>>>> What's so bad about this? As said above, I think it would be much more
>>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
>>>> start out with a r/o variant now...
>>>>   
>>>
>>> I'm not sure I understand you. Are you proposing the following:
>>> Drop the restriction, but don't indicate this via a read only
>>> "cssid-unrestricted" device property but via a "default-css"
>>> read only machine property.
>>>
>>> Libvirt then should know that if "default-css" is present then
>>> we don't have this virtual into 0xfe and non virtual into 0xfd
>>> restriction any more.
>>
>> Yes.
> 
> Unless we implement the ability to set the default css _NOW_, I would
> prefer to not couple different things (ability to change default-css
> and abiltity to move devices around). Otherwise we might need to change
> things again if we find out that our plan does not work out.
> 
> So what about having
> - a cssid-unrestricted machine property that shows that devices can move around
> - later a default-css machine property when we actually implement that
> 
> ?
> 

I can -- compromise and -- live with that. I'm still not convinced
that having the property at the machines is better. But I don't think,
I can convince Connie that having it at the devices is better. I would
like to get this done.

> Christian
> 
> 

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Boris Fiuczynski]

On 11/27/2017 03:13 PM, Halil Pasic wrote:
> 
> 
> On 11/27/2017 02:19 PM, Cornelia Huck wrote:
>> On Mon, 27 Nov 2017 14:11:57 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:
>>>> On Fri, 24 Nov 2017 17:39:04 +0100
>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>    
>>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:
>>
>>>>>> (Unless we simply make this a "default cssid" prop after all - then it
>>>>>> would be more than just a simple indication for libvirt...)
>>>>>>      
>>>>>
>>>>> We are now talking about the "cssid-unrestricted" property. The default
>>>>> cssid is not something I would like to do any time soon.
>>>>
>>>> What's so bad about this? As said above, I think it would be much more
>>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
>>>> start out with a r/o variant now...
>>>>    
>>>
>>> I'm not sure I understand you. Are you proposing the following:
>>> Drop the restriction, but don't indicate this via a read only
>>> "cssid-unrestricted" device property but via a "default-css"
>>> read only machine property.
>>>
>>> Libvirt then should know that if "default-css" is present then
>>> we don't have this virtual into 0xfe and non virtual into 0xfd
>>> restriction any more.
>>
>> Yes.
>>
> 
> @Boris:
> Does this work for you? Technically it's equivalent to having
> "cssid-unrestricted" at machine level.
> 
> I don't really like the idea of indicating unrestricted via
> something mostly unrelated (at least for me it ain't obvious that
> having the id of the default css exposed means we got rid of the
> limitation.). But I'm tied of discussing this, so I'm willing to
> compromise.
> 
> Halil
> 
I agree with Christian that we should not throw the "setting of a 
default cssid" together with "unrestricted cssid" and than use read/only 
and read/write to distinguish between the two!

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Mon, 27 Nov 2017 16:09:09 +0100
Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:

> On 11/27/2017 03:13 PM, Halil Pasic wrote:
> > 
> > 
> > On 11/27/2017 02:19 PM, Cornelia Huck wrote:  
> >> On Mon, 27 Nov 2017 14:11:57 +0100
> >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>  
> >>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:  
> >>>> On Fri, 24 Nov 2017 17:39:04 +0100
> >>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>>      
> >>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
> >>  
> >>>>>> (Unless we simply make this a "default cssid" prop after all - then it
> >>>>>> would be more than just a simple indication for libvirt...)
> >>>>>>        
> >>>>>
> >>>>> We are now talking about the "cssid-unrestricted" property. The default
> >>>>> cssid is not something I would like to do any time soon.  
> >>>>
> >>>> What's so bad about this? As said above, I think it would be much more
> >>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
> >>>> start out with a r/o variant now...
> >>>>      
> >>>
> >>> I'm not sure I understand you. Are you proposing the following:
> >>> Drop the restriction, but don't indicate this via a read only
> >>> "cssid-unrestricted" device property but via a "default-css"
> >>> read only machine property.
> >>>
> >>> Libvirt then should know that if "default-css" is present then
> >>> we don't have this virtual into 0xfe and non virtual into 0xfd
> >>> restriction any more.  
> >>
> >> Yes.
> >>  
> > 
> > @Boris:
> > Does this work for you? Technically it's equivalent to having
> > "cssid-unrestricted" at machine level.
> > 
> > I don't really like the idea of indicating unrestricted via
> > something mostly unrelated (at least for me it ain't obvious that
> > having the id of the default css exposed means we got rid of the
> > limitation.). But I'm tied of discussing this, so I'm willing to
> > compromise.
> > 
> > Halil
> >   
> I agree with Christian that we should not throw the "setting of a 
> default cssid" together with "unrestricted cssid" and than use read/only 
> and read/write to distinguish between the two!
> 

OK, let's step back and summarize a bit.

Current situation:
- virtual devices must go into 0xfe, non-virtual devices must go into
  non-0xfe
- 0xfe is the default cssid; non-mcss-e OSs see only devices in there
- to expose non-virtual devices to today's guests, you need to use the
  squash-mcss parameter, which tries to put non-virtual devices into
  the same place in css 0xfe

This is problematic in various ways (potential for incomprehensible
errors, amongst others), so we agreed that the way to go is to drop the
virtual-vs-non-virtual cssid restrictions and allow any device in any
css image.

Now, we need a way for libvirt to detect this, so that it knows that it
can put e.g. vfio-ccw devices in the same css image as virtio devices.

Proposal 1: Use a device attribute to show that the device can be put
anywhere. This approach feels wrong to me (the non-restriction is not a
property of the individual device, but of the whole css resp. the
machine), so I would continue to veto this.

Proposal 2: Export the default cssid as a machine property. If this
property exists, it also implies that devices can be put into any css
image (although it makes the most sense to put them into the default
css image as indicated by the property). Can be made r/w later if it is
too much for 2.12.

Proposal 3: Add a machine property that indicates cssids are not
restricted. Later, optionally add a second property that exposes the
default cssid and makes it configurable.

Personally, I prefer 2 (especially as this also allows to find out
where the best place to put devices for non-mcss-e enabled guests is),
but I could live with 3 as well (if making this r/w later would be
problematic for libvirt.)

(In every case, we would deprecate and later remove the squash
parameter.)

[As an aside, how hard is it to make the default cssid configurable? At
a glance, it does not seem too bad to me.]

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/27/2017 05:56 PM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 16:09:09 +0100
> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
> 
>> On 11/27/2017 03:13 PM, Halil Pasic wrote:
>>>
>>>
>>> On 11/27/2017 02:19 PM, Cornelia Huck wrote:  
>>>> On Mon, 27 Nov 2017 14:11:57 +0100
>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>  
>>>>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:  
>>>>>> On Fri, 24 Nov 2017 17:39:04 +0100
>>>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>>>      
>>>>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
>>>>  
>>>>>>>> (Unless we simply make this a "default cssid" prop after all - then it
>>>>>>>> would be more than just a simple indication for libvirt...)
>>>>>>>>        
>>>>>>>
>>>>>>> We are now talking about the "cssid-unrestricted" property. The default
>>>>>>> cssid is not something I would like to do any time soon.  
>>>>>>
>>>>>> What's so bad about this? As said above, I think it would be much more
>>>>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
>>>>>> start out with a r/o variant now...
>>>>>>      
>>>>>
>>>>> I'm not sure I understand you. Are you proposing the following:
>>>>> Drop the restriction, but don't indicate this via a read only
>>>>> "cssid-unrestricted" device property but via a "default-css"
>>>>> read only machine property.
>>>>>
>>>>> Libvirt then should know that if "default-css" is present then
>>>>> we don't have this virtual into 0xfe and non virtual into 0xfd
>>>>> restriction any more.  
>>>>
>>>> Yes.
>>>>  
>>>
>>> @Boris:
>>> Does this work for you? Technically it's equivalent to having
>>> "cssid-unrestricted" at machine level.
>>>
>>> I don't really like the idea of indicating unrestricted via
>>> something mostly unrelated (at least for me it ain't obvious that
>>> having the id of the default css exposed means we got rid of the
>>> limitation.). But I'm tied of discussing this, so I'm willing to
>>> compromise.
>>>
>>> Halil
>>>   
>> I agree with Christian that we should not throw the "setting of a 
>> default cssid" together with "unrestricted cssid" and than use read/only 
>> and read/write to distinguish between the two!
>>
> 
> OK, let's step back and summarize a bit.
> 
> Current situation:
> - virtual devices must go into 0xfe, non-virtual devices must go into
>   non-0xfe
> - 0xfe is the default cssid; non-mcss-e OSs see only devices in there
> - to expose non-virtual devices to today's guests, you need to use the
>   squash-mcss parameter, which tries to put non-virtual devices into
>   the same place in css 0xfe
> 
> This is problematic in various ways (potential for incomprehensible
> errors, amongst others), so we agreed that the way to go is to drop the
> virtual-vs-non-virtual cssid restrictions and allow any device in any
> css image.
> 
> Now, we need a way for libvirt to detect this, so that it knows that it
> can put e.g. vfio-ccw devices in the same css image as virtio devices.
> 
> Proposal 1: Use a device attribute to show that the device can be put
> anywhere. This approach feels wrong to me (the non-restriction is not a
> property of the individual device, but of the whole css resp. the
> machine), so I would continue to veto this.
> 
> Proposal 2: Export the default cssid as a machine property. If this
> property exists, it also implies that devices can be put into any css
> image (although it makes the most sense to put them into the default
> css image as indicated by the property). Can be made r/w later if it is
> too much for 2.12.
> 
> Proposal 3: Add a machine property that indicates cssids are not
> restricted. Later, optionally add a second property that exposes the
> default cssid and makes it configurable.
> 
> Personally, I prefer 2 (especially as this also allows to find out
> where the best place to put devices for non-mcss-e enabled guests is),
> but I could live with 3 as well (if making this r/w later would be
> problematic for libvirt.)
> 
> (In every case, we would deprecate and later remove the squash
> parameter.)
> 
> [As an aside, how hard is it to make the default cssid configurable? At
> a glance, it does not seem too bad to me.]
> 

Will try to spin a patch based on 3 tomorrow as that seems closest
to consensus. One more thing included will be this auto-generated
change.

Halil

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-11-27 17:56:07 +0100]:

> On Mon, 27 Nov 2017 16:09:09 +0100
> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
> 
> > On 11/27/2017 03:13 PM, Halil Pasic wrote:
> > > 
> > > 
> > > On 11/27/2017 02:19 PM, Cornelia Huck wrote:  
> > >> On Mon, 27 Nov 2017 14:11:57 +0100
> > >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > >>  
> > >>> On 11/27/2017 01:56 PM, Cornelia Huck wrote:  
> > >>>> On Fri, 24 Nov 2017 17:39:04 +0100
> > >>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > >>>>      
> > >>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
> > >>  
> > >>>>>> (Unless we simply make this a "default cssid" prop after all - then it
> > >>>>>> would be more than just a simple indication for libvirt...)
> > >>>>>>        
> > >>>>>
> > >>>>> We are now talking about the "cssid-unrestricted" property. The default
> > >>>>> cssid is not something I would like to do any time soon.  
> > >>>>
> > >>>> What's so bad about this? As said above, I think it would be much more
> > >>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply
> > >>>> start out with a r/o variant now...
> > >>>>      
> > >>>
> > >>> I'm not sure I understand you. Are you proposing the following:
> > >>> Drop the restriction, but don't indicate this via a read only
> > >>> "cssid-unrestricted" device property but via a "default-css"
> > >>> read only machine property.
> > >>>
> > >>> Libvirt then should know that if "default-css" is present then
> > >>> we don't have this virtual into 0xfe and non virtual into 0xfd
> > >>> restriction any more.  
> > >>
> > >> Yes.
> > >>  
> > > 
> > > @Boris:
> > > Does this work for you? Technically it's equivalent to having
> > > "cssid-unrestricted" at machine level.
> > > 
> > > I don't really like the idea of indicating unrestricted via
> > > something mostly unrelated (at least for me it ain't obvious that
> > > having the id of the default css exposed means we got rid of the
> > > limitation.). But I'm tied of discussing this, so I'm willing to
> > > compromise.
> > > 
> > > Halil
> > >   
> > I agree with Christian that we should not throw the "setting of a 
> > default cssid" together with "unrestricted cssid" and than use read/only 
> > and read/write to distinguish between the two!
> > 
> 
> OK, let's step back and summarize a bit.
> 
> Current situation:
> - virtual devices must go into 0xfe, non-virtual devices must go into
>   non-0xfe
> - 0xfe is the default cssid; non-mcss-e OSs see only devices in there
> - to expose non-virtual devices to today's guests, you need to use the
>   squash-mcss parameter, which tries to put non-virtual devices into
>   the same place in css 0xfe
When squashing, everything will be squashed to css 0 which is the
deafult css in this case.

[No effect to the conclusions below, but better to clarify.:-]

> 
> This is problematic in various ways (potential for incomprehensible
> errors, amongst others), so we agreed that the way to go is to drop the
> virtual-vs-non-virtual cssid restrictions and allow any device in any
> css image.
> 
> Now, we need a way for libvirt to detect this, so that it knows that it
> can put e.g. vfio-ccw devices in the same css image as virtio devices.
> 
> Proposal 1: Use a device attribute to show that the device can be put
> anywhere. This approach feels wrong to me (the non-restriction is not a
> property of the individual device, but of the whole css resp. the
> machine), so I would continue to veto this.
> 
> Proposal 2: Export the default cssid as a machine property. If this
> property exists, it also implies that devices can be put into any css
> image (although it makes the most sense to put them into the default
> css image as indicated by the property). Can be made r/w later if it is
> too much for 2.12.
> 
> Proposal 3: Add a machine property that indicates cssids are not
> restricted. Later, optionally add a second property that exposes the
> default cssid and makes it configurable.
> 
> Personally, I prefer 2 (especially as this also allows to find out
> where the best place to put devices for non-mcss-e enabled guests is),
> but I could live with 3 as well (if making this r/w later would be
> problematic for libvirt.)
> 
> (In every case, we would deprecate and later remove the squash
> parameter.)
> 
> [As an aside, how hard is it to make the default cssid configurable? At
> a glance, it does not seem too bad to me.]
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-11-27 13:58:16 +0100]:

> On Mon, 27 Nov 2017 10:20:56 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-11-24 17:39:04 +0100]:
> > 
> > > 
> > > 
> > > On 11/24/2017 05:15 PM, Cornelia Huck wrote:  
> > > >>> In theory this should work. 
> > > >>>
> > > >>> In reality it seems more complicated. A per-device property is easy and can be
> > > >>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a new 
> > > >>> machine property would require to change the qemu help output and qemu-options 
> > > >>> file (which makes it visible for all architectures).    
> > > >> And then we have the fun of describing, that this property is weird, and can
> > > >> not be set, and it's value does not matter.  
> > > > Well, that's the case for both, no?  
> > > 
> > > 
> > > I don't think we have to document _device_ properites in qemu-options.hx
> > > I don't see any documented neither for virtio-ccw nor for vfio-ccw. The
> > > machine properties, on the contrary, are documented in this file.  
> > Is it sane and possible to reuse the existing s390-squash-mcss property
> > to achieve the goal?  I mean, when it is false (which is the default
> > value), can we treat it as "we are allowed to put devices everywhere"?
> > Then we'd have the way to use a property of the -M to tell libvirt that
> > devices can be everywhere?
> > 
> > However then we can not drop it completely I guess, since Libvirt will
> > depends on it. But we can ignore the operation of setting it's value to
> > true.
> 
> I don't think we should reuse it, as it would have rather confusing
> semantics (which can't be easily sorted out unless you check for the
> qemu version).
> 
Right. This is something that I missed. So please ignore my noise.

-- 
Dong Jia Shi

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Boris Fiuczynski]

On 11/27/2017 05:56 PM, Cornelia Huck wrote:
> Proposal 2: Export the default cssid as a machine property. If this
> property exists, it also implies that devices can be put into any css
> image (although it makes the most sense to put them into the default
> css image as indicated by the property). Can be made r/w later if it is
> too much for 2.12.
Just as a side discussion:
I know of qom command query-machines but that does not seem to provide 
the information you suggest to use with proposal 2.
What qom command do you suggest to use for the introspection of the 
machine options access mode?

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 28 Nov 2017 09:53:15 +0100
Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:

> On 11/27/2017 05:56 PM, Cornelia Huck wrote:
> > Proposal 2: Export the default cssid as a machine property. If this
> > property exists, it also implies that devices can be put into any css
> > image (although it makes the most sense to put them into the default
> > css image as indicated by the property). Can be made r/w later if it is
> > too much for 2.12.  
> Just as a side discussion:
> I know of qom command query-machines but that does not seem to provide 
> the information you suggest to use with proposal 2.
> What qom command do you suggest to use for the introspection of the 
> machine options access mode?
> 

Is qom-get what you are looking for?

virsh # qemu-monitor-command vm1 --pretty '{ "execute": "qom-get", "arguments": { "path": "/machine/", "property": "accel"} }'
{
  "return": "kvm",
  "id": "libvirt-18"
}

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Boris Fiuczynski]

On 11/28/2017 11:22 AM, Cornelia Huck wrote:
> On Tue, 28 Nov 2017 09:53:15 +0100
> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
> 
>> On 11/27/2017 05:56 PM, Cornelia Huck wrote:
>>> Proposal 2: Export the default cssid as a machine property. If this
>>> property exists, it also implies that devices can be put into any css
>>> image (although it makes the most sense to put them into the default
>>> css image as indicated by the property). Can be made r/w later if it is
>>> too much for 2.12.
>> Just as a side discussion:
>> I know of qom command query-machines but that does not seem to provide
>> the information you suggest to use with proposal 2.
Sorry, I meant query-command-line-options.

>> What qom command do you suggest to use for the introspection of the
>> machine options access mode?
>>
> 
> Is qom-get what you are looking for?
> 
> virsh # qemu-monitor-command vm1 --pretty '{ "execute": "qom-get", "arguments": { "path": "/machine/", "property": "accel"} }'
> {
>    "return": "kvm",
>    "id": "libvirt-18"
> }
> 
How do you find out from returned values that accel is r/o or r/w?

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 28 Nov 2017 12:49:04 +0100
Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:

> On 11/28/2017 11:22 AM, Cornelia Huck wrote:
> > On Tue, 28 Nov 2017 09:53:15 +0100
> > Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
> >   
> >> On 11/27/2017 05:56 PM, Cornelia Huck wrote:  
> >>> Proposal 2: Export the default cssid as a machine property. If this
> >>> property exists, it also implies that devices can be put into any css
> >>> image (although it makes the most sense to put them into the default
> >>> css image as indicated by the property). Can be made r/w later if it is
> >>> too much for 2.12.  
> >> Just as a side discussion:
> >> I know of qom command query-machines but that does not seem to provide
> >> the information you suggest to use with proposal 2.  
> Sorry, I meant query-command-line-options.
> 
> >> What qom command do you suggest to use for the introspection of the
> >> machine options access mode?
> >>  
> > 
> > Is qom-get what you are looking for?
> > 
> > virsh # qemu-monitor-command vm1 --pretty '{ "execute": "qom-get", "arguments": { "path": "/machine/", "property": "accel"} }'
> > {
> >    "return": "kvm",
> >    "id": "libvirt-18"
> > }
> >   
> How do you find out from returned values that accel is r/o or r/w?
> 

I read some code and it turns out that we aren't really prepared for
any r/o opts... sigh.

So, proposal 2 is only viable if we make it configurable from the start.

Halil, do you see any chance to do this for 2.12? There's plenty of
time left, and I don't think it's too hard. If not, we don't have any
other option than proposal 3, even though I don't like it a lot.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/28/2017 01:14 PM, Cornelia Huck wrote:
> On Tue, 28 Nov 2017 12:49:04 +0100
> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
> 
>> On 11/28/2017 11:22 AM, Cornelia Huck wrote:
>>> On Tue, 28 Nov 2017 09:53:15 +0100
>>> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 11/27/2017 05:56 PM, Cornelia Huck wrote:  
>>>>> Proposal 2: Export the default cssid as a machine property. If this
>>>>> property exists, it also implies that devices can be put into any css
>>>>> image (although it makes the most sense to put them into the default
>>>>> css image as indicated by the property). Can be made r/w later if it is
>>>>> too much for 2.12.  
>>>> Just as a side discussion:
>>>> I know of qom command query-machines but that does not seem to provide
>>>> the information you suggest to use with proposal 2.  
>> Sorry, I meant query-command-line-options.
>>
>>>> What qom command do you suggest to use for the introspection of the
>>>> machine options access mode?
>>>>  
>>>
>>> Is qom-get what you are looking for?
>>>
>>> virsh # qemu-monitor-command vm1 --pretty '{ "execute": "qom-get", "arguments": { "path": "/machine/", "property": "accel"} }'
>>> {
>>>    "return": "kvm",
>>>    "id": "libvirt-18"
>>> }
>>>   
>> How do you find out from returned values that accel is r/o or r/w?
>>
> 
> I read some code and it turns out that we aren't really prepared for
> any r/o opts... sigh.
> 
> So, proposal 2 is only viable if we make it configurable from the start.

I really really dislike option 2.
Binding the capability to "assign freely" to a property that is named default css is
just wrong. Both capabilities are really independent.

I strongly prefer option 3.

> 
> Halil, do you see any chance to do this for 2.12? There's plenty of
> time left, and I don't think it's too hard. If not, we don't have any
> other option than proposal 3, even though I don't like it a lot.
> 

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/28/2017 01:24 PM, Christian Borntraeger wrote:
> 
> 
> On 11/28/2017 01:14 PM, Cornelia Huck wrote:
>> On Tue, 28 Nov 2017 12:49:04 +0100
>> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
>>
>>> On 11/28/2017 11:22 AM, Cornelia Huck wrote:
>>>> On Tue, 28 Nov 2017 09:53:15 +0100
>>>> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
>>>>   
>>>>> On 11/27/2017 05:56 PM, Cornelia Huck wrote:  
>>>>>> Proposal 2: Export the default cssid as a machine property. If this
>>>>>> property exists, it also implies that devices can be put into any css
>>>>>> image (although it makes the most sense to put them into the default
>>>>>> css image as indicated by the property). Can be made r/w later if it is
>>>>>> too much for 2.12.  
>>>>> Just as a side discussion:
>>>>> I know of qom command query-machines but that does not seem to provide
>>>>> the information you suggest to use with proposal 2.  
>>> Sorry, I meant query-command-line-options.
>>>
>>>>> What qom command do you suggest to use for the introspection of the
>>>>> machine options access mode?
>>>>>  
>>>>
>>>> Is qom-get what you are looking for?
>>>>
>>>> virsh # qemu-monitor-command vm1 --pretty '{ "execute": "qom-get", "arguments": { "path": "/machine/", "property": "accel"} }'
>>>> {
>>>>    "return": "kvm",
>>>>    "id": "libvirt-18"
>>>> }
>>>>   
>>> How do you find out from returned values that accel is r/o or r/w?
>>>
>>
>> I read some code and it turns out that we aren't really prepared for
>> any r/o opts... sigh.
>>
>> So, proposal 2 is only viable if we make it configurable from the start.
> 
> I really really dislike option 2.
> Binding the capability to "assign freely" to a property that is named default css is
> just wrong. Both capabilities are really independent.
> 

I fully agree with Christian.

> I strongly prefer option 3.
> 

In the meanwhile I strongly prefer option 1 (at the ccw devices). I've just
sent a v2, and IMHO it shows the limitations of machine properties very well.

>>
>> Halil, do you see any chance to do this for 2.12? There's plenty of
>> time left, and I don't think it's too hard. If not, we don't have any
>> other option than proposal 3, even though I don't like it a lot.
>>

I do think we have enough time to do this right. And of course I'm willing
to do it right. IMHO the 3 options summarized by Connie are not the only
options. But if we go for reworking our QOM composition tree, it will take
a lot of discussion. I'm not sure all the required people have enough spare
time for that.

Halil

 

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/28/2017 02:17 PM, Halil Pasic wrote:
> 
> 
> On 11/28/2017 01:24 PM, Christian Borntraeger wrote:
>>
>>
>> On 11/28/2017 01:14 PM, Cornelia Huck wrote:
>>> On Tue, 28 Nov 2017 12:49:04 +0100
>>> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
>>>
>>>> On 11/28/2017 11:22 AM, Cornelia Huck wrote:
>>>>> On Tue, 28 Nov 2017 09:53:15 +0100
>>>>> Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> wrote:
>>>>>   
>>>>>> On 11/27/2017 05:56 PM, Cornelia Huck wrote:  
>>>>>>> Proposal 2: Export the default cssid as a machine property. If this
>>>>>>> property exists, it also implies that devices can be put into any css
>>>>>>> image (although it makes the most sense to put them into the default
>>>>>>> css image as indicated by the property). Can be made r/w later if it is
>>>>>>> too much for 2.12.  
>>>>>> Just as a side discussion:
>>>>>> I know of qom command query-machines but that does not seem to provide
>>>>>> the information you suggest to use with proposal 2.  
>>>> Sorry, I meant query-command-line-options.
>>>>
>>>>>> What qom command do you suggest to use for the introspection of the
>>>>>> machine options access mode?
>>>>>>  
>>>>>
>>>>> Is qom-get what you are looking for?
>>>>>
>>>>> virsh # qemu-monitor-command vm1 --pretty '{ "execute": "qom-get", "arguments": { "path": "/machine/", "property": "accel"} }'
>>>>> {
>>>>>    "return": "kvm",
>>>>>    "id": "libvirt-18"
>>>>> }
>>>>>   
>>>> How do you find out from returned values that accel is r/o or r/w?
>>>>
>>>
>>> I read some code and it turns out that we aren't really prepared for
>>> any r/o opts... sigh.
>>>
>>> So, proposal 2 is only viable if we make it configurable from the start.
>>
>> I really really dislike option 2.
>> Binding the capability to "assign freely" to a property that is named default css is
>> just wrong. Both capabilities are really independent.
>>
> 
> I fully agree with Christian.
> 
>> I strongly prefer option 3.
>>
> 
> In the meanwhile I strongly prefer option 1 (at the ccw devices). I've just
> sent a v2, and IMHO it shows the limitations of machine properties very well.

option 1 is still fine with me.

> 
>>>
>>> Halil, do you see any chance to do this for 2.12? There's plenty of
>>> time left, and I don't think it's too hard. If not, we don't have any
>>> other option than proposal 3, even though I don't like it a lot.
>>>
> 
> I do think we have enough time to do this right. And of course I'm willing
> to do it right. IMHO the 3 options summarized by Connie are not the only
> options. But if we go for reworking our QOM composition tree, it will take
> a lot of discussion. I'm not sure all the required people have enough spare
> time for that.
> 
> Halil

I am actually not sure if we really want to have a "user-definable default css" anytime
soon. I think it might really open a can of worms in regard to management tooling.

What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
we need to lift the restriction of having vfio not in FE.

What is the use case for allowing to specify a different default CSS today? Unless
we have any usecase this will make things just more complicated for the benefit of what?

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 28 Nov 2017 14:25:08 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/28/2017 02:17 PM, Halil Pasic wrote:

> > In the meanwhile I strongly prefer option 1 (at the ccw devices). I've just
> > sent a v2, and IMHO it shows the limitations of machine properties very well.  
> 
> option 1 is still fine with me.

I still dislike it a lot. Machine properties have their own set of
problems, but I think devices are really the wrong place for a global
property.

> 
> >   
> >>>
> >>> Halil, do you see any chance to do this for 2.12? There's plenty of
> >>> time left, and I don't think it's too hard. If not, we don't have any
> >>> other option than proposal 3, even though I don't like it a lot.
> >>>  
> > 
> > I do think we have enough time to do this right. And of course I'm willing
> > to do it right. IMHO the 3 options summarized by Connie are not the only
> > options. But if we go for reworking our QOM composition tree, it will take
> > a lot of discussion. I'm not sure all the required people have enough spare
> > time for that.
> > 
> > Halil  
> 
> I am actually not sure if we really want to have a "user-definable default css" anytime
> soon. I think it might really open a can of worms in regard to management tooling.

Fair enough, there's really no short-term use case.

> What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
> we need to lift the restriction of having vfio not in FE.

This, however, worries me. I don't really consider vfio-ccw ready for
prime time yet. We still need to figure out path handling at the very
least. And I'm still not sure that our non-handling of halt/clear won't
cause major issues with e.g. a storage server error recovery.

Can we flag vfio-ccw as experimental or such in libvirt?

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Halil Pasic]

On 11/24/2017 01:46 PM, Cornelia Huck wrote:
> On Thu, 23 Nov 2017 14:33:56 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Having an adequate representation for the css in QOM would be certainly
>> interesting, but at the same time (IMHO) is somewhat challenging. Let me
>> make some observations, which should some of my concerns. 
>>
>> We already have virtual-css-bridge which is  the QOM type/object that
>> kind of conceptually stands for the channel subsystem as subsystem (not
>> for the images but for the big whole).
>>
>> The virtual-css-bridge is however non-user-creatable (inherited this from
>> sysbus-device-type), so I doubt it can be used for specifying properties
>> on the command line. That is no good for something like specifying the
>> default css (image, not the big whole). What we usually do is pull up the
>> property to the machine level. But this comes at a cost.
> 
> It might be better to not have inherit this from sysbus (at the price
> of doing some stuff like reset handling ourselves.) Not sure if that
> helps for this concrete issue, though.
> 

My point was: I'm not comfortable with our QOM interfaces. I would like
to become comfortable with them, but I we should make it  a pre-req
for solving the problem at hand (management software support for vfio-ccw).

Thus I  would prefer to resume this discussion later. I will argue whit
whatever I disagree though (in this reply), just to not forget.

>>
>> A small digression, Christian pointed me to the -set cmd line option.
>> I've done a quick PoC (assigned an id in code and did the command line)
>> but I failed.
>>
>> Furthermore the (canonical) QOM path of virtual-css-bridge is
>> /machine/unattached/device[2] (there is a link from sysbus but the child
>> property is connecting it to unattached, see [1]). I would prefer seeing
>> and working wit something like /machine/css/default_css.
> 
> That's because the parent reference is not set up before registering
> the device. It should do a object_property_add_child() (as e.g. the
> flic does) so that it will show up as attached to the machine.
> 

I read it as this is a bug.

>>
>> Under the virtual-css-bridge there is a virtual-css-bus called
>> QOM-path-ly virtual-css, so it's canonical path is
>> /machine/unattached/device[2]/virtual-css. The virtual-css-bus is in qdev
>> a bus however. AFAIU buses are not very suitable for hosting properties
>> (as one can't attach/add buses directly only via devices.
>>
>> Another interesting fact is that virtio-ccw devices are QOMly only linked
>> to their qdev parent bus, the virtual-ccs-bus, and are a QOMly children
>> of /machine/peripheral (see [2]).
> 
> That's true of anything added via qdev_device_add().
> 
> [As an aside, that's why the autogenerated net devices show up as
> unattached as well. Not sure if it is worth doing anything about it.]
> 

I think, in the meanwhile I understood why.

>>
>> To sum it up: I'm pretty confused with the QOM view of things.
>>
>> My intuition would be that the object/device has to be somewhere between
>> /machine and the vrtio-ccw devices in the children graph.  And since we
>> want to do a command line controllable properties on the css object, the
>> object probably needs to be a device.
>>
>> Given what we currently have I don't see a good place for this css
>> object.
> 
> I don't think qom is supposed to map the hardware hierarchy (I might be
> mistaken, though.)

I disagree. Have a look at:
https://wiki.qemu.org/Features/QOM#Naming_in_QOM

The example pretty much looks like it is supposed to reflect the
devices and buses structure. QOM is a hierarchy, if it does
not map to the hardware hierarchy to what hierarchy is it supposed
to map?

See also:


> I don't see anything speaking against a css object
> attached to the machine (if we can't use the css bridge for that
> purpose.)
> 
>>
>> Furthermore I'm currently under the impression that every non-abstract
>> device is user creatable (but some can be created only implicitly). Some
>> seem to go even further and say should workwith device-add [3].
> 
> There are quite a number of devices for which being user-createable
> does not make any sense at all.
> 

I disagree. A device that can not be created (by the user, only user
create devices) is useless. You are probably speaking about implicit/
explicit. In my interpretation the virtual-css-bridge is created by the
user. If the user specifies a machine deriving form s390-virtio-ccw
then the user implicitly creates a virtual-css-bridge. But if the user
user the same binary to create a 'none' machine he does not create
a virtual-css-bridge.

Another interesting example is virtio-net. It is created implicity
by he user with a s390-virtio-ccw if it is not created explicitly by the user
and if the user did not explicitly disable implicit creation. For
machine type none no virtio-net is created implicitly. So the user
using a s390-virtio-ccw has 3 options: create it implicitly, create
it explicitly, disable implicit creation.

Of course if there has to be exactly one  (or N) a certain device for a
machine, and the device has not user adjustable properties, then
there is not reason for the user for wanting to explicitly specify
the device on the command line.

I think, you misunderstood me. If not. Please tell me how do we
set properties for non-user-createable non-abstract devices on
the command line?

> What _could_ make sense is having a way to add css images manually and
> having specify whether they are default. (Obviously not when they are
> hotplugged.) For compatibility, we could create css image 0xfe
> automatically as default if none has been defined. Not sure how easy it
> would be to get this right, though.
> 

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/28/2017 03:01 PM, Cornelia Huck wrote:
> On Tue, 28 Nov 2017 14:25:08 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 11/28/2017 02:17 PM, Halil Pasic wrote:
> 
>>> In the meanwhile I strongly prefer option 1 (at the ccw devices). I've just
>>> sent a v2, and IMHO it shows the limitations of machine properties very well.  
>>
>> option 1 is still fine with me.
> 
> I still dislike it a lot. Machine properties have their own set of
> problems, but I think devices are really the wrong place for a global
> property.
> 
>>
>>>   
>>>>>
>>>>> Halil, do you see any chance to do this for 2.12? There's plenty of
>>>>> time left, and I don't think it's too hard. If not, we don't have any
>>>>> other option than proposal 3, even though I don't like it a lot.
>>>>>  
>>>
>>> I do think we have enough time to do this right. And of course I'm willing
>>> to do it right. IMHO the 3 options summarized by Connie are not the only
>>> options. But if we go for reworking our QOM composition tree, it will take
>>> a lot of discussion. I'm not sure all the required people have enough spare
>>> time for that.
>>>
>>> Halil  
>>
>> I am actually not sure if we really want to have a "user-definable default css" anytime
>> soon. I think it might really open a can of worms in regard to management tooling.
> 
> Fair enough, there's really no short-term use case.
> 
>> What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
>> we need to lift the restriction of having vfio not in FE.
> 
> This, however, worries me. I don't really consider vfio-ccw ready for
> prime time yet. We still need to figure out path handling at the very
> least. And I'm still not sure that our non-handling of halt/clear won't
> cause major issues with e.g. a storage server error recovery.
> 
> Can we flag vfio-ccw as experimental or such in libvirt?

We should have flagged vfio-ccw experimental in QEMU then.
e.g. by using x-vfio-ccw or whatever.I dont think we can do that
after the facts.

I am not that deep into vfio-ccw, but I was under the impression that
the missing features should not affect the vfio interface that is
created by libvirt. After all this should just be a "-device vfio-ccw,....."
statement and some setup steps beforehand (unbind + setup of vfio-ccw)

If your concern is the serviceability I think it would be valid for a RHEL
KVM to disable vfio-ccw in the kernel. Maybe we could even provide a config
for QEMU?


PS: I learned from the PCI and CCW experience that I do not want
to upstream kernel/qemu code unless I have a working libvirt code that
shows that the thing will work.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 28 Nov 2017 15:17:49 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/28/2017 03:01 PM, Cornelia Huck wrote:
> > On Tue, 28 Nov 2017 14:25:08 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> >> What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
> >> we need to lift the restriction of having vfio not in FE.  
> > 
> > This, however, worries me. I don't really consider vfio-ccw ready for
> > prime time yet. We still need to figure out path handling at the very
> > least. And I'm still not sure that our non-handling of halt/clear won't
> > cause major issues with e.g. a storage server error recovery.
> > 
> > Can we flag vfio-ccw as experimental or such in libvirt?  
> 
> We should have flagged vfio-ccw experimental in QEMU then.
> e.g. by using x-vfio-ccw or whatever.I dont think we can do that
> after the facts.

Yes, we should have done that... can't fix that now, unfortunately.

> 
> I am not that deep into vfio-ccw, but I was under the impression that
> the missing features should not affect the vfio interface that is
> created by libvirt. After all this should just be a "-device vfio-ccw,....."
> statement and some setup steps beforehand (unbind + setup of vfio-ccw)

The halt/clear stuff is highly unlikely to influence the configuration
interface. I'm still not too clear about path handling, though. Will we
need different modes (hypervisor managed vs. full passthrough? probably
only passthrough)? What about pgid handling - do we need some kind of
pgid manager? (Keep in mind that you get to set the pgid once. This has
implications for e.g. reserve/release.)

Also, what about devices other than ECKD DASD? Has there been any
testing? Tapes should be interesting; the other interesting ccw devices
are QDIO-based and I'm not sure we want to spend anything on supporting
those.

The interface is probably fine, but I'd like to get an idea about the
path stuff (is the attachment spec that contains the pgid stuff
publicly available, btw.?)

> 
> If your concern is the serviceability I think it would be valid for a RHEL
> KVM to disable vfio-ccw in the kernel. Maybe we could even provide a config
> for QEMU?

It's fine just to turn off vfio-ccw in the kernel.

> PS: I learned from the PCI and CCW experience that I do not want
> to upstream kernel/qemu code unless I have a working libvirt code that
> shows that the thing will work.

Yes, I understand the wish to verify the interface, and I think it's a
good idea. What I'm worried about is that this might be the precursor
to a push to support it, and I don't think vfio-ccw is ready for that
yet.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/28/2017 03:45 PM, Cornelia Huck wrote:
> On Tue, 28 Nov 2017 15:17:49 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 11/28/2017 03:01 PM, Cornelia Huck wrote:
>>> On Tue, 28 Nov 2017 14:25:08 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>>>> What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
>>>> we need to lift the restriction of having vfio not in FE.  
>>>
>>> This, however, worries me. I don't really consider vfio-ccw ready for
>>> prime time yet. We still need to figure out path handling at the very
>>> least. And I'm still not sure that our non-handling of halt/clear won't
>>> cause major issues with e.g. a storage server error recovery.
>>>
>>> Can we flag vfio-ccw as experimental or such in libvirt?  
>>
>> We should have flagged vfio-ccw experimental in QEMU then.
>> e.g. by using x-vfio-ccw or whatever.I dont think we can do that
>> after the facts.
> 
> Yes, we should have done that... can't fix that now, unfortunately.
> 
>>
>> I am not that deep into vfio-ccw, but I was under the impression that
>> the missing features should not affect the vfio interface that is
>> created by libvirt. After all this should just be a "-device vfio-ccw,....."
>> statement and some setup steps beforehand (unbind + setup of vfio-ccw)
> 
> The halt/clear stuff is highly unlikely to influence the configuration
> interface. I'm still not too clear about path handling, though. Will we
> need different modes (hypervisor managed vs. full passthrough? probably
> only passthrough)? What about pgid handling - do we need some kind of
> pgid manager? (Keep in mind that you get to set the pgid once. This has
> implications for e.g. reserve/release.)
> 
> Also, what about devices other than ECKD DASD? Has there been any
> testing? Tapes should be interesting; the other interesting ccw devices
> are QDIO-based and I'm not sure we want to spend anything on supporting
> those.

DASD is probably the most important thing today, QDIO might never happen
(or very late).
See my proposal below.

> 
> The interface is probably fine, but I'd like to get an idea about the
> path stuff (is the attachment spec that contains the pgid stuff
> publicly available, btw.?)
> 
>>
>> If your concern is the serviceability I think it would be valid for a RHEL
>> KVM to disable vfio-ccw in the kernel. Maybe we could even provide a config
>> for QEMU?
> 
> It's fine just to turn off vfio-ccw in the kernel.
> 
>> PS: I learned from the PCI and CCW experience that I do not want
>> to upstream kernel/qemu code unless I have a working libvirt code that
>> shows that the thing will work.
> 
> Yes, I understand the wish to verify the interface, and I think it's a
> good idea. What I'm worried about is that this might be the precursor
> to a push to support it, and I don't think vfio-ccw is ready for that
> yet.
> 
FWIW, libvirt should not care if a device works in all cases or not because 
libvirt versions should work with all kind of QEMU/kernel combinations. 
Fencing in libvirt that the kernel part is not up to the task is making
me feel the same way as you when you see  the css-unrestricted property
at a device ;-)
Having said that, I think that having vfio-ccw support has a value (and it
actually works for an unnamed test infrastructure). In addition to that I 
am much more likely to actually test this continuously if I have libvirt support.


So what about the following: 

1. We will implement the libvirt support with either a or b:
a: if we find a solution to our "where to put the property" dispute use that to decide
if we can add vfio-ccw
b if not: just provide a patch that lifts the restriction without any property.
in libvirt blindy assume  that free assignment will work. old QEMUs will complain at
startup and  libvirt will print the QEMU error. This is similar to other situations 
where libvirt cannot fully bprobe  if the QEMU will work or not. (not having vfio-ccw
support in the kernel will certainly allow libvirt to reject this upfront)

2. at the same time we mark vfio-ccw experimental in the kernel to make clear
 that this is still work in progress

3. (optional) we could even whitelist devices that work in a reasonable fashion (e.g.
do not whitelist qdio but whitelist DASD)

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Wed, 29 Nov 2017 19:51:23 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/28/2017 03:45 PM, Cornelia Huck wrote:
> > On Tue, 28 Nov 2017 15:17:49 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 11/28/2017 03:01 PM, Cornelia Huck wrote:  
> >>> On Tue, 28 Nov 2017 14:25:08 +0100
> >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:  
> >   
> >>>> What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
> >>>> we need to lift the restriction of having vfio not in FE.    
> >>>
> >>> This, however, worries me. I don't really consider vfio-ccw ready for
> >>> prime time yet. We still need to figure out path handling at the very
> >>> least. And I'm still not sure that our non-handling of halt/clear won't
> >>> cause major issues with e.g. a storage server error recovery.
> >>>
> >>> Can we flag vfio-ccw as experimental or such in libvirt?    
> >>
> >> We should have flagged vfio-ccw experimental in QEMU then.
> >> e.g. by using x-vfio-ccw or whatever.I dont think we can do that
> >> after the facts.  
> > 
> > Yes, we should have done that... can't fix that now, unfortunately.
> >   
> >>
> >> I am not that deep into vfio-ccw, but I was under the impression that
> >> the missing features should not affect the vfio interface that is
> >> created by libvirt. After all this should just be a "-device vfio-ccw,....."
> >> statement and some setup steps beforehand (unbind + setup of vfio-ccw)  
> > 
> > The halt/clear stuff is highly unlikely to influence the configuration
> > interface. I'm still not too clear about path handling, though. Will we
> > need different modes (hypervisor managed vs. full passthrough? probably
> > only passthrough)? What about pgid handling - do we need some kind of
> > pgid manager? (Keep in mind that you get to set the pgid once. This has
> > implications for e.g. reserve/release.)
> > 
> > Also, what about devices other than ECKD DASD? Has there been any
> > testing? Tapes should be interesting; the other interesting ccw devices
> > are QDIO-based and I'm not sure we want to spend anything on supporting
> > those.  
> 
> DASD is probably the most important thing today, QDIO might never happen
> (or very late).

One thing I'd find interesting about tapes is very long running channel
programs (like rewind) and how they interact with halt/clear. But yes,
I would think that DASD is the most important one in practice.

> See my proposal below.
> 
> > 
> > The interface is probably fine, but I'd like to get an idea about the
> > path stuff (is the attachment spec that contains the pgid stuff
> > publicly available, btw.?)
> >   
> >>
> >> If your concern is the serviceability I think it would be valid for a RHEL
> >> KVM to disable vfio-ccw in the kernel. Maybe we could even provide a config
> >> for QEMU?  
> > 
> > It's fine just to turn off vfio-ccw in the kernel.
> >   
> >> PS: I learned from the PCI and CCW experience that I do not want
> >> to upstream kernel/qemu code unless I have a working libvirt code that
> >> shows that the thing will work.  
> > 
> > Yes, I understand the wish to verify the interface, and I think it's a
> > good idea. What I'm worried about is that this might be the precursor
> > to a push to support it, and I don't think vfio-ccw is ready for that
> > yet.
> >   
> FWIW, libvirt should not care if a device works in all cases or not because 
> libvirt versions should work with all kind of QEMU/kernel combinations. 
> Fencing in libvirt that the kernel part is not up to the task is making
> me feel the same way as you when you see  the css-unrestricted property
> at a device ;-)

I'm more worried about the political angle than the technical. If we
don't get pressure to support this too soon, I don't care that much
about experimental or not.

> Having said that, I think that having vfio-ccw support has a value (and it
> actually works for an unnamed test infrastructure). In addition to that I 
> am much more likely to actually test this continuously if I have libvirt support.

Good to hear that it works for a non-Linux guest. Any plans to test
something like storage server failover?

[I'd really love to do something about the path handling stuff, but the
combination of scant documentation and scantier time works against me.]

> 
> 
> So what about the following: 
> 
> 1. We will implement the libvirt support with either a or b:
> a: if we find a solution to our "where to put the property" dispute use that to decide
> if we can add vfio-ccw
> b if not: just provide a patch that lifts the restriction without any property.
> in libvirt blindy assume  that free assignment will work. old QEMUs will complain at
> startup and  libvirt will print the QEMU error. This is similar to other situations 
> where libvirt cannot fully bprobe  if the QEMU will work or not. (not having vfio-ccw
> support in the kernel will certainly allow libvirt to reject this upfront)

I hope we end up with a solution that works for everyone...

> 
> 2. at the same time we mark vfio-ccw experimental in the kernel to make clear
>  that this is still work in progress

I'm not sure we can do that after the fact, but let's do it if we can.

> 
> 3. (optional) we could even whitelist devices that work in a reasonable fashion (e.g.
> do not whitelist qdio but whitelist DASD)

I think it's not quite that easy with vfio-ccw working at the
subchannel level.

Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Christian Borntraeger]

On 11/30/2017 10:50 AM, Cornelia Huck wrote:
> On Wed, 29 Nov 2017 19:51:23 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 11/28/2017 03:45 PM, Cornelia Huck wrote:
>>> On Tue, 28 Nov 2017 15:17:49 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> On 11/28/2017 03:01 PM, Cornelia Huck wrote:  
>>>>> On Tue, 28 Nov 2017 14:25:08 +0100
>>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:  
>>>   
>>>>>> What I want now is to enable vfio-ccw for libvirt and Linux guests and for that
>>>>>> we need to lift the restriction of having vfio not in FE.    
>>>>>
>>>>> This, however, worries me. I don't really consider vfio-ccw ready for
>>>>> prime time yet. We still need to figure out path handling at the very
>>>>> least. And I'm still not sure that our non-handling of halt/clear won't
>>>>> cause major issues with e.g. a storage server error recovery.
>>>>>
>>>>> Can we flag vfio-ccw as experimental or such in libvirt?    
>>>>
>>>> We should have flagged vfio-ccw experimental in QEMU then.
>>>> e.g. by using x-vfio-ccw or whatever.I dont think we can do that
>>>> after the facts.  
>>>
>>> Yes, we should have done that... can't fix that now, unfortunately.
>>>   
>>>>
>>>> I am not that deep into vfio-ccw, but I was under the impression that
>>>> the missing features should not affect the vfio interface that is
>>>> created by libvirt. After all this should just be a "-device vfio-ccw,....."
>>>> statement and some setup steps beforehand (unbind + setup of vfio-ccw)  
>>>
>>> The halt/clear stuff is highly unlikely to influence the configuration
>>> interface. I'm still not too clear about path handling, though. Will we
>>> need different modes (hypervisor managed vs. full passthrough? probably
>>> only passthrough)? What about pgid handling - do we need some kind of
>>> pgid manager? (Keep in mind that you get to set the pgid once. This has
>>> implications for e.g. reserve/release.)
>>>
>>> Also, what about devices other than ECKD DASD? Has there been any
>>> testing? Tapes should be interesting; the other interesting ccw devices
>>> are QDIO-based and I'm not sure we want to spend anything on supporting
>>> those.  
>>
>> DASD is probably the most important thing today, QDIO might never happen
>> (or very late).
> 
> One thing I'd find interesting about tapes is very long running channel
> programs (like rewind) and how they interact with halt/clear. But yes,
> I would think that DASD is the most important one in practice.
> 
>> See my proposal below.
>>
>>>
>>> The interface is probably fine, but I'd like to get an idea about the
>>> path stuff (is the attachment spec that contains the pgid stuff
>>> publicly available, btw.?)
>>>   
>>>>
>>>> If your concern is the serviceability I think it would be valid for a RHEL
>>>> KVM to disable vfio-ccw in the kernel. Maybe we could even provide a config
>>>> for QEMU?  
>>>
>>> It's fine just to turn off vfio-ccw in the kernel.
>>>   
>>>> PS: I learned from the PCI and CCW experience that I do not want
>>>> to upstream kernel/qemu code unless I have a working libvirt code that
>>>> shows that the thing will work.  
>>>
>>> Yes, I understand the wish to verify the interface, and I think it's a
>>> good idea. What I'm worried about is that this might be the precursor
>>> to a push to support it, and I don't think vfio-ccw is ready for that
>>> yet.
>>>   
>> FWIW, libvirt should not care if a device works in all cases or not because 
>> libvirt versions should work with all kind of QEMU/kernel combinations. 
>> Fencing in libvirt that the kernel part is not up to the task is making
>> me feel the same way as you when you see  the css-unrestricted property
>> at a device ;-)
> 
> I'm more worried about the political angle than the technical. If we
> don't get pressure to support this too soon, I don't care that much
> about experimental or not.
> 
>> Having said that, I think that having vfio-ccw support has a value (and it
>> actually works for an unnamed test infrastructure). In addition to that I 
>> am much more likely to actually test this continuously if I have libvirt support.
> 
> Good to hear that it works for a non-Linux guest. Any plans to test
> something like storage server failover?
> 
> [I'd really love to do something about the path handling stuff, but the
> combination of scant documentation and scantier time works against me.]
> 
>>
>>
>> So what about the following: 
>>
>> 1. We will implement the libvirt support with either a or b:
>> a: if we find a solution to our "where to put the property" dispute use that to decide
>> if we can add vfio-ccw
>> b if not: just provide a patch that lifts the restriction without any property.
>> in libvirt blindy assume  that free assignment will work. old QEMUs will complain at
>> startup and  libvirt will print the QEMU error. This is similar to other situations 
>> where libvirt cannot fully bprobe  if the QEMU will work or not. (not having vfio-ccw
>> support in the kernel will certainly allow libvirt to reject this upfront)
> 
> I hope we end up with a solution that works for everyone...
> 
>>
>> 2. at the same time we mark vfio-ccw experimental in the kernel to make clear
>>  that this is still work in progress
> 
> I'm not sure we can do that after the fact, but let's do it if we can.

I think we can change that. Its just a Kconfig dependency.
 
>>
>> 3. (optional) we could even whitelist devices that work in a reasonable fashion (e.g.
>> do not whitelist qdio but whitelist DASD)
> 
> I think it's not quite that easy with vfio-ccw working at the
> subchannel level.
>