| Message ID | 20171201143136.62497-3-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | unrestrict cssids related patches | expand |
On 12/01/2017 03:31 PM, Halil Pasic wrote: > Let us advertise the changes introduced by "s390x/css: unrestrict cssids" > to the management software (so it can tell are cssids unrestricted or > restricted). > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > > Boris says having the property on the virtual-css-bridge is good form > Libvirt PoV. @Shalini: could you verify that things work out fine > (provided we get at least a preliminary blessing from Connie). > > Consider squashing into "s390x/css: unrestrict cssids". FWIW, a property on the virtual-css-bridge is Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> So if Conny is also ok with this we might have found our solution. > --- > hw/s390x/css-bridge.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > index c4a9735d71..c7e8998680 100644 > --- a/hw/s390x/css-bridge.c > +++ b/hw/s390x/css-bridge.c > @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static bool prop_get_true(Object *obj, Error **errp) > +{ > + return true; > +} > + > static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > { > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > hc->unplug = ccw_device_unplug; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > dc->props = virtual_css_bridge_properties; > + object_class_property_add_bool(klass, "cssid-unrestricted", > + prop_get_true, NULL, NULL); > + object_class_property_set_description(klass, "cssid-unrestricted", > + "A css device can use any cssid, regardless whether virtual" > + " or not (read only, always true)", > + NULL); > } > > static const TypeInfo virtual_css_bridge_info = { >
On Fri, 1 Dec 2017 15:31:35 +0100 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Let us advertise the changes introduced by "s390x/css: unrestrict cssids" > to the management software (so it can tell are cssids unrestricted or > restricted). > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > > Boris says having the property on the virtual-css-bridge is good form > Libvirt PoV. @Shalini: could you verify that things work out fine > (provided we get at least a preliminary blessing from Connie). > > Consider squashing into "s390x/css: unrestrict cssids". > --- > hw/s390x/css-bridge.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > index c4a9735d71..c7e8998680 100644 > --- a/hw/s390x/css-bridge.c > +++ b/hw/s390x/css-bridge.c > @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static bool prop_get_true(Object *obj, Error **errp) > +{ > + return true; > +} > + > static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > { > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > hc->unplug = ccw_device_unplug; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > dc->props = virtual_css_bridge_properties; > + object_class_property_add_bool(klass, "cssid-unrestricted", > + prop_get_true, NULL, NULL); > + object_class_property_set_description(klass, "cssid-unrestricted", > + "A css device can use any cssid, regardless whether virtual" extra space -----------------------------^ > + " or not (read only, always true)", > + NULL); > } > > static const TypeInfo virtual_css_bridge_info = { Looks reasonable. If this works as expected, I'll squash it into the previous patch.
On 12/04/2017 12:15 PM, Cornelia Huck wrote: > On Fri, 1 Dec 2017 15:31:35 +0100 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" >> to the management software (so it can tell are cssids unrestricted or >> restricted). >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> >> Boris says having the property on the virtual-css-bridge is good form >> Libvirt PoV. @Shalini: could you verify that things work out fine >> (provided we get at least a preliminary blessing from Connie). >> >> Consider squashing into "s390x/css: unrestrict cssids". >> --- >> hw/s390x/css-bridge.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c >> index c4a9735d71..c7e8998680 100644 >> --- a/hw/s390x/css-bridge.c >> +++ b/hw/s390x/css-bridge.c >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> +static bool prop_get_true(Object *obj, Error **errp) >> +{ >> + return true; >> +} >> + >> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >> { >> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >> hc->unplug = ccw_device_unplug; >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> dc->props = virtual_css_bridge_properties; >> + object_class_property_add_bool(klass, "cssid-unrestricted", >> + prop_get_true, NULL, NULL); >> + object_class_property_set_description(klass, "cssid-unrestricted", >> + "A css device can use any cssid, regardless whether virtual" > > extra space -----------------------------^ > Nod. >> + " or not (read only, always true)", Do we need "." here ----------------------------^ ? >> + NULL); >> } >> >> static const TypeInfo virtual_css_bridge_info = { > > Looks reasonable. If this works as expected, I'll squash it into the > previous patch. > I've just asked Shalini to verify the libvirt perspective. Supposed we verify this works as expected, I read I don't have to spin a v2 and you are going to fix the issues found yourself. Right?
On Mon, 4 Dec 2017 16:07:27 +0100 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 12/04/2017 12:15 PM, Cornelia Huck wrote: > > On Fri, 1 Dec 2017 15:31:35 +0100 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" > >> to the management software (so it can tell are cssids unrestricted or > >> restricted). > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> > >> Boris says having the property on the virtual-css-bridge is good form > >> Libvirt PoV. @Shalini: could you verify that things work out fine > >> (provided we get at least a preliminary blessing from Connie). > >> > >> Consider squashing into "s390x/css: unrestrict cssids". > >> --- > >> hw/s390x/css-bridge.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > >> index c4a9735d71..c7e8998680 100644 > >> --- a/hw/s390x/css-bridge.c > >> +++ b/hw/s390x/css-bridge.c > >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> +static bool prop_get_true(Object *obj, Error **errp) > >> +{ > >> + return true; > >> +} > >> + > >> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > >> { > >> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > >> hc->unplug = ccw_device_unplug; > >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > >> dc->props = virtual_css_bridge_properties; > >> + object_class_property_add_bool(klass, "cssid-unrestricted", > >> + prop_get_true, NULL, NULL); > >> + object_class_property_set_description(klass, "cssid-unrestricted", > >> + "A css device can use any cssid, regardless whether virtual" > > > > extra space -----------------------------^ > > > > Nod. > > >> + " or not (read only, always true)", > > Do we need "." here ----------------------------^ ? None of the other descs do that. > > >> + NULL); > >> } > >> > >> static const TypeInfo virtual_css_bridge_info = { > > > > Looks reasonable. If this works as expected, I'll squash it into the > > previous patch. > > > > I've just asked Shalini to verify the libvirt perspective. > > Supposed we verify this works as expected, I read I don't have to spin > a v2 and you are going to fix the issues found yourself. Right? I'd prefer a v2 that I can simply apply. Let's wait for some more acks/r-bs.
On 12/04/2017 05:07 PM, Cornelia Huck wrote: > On Mon, 4 Dec 2017 16:07:27 +0100 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 12/04/2017 12:15 PM, Cornelia Huck wrote: >>> On Fri, 1 Dec 2017 15:31:35 +0100 >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >>> >>>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" >>>> to the management software (so it can tell are cssids unrestricted or >>>> restricted). >>>> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>> --- >>>> >>>> Boris says having the property on the virtual-css-bridge is good form >>>> Libvirt PoV. @Shalini: could you verify that things work out fine >>>> (provided we get at least a preliminary blessing from Connie). >>>> >>>> Consider squashing into "s390x/css: unrestrict cssids". >>>> --- >>>> hw/s390x/css-bridge.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c >>>> index c4a9735d71..c7e8998680 100644 >>>> --- a/hw/s390x/css-bridge.c >>>> +++ b/hw/s390x/css-bridge.c >>>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> >>>> +static bool prop_get_true(Object *obj, Error **errp) >>>> +{ >>>> + return true; >>>> +} >>>> + >>>> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>>> { >>>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >>>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>>> hc->unplug = ccw_device_unplug; >>>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >>>> dc->props = virtual_css_bridge_properties; >>>> + object_class_property_add_bool(klass, "cssid-unrestricted", >>>> + prop_get_true, NULL, NULL); >>>> + object_class_property_set_description(klass, "cssid-unrestricted", >>>> + "A css device can use any cssid, regardless whether virtual" >>> >>> extra space -----------------------------^ >>> >> >> Nod. >> >>>> + " or not (read only, always true)", >> >> Do we need "." here ----------------------------^ ? > > None of the other descs do that. > >> >>>> + NULL); >>>> } >>>> >>>> static const TypeInfo virtual_css_bridge_info = { >>> >>> Looks reasonable. If this works as expected, I'll squash it into the >>> previous patch. >>> >> >> I've just asked Shalini to verify the libvirt perspective. >> >> Supposed we verify this works as expected, I read I don't have to spin >> a v2 and you are going to fix the issues found yourself. Right? > > I'd prefer a v2 that I can simply apply. Let's wait for some more > acks/r-bs. > Cool with me ;)!
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-12-04 16:07:27 +0100]: > > > On 12/04/2017 12:15 PM, Cornelia Huck wrote: > > On Fri, 1 Dec 2017 15:31:35 +0100 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" > >> to the management software (so it can tell are cssids unrestricted or > >> restricted). > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> > >> Boris says having the property on the virtual-css-bridge is good form > >> Libvirt PoV. @Shalini: could you verify that things work out fine > >> (provided we get at least a preliminary blessing from Connie). > >> > >> Consider squashing into "s390x/css: unrestrict cssids". > >> --- > >> hw/s390x/css-bridge.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > >> index c4a9735d71..c7e8998680 100644 > >> --- a/hw/s390x/css-bridge.c > >> +++ b/hw/s390x/css-bridge.c > >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> +static bool prop_get_true(Object *obj, Error **errp) > >> +{ > >> + return true; > >> +} > >> + > >> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > >> { > >> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > >> hc->unplug = ccw_device_unplug; > >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > >> dc->props = virtual_css_bridge_properties; > >> + object_class_property_add_bool(klass, "cssid-unrestricted", > >> + prop_get_true, NULL, NULL); > >> + object_class_property_set_description(klass, "cssid-unrestricted", > >> + "A css device can use any cssid, regardless whether virtual" > > > > extra space -----------------------------^ > > > > Nod. > > >> + " or not (read only, always true)", > > Do we need "." here ----------------------------^ ? > > >> + NULL); > >> } > >> > >> static const TypeInfo virtual_css_bridge_info = { > > > > Looks reasonable. If this works as expected, I'll squash it into the > > previous patch. > > > > I've just asked Shalini to verify the libvirt perspective. > > Supposed we verify this works as expected, I read I don't have to spin > a v2 and you are going to fix the issues found yourself. Right? Supposed this works as expected, you have my r-b. ;)
On 01.12.2017 15:31, Halil Pasic wrote: > Let us advertise the changes introduced by "s390x/css: unrestrict cssids" > to the management software (so it can tell are cssids unrestricted or > restricted). > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > > Boris says having the property on the virtual-css-bridge is good form > Libvirt PoV. @Shalini: could you verify that things work out fine > (provided we get at least a preliminary blessing from Connie). > > Consider squashing into "s390x/css: unrestrict cssids". > --- > hw/s390x/css-bridge.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > index c4a9735d71..c7e8998680 100644 > --- a/hw/s390x/css-bridge.c > +++ b/hw/s390x/css-bridge.c > @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static bool prop_get_true(Object *obj, Error **errp) > +{ > + return true; > +} > + > static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > { > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > hc->unplug = ccw_device_unplug; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > dc->props = virtual_css_bridge_properties; > + object_class_property_add_bool(klass, "cssid-unrestricted", > + prop_get_true, NULL, NULL); > + object_class_property_set_description(klass, "cssid-unrestricted", > + "A css device can use any cssid, regardless whether virtual" > + " or not (read only, always true)", I'd maybe remove the "always true" in the description here, since that might create wrong assumptions with regards to future versions or lead to bad code in the upper layers. If someone reads "always true", they simply might omit the check of the value of this property. If we then ever want to change it to "false" again, we're in trouble (sure, we could simply completely remove the property again, but we have to remember to do that instead of setting it to false ... so let's better play safe right now already, ok?) Thomas
On 12/05/2017 09:28 AM, Thomas Huth wrote: > On 01.12.2017 15:31, Halil Pasic wrote: >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" >> to the management software (so it can tell are cssids unrestricted or >> restricted). >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> >> Boris says having the property on the virtual-css-bridge is good form >> Libvirt PoV. @Shalini: could you verify that things work out fine >> (provided we get at least a preliminary blessing from Connie). >> >> Consider squashing into "s390x/css: unrestrict cssids". >> --- >> hw/s390x/css-bridge.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c >> index c4a9735d71..c7e8998680 100644 >> --- a/hw/s390x/css-bridge.c >> +++ b/hw/s390x/css-bridge.c >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> +static bool prop_get_true(Object *obj, Error **errp) >> +{ >> + return true; >> +} >> + >> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >> { >> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >> hc->unplug = ccw_device_unplug; >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> dc->props = virtual_css_bridge_properties; >> + object_class_property_add_bool(klass, "cssid-unrestricted", >> + prop_get_true, NULL, NULL); >> + object_class_property_set_description(klass, "cssid-unrestricted", >> + "A css device can use any cssid, regardless whether virtual" >> + " or not (read only, always true)", > > I'd maybe remove the "always true" in the description here, since that > might create wrong assumptions with regards to future versions or lead > to bad code in the upper layers. If someone reads "always true", they > simply might omit the check of the value of this property. If we then > ever want to change it to "false" again, we're in trouble (sure, we > could simply completely remove the property again, but we have to > remember to do that instead of setting it to false ... so let's better > play safe right now already, ok?) > > Thomas > Libvirt intends to check for the existence of the property and ignore it's value. I've been told in Libvirt capabilities are usually tied to existence. For inspecting the value one would have to work on an instance. I don't think that would work with Libvirt's capability probing scheme well. So what you describe is basically like intended. I was not to document always true though, but then Connie had a version with always true and I got convinced it ain't a bad idea. If both Connie and you agree that 'always true' is to be dropped I'm fine with dropping it. Thanks for the review! Halil
On Tue, 5 Dec 2017 11:08:18 +0100 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 12/05/2017 09:28 AM, Thomas Huth wrote: > > On 01.12.2017 15:31, Halil Pasic wrote: > >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" > >> to the management software (so it can tell are cssids unrestricted or > >> restricted). > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> > >> Boris says having the property on the virtual-css-bridge is good form > >> Libvirt PoV. @Shalini: could you verify that things work out fine > >> (provided we get at least a preliminary blessing from Connie). > >> > >> Consider squashing into "s390x/css: unrestrict cssids". > >> --- > >> hw/s390x/css-bridge.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > >> index c4a9735d71..c7e8998680 100644 > >> --- a/hw/s390x/css-bridge.c > >> +++ b/hw/s390x/css-bridge.c > >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> +static bool prop_get_true(Object *obj, Error **errp) > >> +{ > >> + return true; > >> +} > >> + > >> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > >> { > >> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) > >> hc->unplug = ccw_device_unplug; > >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > >> dc->props = virtual_css_bridge_properties; > >> + object_class_property_add_bool(klass, "cssid-unrestricted", > >> + prop_get_true, NULL, NULL); > >> + object_class_property_set_description(klass, "cssid-unrestricted", > >> + "A css device can use any cssid, regardless whether virtual" > >> + " or not (read only, always true)", > > > > I'd maybe remove the "always true" in the description here, since that > > might create wrong assumptions with regards to future versions or lead > > to bad code in the upper layers. If someone reads "always true", they > > simply might omit the check of the value of this property. If we then > > ever want to change it to "false" again, we're in trouble (sure, we > > could simply completely remove the property again, but we have to > > remember to do that instead of setting it to false ... so let's better > > play safe right now already, ok?) > > > > Thomas > > > > Libvirt intends to check for the existence of the property and ignore > it's value. I've been told in Libvirt capabilities are usually tied > to existence. For inspecting the value one would have to work on an > instance. I don't think that would work with Libvirt's capability > probing scheme well. > > So what you describe is basically like intended. I was not to document > always true though, but then Connie had a version with always true > and I got convinced it ain't a bad idea. If both Connie and you agree > that 'always true' is to be dropped I'm fine with dropping it. I highly doubt that we will want to switch it to false again, especially considering libvirt usage. So I'd prefer to document this.
On 05.12.2017 11:08, Halil Pasic wrote: > > > On 12/05/2017 09:28 AM, Thomas Huth wrote: >> On 01.12.2017 15:31, Halil Pasic wrote: >>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" >>> to the management software (so it can tell are cssids unrestricted or >>> restricted). >>> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> --- >>> >>> Boris says having the property on the virtual-css-bridge is good form >>> Libvirt PoV. @Shalini: could you verify that things work out fine >>> (provided we get at least a preliminary blessing from Connie). >>> >>> Consider squashing into "s390x/css: unrestrict cssids". >>> --- >>> hw/s390x/css-bridge.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c >>> index c4a9735d71..c7e8998680 100644 >>> --- a/hw/s390x/css-bridge.c >>> +++ b/hw/s390x/css-bridge.c >>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> +static bool prop_get_true(Object *obj, Error **errp) >>> +{ >>> + return true; >>> +} >>> + >>> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>> { >>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>> hc->unplug = ccw_device_unplug; >>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >>> dc->props = virtual_css_bridge_properties; >>> + object_class_property_add_bool(klass, "cssid-unrestricted", >>> + prop_get_true, NULL, NULL); >>> + object_class_property_set_description(klass, "cssid-unrestricted", >>> + "A css device can use any cssid, regardless whether virtual" >>> + " or not (read only, always true)", >> >> I'd maybe remove the "always true" in the description here, since that >> might create wrong assumptions with regards to future versions or lead >> to bad code in the upper layers. If someone reads "always true", they >> simply might omit the check of the value of this property. If we then >> ever want to change it to "false" again, we're in trouble (sure, we >> could simply completely remove the property again, but we have to >> remember to do that instead of setting it to false ... so let's better >> play safe right now already, ok?) >> >> Thomas >> > > Libvirt intends to check for the existence of the property and ignore > it's value. OK, I wasn't aware of the fact that libvirt normally only checks for the presence, but not for the value. Then yes, please keep the "always true" in the comment. Thomas
On 12/04/2017 04:07 PM, Halil Pasic wrote: > > On 12/04/2017 12:15 PM, Cornelia Huck wrote: >> On Fri, 1 Dec 2017 15:31:35 +0100 >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> >>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" >>> to the management software (so it can tell are cssids unrestricted or >>> restricted). >>> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> --- >>> >>> Boris says having the property on the virtual-css-bridge is good form >>> Libvirt PoV. @Shalini: could you verify that things work out fine >>> (provided we get at least a preliminary blessing from Connie). >>> >>> Consider squashing into "s390x/css: unrestrict cssids". >>> --- >>> hw/s390x/css-bridge.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c >>> index c4a9735d71..c7e8998680 100644 >>> --- a/hw/s390x/css-bridge.c >>> +++ b/hw/s390x/css-bridge.c >>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> +static bool prop_get_true(Object *obj, Error **errp) >>> +{ >>> + return true; >>> +} >>> + >>> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>> { >>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>> hc->unplug = ccw_device_unplug; >>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >>> dc->props = virtual_css_bridge_properties; >>> + object_class_property_add_bool(klass, "cssid-unrestricted", >>> + prop_get_true, NULL, NULL); >>> + object_class_property_set_description(klass, "cssid-unrestricted", >>> + "A css device can use any cssid, regardless whether virtual" >> extra space -----------------------------^ >> > Nod. > >>> + " or not (read only, always true)", > Do we need "." here ----------------------------^ ? > >>> + NULL); >>> } >>> >>> static const TypeInfo virtual_css_bridge_info = { >> Looks reasonable. If this works as expected, I'll squash it into the >> previous patch. >> > I've just asked Shalini to verify the libvirt perspective. Verified. The patch works as expected. > > Supposed we verify this works as expected, I read I don't have to spin > a v2 and you are going to fix the issues found yourself. Right?
On Tue, 5 Dec 2017 18:28:47 +0100 Shalini Chellathurai Saroja <shalini@linux.vnet.ibm.com> wrote: > On 12/04/2017 04:07 PM, Halil Pasic wrote: > > > > On 12/04/2017 12:15 PM, Cornelia Huck wrote: > >> Looks reasonable. If this works as expected, I'll squash it into the > >> previous patch. > >> > > I've just asked Shalini to verify the libvirt perspective. > > Verified. The patch works as expected. Cool, thanks! It seems we have a winner here.
On 12/05/2017 06:28 PM, Shalini Chellathurai Saroja wrote: > > > On 12/04/2017 04:07 PM, Halil Pasic wrote: >> >> On 12/04/2017 12:15 PM, Cornelia Huck wrote: >>> On Fri, 1 Dec 2017 15:31:35 +0100 >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >>> >>>> Let us advertise the changes introduced by "s390x/css: unrestrict cssids" >>>> to the management software (so it can tell are cssids unrestricted or >>>> restricted). >>>> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>> --- >>>> >>>> Boris says having the property on the virtual-css-bridge is good form >>>> Libvirt PoV. @Shalini: could you verify that things work out fine >>>> (provided we get at least a preliminary blessing from Connie). >>>> >>>> Consider squashing into "s390x/css: unrestrict cssids". >>>> --- >>>> hw/s390x/css-bridge.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c >>>> index c4a9735d71..c7e8998680 100644 >>>> --- a/hw/s390x/css-bridge.c >>>> +++ b/hw/s390x/css-bridge.c >>>> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> +static bool prop_get_true(Object *obj, Error **errp) >>>> +{ >>>> + return true; >>>> +} >>>> + >>>> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>>> { >>>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >>>> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) >>>> hc->unplug = ccw_device_unplug; >>>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >>>> dc->props = virtual_css_bridge_properties; >>>> + object_class_property_add_bool(klass, "cssid-unrestricted", >>>> + prop_get_true, NULL, NULL); >>>> + object_class_property_set_description(klass, "cssid-unrestricted", >>>> + "A css device can use any cssid, regardless whether virtual" >>> extra space -----------------------------^ >>> >> Nod. >> >>>> + " or not (read only, always true)", >> Do we need "." here ----------------------------^ ? >> >>>> + NULL); >>>> } >>>> static const TypeInfo virtual_css_bridge_info = { >>> Looks reasonable. If this works as expected, I'll squash it into the >>> previous patch. >>> >> I've just asked Shalini to verify the libvirt perspective. > > Verified. The patch works as expected. > Many thanks! I intend to address the remaining issues of the series and spin a v2 today. Halil
diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c index c4a9735d71..c7e8998680 100644 --- a/hw/s390x/css-bridge.c +++ b/hw/s390x/css-bridge.c @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static bool prop_get_true(Object *obj, Error **errp) +{ + return true; +} + static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) { HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data) hc->unplug = ccw_device_unplug; set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->props = virtual_css_bridge_properties; + object_class_property_add_bool(klass, "cssid-unrestricted", + prop_get_true, NULL, NULL); + object_class_property_set_description(klass, "cssid-unrestricted", + "A css device can use any cssid, regardless whether virtual" + " or not (read only, always true)", + NULL); } static const TypeInfo virtual_css_bridge_info = {
Let us advertise the changes introduced by "s390x/css: unrestrict cssids" to the management software (so it can tell are cssids unrestricted or restricted). Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- Boris says having the property on the virtual-css-bridge is good form Libvirt PoV. @Shalini: could you verify that things work out fine (provided we get at least a preliminary blessing from Connie). Consider squashing into "s390x/css: unrestrict cssids". --- hw/s390x/css-bridge.c | 11 +++++++++++ 1 file changed, 11 insertions(+)