| Message ID | 20171121111825.17916-1-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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]) {
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.
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
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]) { >
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.)
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]) { > > >
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 [..]
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.
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]) {
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.
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.
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?
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?
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;
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.
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.
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.
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)
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).
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.
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.
* 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.
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...
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).
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.
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.
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
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
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 > >
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!
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.]
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
* 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.] >
* 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.
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?
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" }
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?
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.
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. >
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
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?
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?
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. >
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.
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.
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)
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.
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. >
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]) {