* [PATCH 1/2] virtio: allow drivers to validate features
@ 2017-03-29 17:14 Michael S. Tsirkin
2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2017-03-29 17:14 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, virtualization
Some drivers can't support all features in all configurations. At the
moment we blindly set FEATURES_OK and later FAILED. Support this better
by adding a callback drivers can use to do some early checks.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio.c | 6 ++++++
include/linux/virtio.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 400d70b..48230a5 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -232,6 +232,12 @@ static int virtio_dev_probe(struct device *_d)
if (device_features & (1ULL << i))
__virtio_set_bit(dev, i);
+ if (drv->validate) {
+ err = drv->validate(dev);
+ if (err)
+ goto err;
+ }
+
err = virtio_finalize_features(dev);
if (err)
goto err;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 193fea9..ed04753 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -176,6 +176,7 @@ struct virtio_driver {
unsigned int feature_table_size;
const unsigned int *feature_table_legacy;
unsigned int feature_table_size_legacy;
+ int (*validate)(struct virtio_device *dev);
int (*probe)(struct virtio_device *dev);
void (*scan)(struct virtio_device *dev);
void (*remove)(struct virtio_device *dev);
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] virtio_net: clear MTU when out of range 2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin @ 2017-03-29 17:14 ` Michael S. Tsirkin 2017-03-30 9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck 2017-03-30 19:39 ` David Miller 2 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2017-03-29 17:14 UTC (permalink / raw) To: linux-kernel; +Cc: netdev, virtualization virtio attempts to clear the MTU feature bit if the value is out of the supported range, but this has no real effect since FEATURES_OK has already been set. Fix this up by checking the MTU in the new validate callback. Fixes: 14de9d114a82 ("virtio-net: Add initial MTU advice feature") Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/net/virtio_net.c | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index f6a379d..6aba098 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2312,14 +2312,8 @@ static bool virtnet_validate_features(struct virtio_device *vdev) #define MIN_MTU ETH_MIN_MTU #define MAX_MTU ETH_MAX_MTU -static int virtnet_probe(struct virtio_device *vdev) +static int virtnet_validate(struct virtio_device *vdev) { - int i, err; - struct net_device *dev; - struct virtnet_info *vi; - u16 max_queue_pairs; - int mtu; - if (!vdev->config->get) { dev_err(&vdev->dev, "%s failure: config access disabled\n", __func__); @@ -2329,6 +2323,25 @@ static int virtnet_probe(struct virtio_device *vdev) if (!virtnet_validate_features(vdev)) return -EINVAL; + if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { + int mtu = virtio_cread16(vdev, + offsetof(struct virtio_net_config, + mtu)); + if (mtu < MIN_MTU) + __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU); + } + + return 0; +} + +static int virtnet_probe(struct virtio_device *vdev) +{ + int i, err; + struct net_device *dev; + struct virtnet_info *vi; + u16 max_queue_pairs; + int mtu; + /* Find if host supports multiqueue virtio_net device */ err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ, struct virtio_net_config, @@ -2444,12 +2457,17 @@ static int virtnet_probe(struct virtio_device *vdev) offsetof(struct virtio_net_config, mtu)); if (mtu < dev->min_mtu) { - __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU); - } else { - dev->mtu = mtu; - dev->max_mtu = mtu; + /* Should never trigger: MTU was previously validated + * in virtnet_validate. + */ + dev_err(&vdev->dev, "device MTU appears to have changed " + "it is now %d < %d", mtu, dev->min_mtu); + goto free_stats; } + dev->mtu = mtu; + dev->max_mtu = mtu; + /* TODO: size buffers correctly in this case. */ if (dev->mtu > ETH_DATA_LEN) vi->big_packets = true; @@ -2630,6 +2648,7 @@ static struct virtio_driver virtio_net_driver = { .driver.name = KBUILD_MODNAME, .driver.owner = THIS_MODULE, .id_table = id_table, + .validate = virtnet_validate, .probe = virtnet_probe, .remove = virtnet_remove, .config_changed = virtnet_config_changed, -- MST ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features 2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin 2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin @ 2017-03-30 9:06 ` Cornelia Huck 2017-03-30 14:57 ` Michael S. Tsirkin 2017-03-30 19:39 ` David Miller 2 siblings, 1 reply; 6+ messages in thread From: Cornelia Huck @ 2017-03-30 9:06 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: linux-kernel, netdev, virtualization On Wed, 29 Mar 2017 20:14:44 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Some drivers can't support all features in all configurations. At the > moment we blindly set FEATURES_OK and later FAILED. Support this better > by adding a callback drivers can use to do some early checks. Looks reasonable. Do we need to document that the driver must not do anything beyond dealing with features and reading the config space that early? > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/virtio/virtio.c | 6 ++++++ > include/linux/virtio.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 400d70b..48230a5 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -232,6 +232,12 @@ static int virtio_dev_probe(struct device *_d) > if (device_features & (1ULL << i)) > __virtio_set_bit(dev, i); > > + if (drv->validate) { > + err = drv->validate(dev); > + if (err) > + goto err; > + } > + > err = virtio_finalize_features(dev); > if (err) > goto err; > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index 193fea9..ed04753 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -176,6 +176,7 @@ struct virtio_driver { > unsigned int feature_table_size; > const unsigned int *feature_table_legacy; > unsigned int feature_table_size_legacy; > + int (*validate)(struct virtio_device *dev); > int (*probe)(struct virtio_device *dev); > void (*scan)(struct virtio_device *dev); > void (*remove)(struct virtio_device *dev); Would be good to add some doc; but other members are undocumented here already... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features 2017-03-30 9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck @ 2017-03-30 14:57 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2017-03-30 14:57 UTC (permalink / raw) To: Cornelia Huck; +Cc: netdev, linux-kernel, virtualization On Thu, Mar 30, 2017 at 11:06:27AM +0200, Cornelia Huck wrote: > On Wed, 29 Mar 2017 20:14:44 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > Some drivers can't support all features in all configurations. At the > > moment we blindly set FEATURES_OK and later FAILED. Support this better > > by adding a callback drivers can use to do some early checks. > > Looks reasonable. Do we need to document that the driver must not do > anything beyond dealing with features and reading the config space that > early? It's up to the driver - we probably should document that on failure neither probe nor remove will be called. On success we proceed to probe. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > drivers/virtio/virtio.c | 6 ++++++ > > include/linux/virtio.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index 400d70b..48230a5 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -232,6 +232,12 @@ static int virtio_dev_probe(struct device *_d) > > if (device_features & (1ULL << i)) > > __virtio_set_bit(dev, i); > > > > + if (drv->validate) { > > + err = drv->validate(dev); > > + if (err) > > + goto err; > > + } > > + > > err = virtio_finalize_features(dev); > > if (err) > > goto err; > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index 193fea9..ed04753 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -176,6 +176,7 @@ struct virtio_driver { > > unsigned int feature_table_size; > > const unsigned int *feature_table_legacy; > > unsigned int feature_table_size_legacy; > > + int (*validate)(struct virtio_device *dev); > > int (*probe)(struct virtio_device *dev); > > void (*scan)(struct virtio_device *dev); > > void (*remove)(struct virtio_device *dev); > > Would be good to add some doc; but other members are undocumented here > already... True. Patches welcome. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features 2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin 2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin 2017-03-30 9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck @ 2017-03-30 19:39 ` David Miller 2017-03-31 3:27 ` Michael S. Tsirkin 2 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2017-03-30 19:39 UTC (permalink / raw) To: mst; +Cc: netdev, linux-kernel, virtualization From: "Michael S. Tsirkin" <mst@redhat.com> Date: Wed, 29 Mar 2017 20:14:44 +0300 > Some drivers can't support all features in all configurations. At the > moment we blindly set FEATURES_OK and later FAILED. Support this better > by adding a callback drivers can use to do some early checks. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Michael do you want me to take these virtio networking fixes into my tree directly or are you going to send me a pull request or something after it all settles down? Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features 2017-03-30 19:39 ` David Miller @ 2017-03-31 3:27 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2017-03-31 3:27 UTC (permalink / raw) To: David Miller; +Cc: linux-kernel, jasowang, virtualization, netdev On Thu, Mar 30, 2017 at 12:39:31PM -0700, David Miller wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > Date: Wed, 29 Mar 2017 20:14:44 +0300 > > > Some drivers can't support all features in all configurations. At the > > moment we blindly set FEATURES_OK and later FAILED. Support this better > > by adding a callback drivers can use to do some early checks. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Michael do you want me to take these virtio networking fixes into my > tree directly or are you going to send me a pull request or something > after it all settles down? > > Thanks. I think I'll send a pull request. Thanks, -- MST ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-31 3:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin 2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin 2017-03-30 9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck 2017-03-30 14:57 ` Michael S. Tsirkin 2017-03-30 19:39 ` David Miller 2017-03-31 3:27 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).