| Message ID | 5138C007.6080305@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 07.03.2013 17:27, schrieb Christian Borntraeger: >> It's a bug in both virtio-ccw that features=0 when get_features is >> called. You can also tell this with: >> >> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >> >> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >> right. > > At least, this patch seems to work. (That also implies, that a transport > must not hide virtio feature bits). To me it indicates that the use of the old qdev property setters is hiding errors resulting from trying to set not-existing properties. If we would set the properties in a way that gets us an Error* on failure like the object_property_set_*() do, we would notice on machine creation (or device_add). Andreas > > > From: Christian Borntraeger <borntraeger@de.ibm.com> > Date: Thu, 7 Mar 2013 17:21:41 +0100 > Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus > > Enable all virtio-net features for the legacy s390 virtio bus. > This also fixes > kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121! > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/s390x/s390-virtio-bus.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > index 1200691..a8a8e19 100644 > --- a/hw/s390x/s390-virtio-bus.c > +++ b/hw/s390x/s390-virtio-bus.c > @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = { > > static Property s390_virtio_net_properties[] = { > DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic), > + DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features), > DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device, > net.txtimer, TX_TIMER_INTERVAL), > DEFINE_PROP_INT32("x-txburst", VirtIOS390Device, >
On 07.03.2013, at 17:27, Christian Borntraeger wrote: >> You're misreading how this works. >> >> Host features are set based on command line arguments. This is >> advertised to the guest. The vdev->get_config() call then sanitizes >> features. For instance, look at: >> >> static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features) >> { >> VirtIONet *n = to_virtio_net(vdev); >> NetClientState *nc = qemu_get_queue(n->nic); >> >> features |= (1 << VIRTIO_NET_F_MAC); >> >> if (!peer_has_vnet_hdr(n)) { >> features &= ~(0x1 << VIRTIO_NET_F_CSUM); >> <snip> >> >> This removes the VIRTIO_NET_F_CSUM feature if the peer doesn't support >> it. It's presupposing that the feature bit is set. >> >> It's a bug in both virtio-ccw that features=0 when get_features is >> called. You can also tell this with: >> >> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >> >> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >> right. > > There is a parse error in your statement (error in both virtio-ccw). > Is virtio-ccw ok or not? > > > At least, this patch seems to work. (That also implies, that a transport > must not hide virtio feature bits). > > > From: Christian Borntraeger <borntraeger@de.ibm.com> > Date: Thu, 7 Mar 2013 17:21:41 +0100 > Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus > > Enable all virtio-net features for the legacy s390 virtio bus. > This also fixes > kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121! > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> Thanks, applied to s390-next. Alex > --- > hw/s390x/s390-virtio-bus.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > index 1200691..a8a8e19 100644 > --- a/hw/s390x/s390-virtio-bus.c > +++ b/hw/s390x/s390-virtio-bus.c > @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = { > > static Property s390_virtio_net_properties[] = { > DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic), > + DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features), > DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device, > net.txtimer, TX_TIMER_INTERVAL), > DEFINE_PROP_INT32("x-txburst", VirtIOS390Device, > -- > 1.8.0.1 >
Christian Borntraeger <borntraeger@de.ibm.com> writes: >> You're misreading how this works. >> >> Host features are set based on command line arguments. This is >> advertised to the guest. The vdev->get_config() call then sanitizes >> features. For instance, look at: >> >> static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features) >> { >> VirtIONet *n = to_virtio_net(vdev); >> NetClientState *nc = qemu_get_queue(n->nic); >> >> features |= (1 << VIRTIO_NET_F_MAC); >> >> if (!peer_has_vnet_hdr(n)) { >> features &= ~(0x1 << VIRTIO_NET_F_CSUM); >> <snip> >> >> This removes the VIRTIO_NET_F_CSUM feature if the peer doesn't support >> it. It's presupposing that the feature bit is set. >> >> It's a bug in both virtio-ccw that features=0 when get_features is >> called. You can also tell this with: >> >> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >> >> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >> right. > > There is a parse error in your statement (error in both virtio-ccw). > Is virtio-ccw ok or not? Sorry, virtio-ccw is okay. > At least, this patch seems to work. (That also implies, that a transport > must not hide virtio feature bits). Looks correct to me. Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Regards, Anthony Liguori > > > From: Christian Borntraeger <borntraeger@de.ibm.com> > Date: Thu, 7 Mar 2013 17:21:41 +0100 > Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus > > Enable all virtio-net features for the legacy s390 virtio bus. > This also fixes > kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121! > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/s390x/s390-virtio-bus.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > index 1200691..a8a8e19 100644 > --- a/hw/s390x/s390-virtio-bus.c > +++ b/hw/s390x/s390-virtio-bus.c > @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = { > > static Property s390_virtio_net_properties[] = { > DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic), > + DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features), > DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device, > net.txtimer, TX_TIMER_INTERVAL), > DEFINE_PROP_INT32("x-txburst", VirtIOS390Device, > -- > 1.8.0.1
Andreas Färber <afaerber@suse.de> writes: > Am 07.03.2013 17:27, schrieb Christian Borntraeger: >>> It's a bug in both virtio-ccw that features=0 when get_features is >>> called. You can also tell this with: >>> >>> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >>> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >>> >>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >>> right. >> >> At least, this patch seems to work. (That also implies, that a transport >> must not hide virtio feature bits). > > To me it indicates that the use of the old qdev property setters is > hiding errors resulting from trying to set not-existing properties. > If we would set the properties in a way that gets us an Error* on > failure like the object_property_set_*() do, we would notice on machine > creation (or device_add). Hrm, I don't understand your statement. Can you elaborate? Regards, Anthony Liguori > > Andreas > >> >> >> From: Christian Borntraeger <borntraeger@de.ibm.com> >> Date: Thu, 7 Mar 2013 17:21:41 +0100 >> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus >> >> Enable all virtio-net features for the legacy s390 virtio bus. >> This also fixes >> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121! >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/s390x/s390-virtio-bus.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c >> index 1200691..a8a8e19 100644 >> --- a/hw/s390x/s390-virtio-bus.c >> +++ b/hw/s390x/s390-virtio-bus.c >> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = { >> >> static Property s390_virtio_net_properties[] = { >> DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic), >> + DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features), >> DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device, >> net.txtimer, TX_TIMER_INTERVAL), >> DEFINE_PROP_INT32("x-txburst", VirtIOS390Device, >> > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On Thu, Mar 07, 2013 at 10:44:18AM -0600, Anthony Liguori wrote: > Andreas Färber <afaerber@suse.de> writes: > > > Am 07.03.2013 17:27, schrieb Christian Borntraeger: > >>> It's a bug in both virtio-ccw that features=0 when get_features is > >>> called. You can also tell this with: > >>> > >>> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * > >>> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), > >>> > >>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it > >>> right. > >> > >> At least, this patch seems to work. (That also implies, that a transport > >> must not hide virtio feature bits). > > > > To me it indicates that the use of the old qdev property setters is > > hiding errors resulting from trying to set not-existing properties. > > If we would set the properties in a way that gets us an Error* on > > failure like the object_property_set_*() do, we would notice on machine > > creation (or device_add). > > Hrm, I don't understand your statement. > > Can you elaborate? > > Regards, > > Anthony Liguori Well to me this indicates that s390 virtio is buggy. It's not supposed to crash whatever set of features we specify. > > > > Andreas > > > >> > >> > >> From: Christian Borntraeger <borntraeger@de.ibm.com> > >> Date: Thu, 7 Mar 2013 17:21:41 +0100 > >> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus > >> > >> Enable all virtio-net features for the legacy s390 virtio bus. > >> This also fixes > >> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121! > >> > >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> hw/s390x/s390-virtio-bus.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > >> index 1200691..a8a8e19 100644 > >> --- a/hw/s390x/s390-virtio-bus.c > >> +++ b/hw/s390x/s390-virtio-bus.c > >> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = { > >> > >> static Property s390_virtio_net_properties[] = { > >> DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic), > >> + DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features), > >> DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device, > >> net.txtimer, TX_TIMER_INTERVAL), > >> DEFINE_PROP_INT32("x-txburst", VirtIOS390Device, > >> > > > > > > -- > > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 07.03.2013 17:44, schrieb Anthony Liguori: > Andreas Färber <afaerber@suse.de> writes: > >> Am 07.03.2013 17:27, schrieb Christian Borntraeger: >>>> It's a bug in both virtio-ccw that features=0 when get_features is >>>> called. You can also tell this with: >>>> >>>> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >>>> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >>>> >>>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >>>> right. >>> >>> At least, this patch seems to work. (That also implies, that a transport >>> must not hide virtio feature bits). >> >> To me it indicates that the use of the old qdev property setters is >> hiding errors resulting from trying to set not-existing properties. >> If we would set the properties in a way that gets us an Error* on >> failure like the object_property_set_*() do, we would notice on machine >> creation (or device_add). > > Hrm, I don't understand your statement. > > Can you elaborate? <afaerber> aliguori_, borntraeger added new qdev static properties as bug fix <afaerber> aliguori_, I was saying if errors setting such properties were not silently ignored, we would notice such bugs earlier I.e., no new field was added, no new value set, so apparently something somewhere is setting some of these properties that are defined via virtio-net.h:DEFINE_VIRTIO_NET_FEATURES() using DEFINE_PROP_BIT(). And if code expects these properties to be settable, failing to set them should be treated as error and an appropriate API for setting individual bits with Error **errp argument should be provided. Unfortunately I don't see any property matching Alex' VIRTIO_NET_F_MAC, so I can't draft a patch for illustration. Regards, Andreas
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Thu, Mar 07, 2013 at 10:44:18AM -0600, Anthony Liguori wrote: >> Andreas Färber <afaerber@suse.de> writes: >> >> > Am 07.03.2013 17:27, schrieb Christian Borntraeger: >> >>> It's a bug in both virtio-ccw that features=0 when get_features is >> >>> called. You can also tell this with: >> >>> >> >>> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >> >>> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >> >>> >> >>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >> >>> right. >> >> >> >> At least, this patch seems to work. (That also implies, that a transport >> >> must not hide virtio feature bits). >> > >> > To me it indicates that the use of the old qdev property setters is >> > hiding errors resulting from trying to set not-existing properties. >> > If we would set the properties in a way that gets us an Error* on >> > failure like the object_property_set_*() do, we would notice on machine >> > creation (or device_add). >> >> Hrm, I don't understand your statement. >> >> Can you elaborate? >> >> Regards, >> >> Anthony Liguori > > > Well to me this indicates that s390 virtio is buggy. > It's not supposed to crash whatever set of features > we specify. Ack. Regards, Anthony Liguori
Andreas Färber <afaerber@suse.de> writes: > Am 07.03.2013 17:44, schrieb Anthony Liguori: >> Andreas Färber <afaerber@suse.de> writes: >> >>> Am 07.03.2013 17:27, schrieb Christian Borntraeger: >>>>> It's a bug in both virtio-ccw that features=0 when get_features is >>>>> called. You can also tell this with: >>>>> >>>>> [10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES * >>>>> virtio-ccw.c: DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]), >>>>> >>>>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it >>>>> right. >>>> >>>> At least, this patch seems to work. (That also implies, that a transport >>>> must not hide virtio feature bits). >>> >>> To me it indicates that the use of the old qdev property setters is >>> hiding errors resulting from trying to set not-existing properties. >>> If we would set the properties in a way that gets us an Error* on >>> failure like the object_property_set_*() do, we would notice on machine >>> creation (or device_add). >> >> Hrm, I don't understand your statement. >> >> Can you elaborate? > > <afaerber> aliguori_, borntraeger added new qdev static properties as > bug fix > <afaerber> aliguori_, I was saying if errors setting such properties > were not silently ignored, we would notice such bugs earlier > > I.e., no new field was added, no new value set, so apparently something > somewhere is setting some of these properties that are defined via > virtio-net.h:DEFINE_VIRTIO_NET_FEATURES() using DEFINE_PROP_BIT(). No code is explicitly setting the property. Each property has a default value (usually true) and that's how we get the initial non-zero value. So in the absence of the define, we end up with no properties and no value for this field. Regards, Anthony Liguori > > And if code expects these properties to be settable, failing to set them > should be treated as error and an appropriate API for setting individual > bits with Error **errp argument should be provided. > > Unfortunately I don't see any property matching Alex' VIRTIO_NET_F_MAC, > so I can't draft a patch for illustration. > > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c index 1200691..a8a8e19 100644 --- a/hw/s390x/s390-virtio-bus.c +++ b/hw/s390x/s390-virtio-bus.c @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = { static Property s390_virtio_net_properties[] = { DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic), + DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features), DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device, net.txtimer, TX_TIMER_INTERVAL), DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,