| Message ID | 20230430131518.2708471-2-alvaro.karsz@solid-run.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio-net: allow usage of small vrings | expand |
On Sun, Apr 30, 2023 at 04:15:16PM +0300, Alvaro Karsz wrote: > This patch exports a new virtio core function: virtio_block_feature. > The function should be called during a virtio driver probe. > > If a virtio driver blocks features during probe and fails probe, virtio > core will reset the device, try to re-negotiate the new features and > probe again. > > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com> > --- > drivers/virtio/virtio.c | 73 ++++++++++++++++++++++++++++++----------- > include/linux/virtio.h | 3 ++ > 2 files changed, 56 insertions(+), 20 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 3893dc29eb2..eaad5b6a7a9 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -167,6 +167,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) > } > EXPORT_SYMBOL_GPL(virtio_add_status); > > +void virtio_block_feature(struct virtio_device *dev, unsigned int f) > +{ > + BUG_ON(f >= 64); > + dev->blocked_features |= (1ULL << f); > +} > +EXPORT_SYMBOL_GPL(virtio_block_feature); > + Let's add documentation please. Also pls call it __virtio_block_feature since it has to be used in a special way - specifically only during probe. > /* Do some validation, then set FEATURES_OK */ > static int virtio_features_ok(struct virtio_device *dev) > { > @@ -234,17 +241,13 @@ void virtio_reset_device(struct virtio_device *dev) > } > EXPORT_SYMBOL_GPL(virtio_reset_device); > > -static int virtio_dev_probe(struct device *_d) > +static int virtio_negotiate_features(struct virtio_device *dev) > { > - int err, i; > - struct virtio_device *dev = dev_to_virtio(_d); > struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); > u64 device_features; > u64 driver_features; > u64 driver_features_legacy; > - > - /* We have a driver! */ > - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > + int i, ret; > > /* Figure out what features the device supports. */ > device_features = dev->config->get_features(dev); > @@ -279,30 +282,61 @@ static int virtio_dev_probe(struct device *_d) > if (device_features & (1ULL << i)) > __virtio_set_bit(dev, i); > > - err = dev->config->finalize_features(dev); > - if (err) > - goto err; > + /* Remove blocked features */ > + dev->features &= ~dev->blocked_features; > + > + ret = dev->config->finalize_features(dev); > + if (ret) > + goto exit; > > if (drv->validate) { > u64 features = dev->features; > > - err = drv->validate(dev); > - if (err) > - goto err; > + ret = drv->validate(dev); > + if (ret) > + goto exit; > > /* Did validation change any features? Then write them again. */ > if (features != dev->features) { > - err = dev->config->finalize_features(dev); > - if (err) > - goto err; > + ret = dev->config->finalize_features(dev); > + if (ret) > + goto exit; > } > } > > - err = virtio_features_ok(dev); > - if (err) > - goto err; > + ret = virtio_features_ok(dev); > +exit: > + return ret; > +} > + > +static int virtio_dev_probe(struct device *_d) > +{ > + int err; > + struct virtio_device *dev = dev_to_virtio(_d); > + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); > + u64 blocked_features; > + bool renegotiate = true; > + > + /* We have a driver! */ > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > + > + /* Store blocked features and attempt to negotiate features & probe. > + * If the probe fails, we check if the driver has blocked any new features. > + * If it has, we reset the device and try again with the new features. > + */ > + while (renegotiate) { > + blocked_features = dev->blocked_features; > + err = virtio_negotiate_features(dev); > + if (err) > + break; > + > + err = drv->probe(dev); there's no way to driver to clear blocked features, but just in case, I'd add BUG_ON to check. > + if (err && blocked_features != dev->blocked_features) > + virtio_reset_device(dev); > + else > + renegotiate = false; > + } > > - err = drv->probe(dev); > if (err) > goto err; > > @@ -319,7 +353,6 @@ static int virtio_dev_probe(struct device *_d) > err: > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > return err; > - > } > > static void virtio_dev_remove(struct device *_d) > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index b93238db94e..2de9b2d3ca4 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -109,6 +109,7 @@ int virtqueue_resize(struct virtqueue *vq, u32 num, > * @vringh_config: configuration ops for host vrings. > * @vqs: the list of virtqueues for this device. > * @features: the features supported by both driver and device. > + * @blocked_features: the features blocked by the driver that can't be negotiated. > * @priv: private pointer for the driver's use. > */ > struct virtio_device { > @@ -124,6 +125,7 @@ struct virtio_device { > const struct vringh_config_ops *vringh_config; > struct list_head vqs; > u64 features; > + u64 blocked_features; add comment here too, explain purpose and rules of use > void *priv; > }; > > @@ -133,6 +135,7 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status); > int register_virtio_device(struct virtio_device *dev); > void unregister_virtio_device(struct virtio_device *dev); > bool is_virtio_device(struct device *dev); > +void virtio_block_feature(struct virtio_device *dev, unsigned int f); > > void virtio_break_device(struct virtio_device *dev); > void __virtio_unbreak_device(struct virtio_device *dev); > -- > 2.34.1
> > +void virtio_block_feature(struct virtio_device *dev, unsigned int f) > > +{ > > + BUG_ON(f >= 64); > > + dev->blocked_features |= (1ULL << f); > > +} > > +EXPORT_SYMBOL_GPL(virtio_block_feature); > > + > > Let's add documentation please. Also pls call it __virtio_block_feature > since it has to be used in a special way - specifically only during > probe. > Ok. > > + /* Store blocked features and attempt to negotiate features & probe. > > + * If the probe fails, we check if the driver has blocked any new features. > > + * If it has, we reset the device and try again with the new features. > > + */ > > + while (renegotiate) { > > + blocked_features = dev->blocked_features; > > + err = virtio_negotiate_features(dev); > > + if (err) > > + break; > > + > > + err = drv->probe(dev); > > > there's no way to driver to clear blocked features, but > just in case, I'd add BUG_ON to check. > Ok. > > * @features: the features supported by both driver and device. > > + * @blocked_features: the features blocked by the driver that can't be negotiated. > > * @priv: private pointer for the driver's use. > > */ > > struct virtio_device { > > @@ -124,6 +125,7 @@ struct virtio_device { > > const struct vringh_config_ops *vringh_config; > > struct list_head vqs; > > u64 features; > > + u64 blocked_features; > > add comment here too, explain purpose and rules of use > Ok.
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 3893dc29eb2..eaad5b6a7a9 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -167,6 +167,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) } EXPORT_SYMBOL_GPL(virtio_add_status); +void virtio_block_feature(struct virtio_device *dev, unsigned int f) +{ + BUG_ON(f >= 64); + dev->blocked_features |= (1ULL << f); +} +EXPORT_SYMBOL_GPL(virtio_block_feature); + /* Do some validation, then set FEATURES_OK */ static int virtio_features_ok(struct virtio_device *dev) { @@ -234,17 +241,13 @@ void virtio_reset_device(struct virtio_device *dev) } EXPORT_SYMBOL_GPL(virtio_reset_device); -static int virtio_dev_probe(struct device *_d) +static int virtio_negotiate_features(struct virtio_device *dev) { - int err, i; - struct virtio_device *dev = dev_to_virtio(_d); struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); u64 device_features; u64 driver_features; u64 driver_features_legacy; - - /* We have a driver! */ - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); + int i, ret; /* Figure out what features the device supports. */ device_features = dev->config->get_features(dev); @@ -279,30 +282,61 @@ static int virtio_dev_probe(struct device *_d) if (device_features & (1ULL << i)) __virtio_set_bit(dev, i); - err = dev->config->finalize_features(dev); - if (err) - goto err; + /* Remove blocked features */ + dev->features &= ~dev->blocked_features; + + ret = dev->config->finalize_features(dev); + if (ret) + goto exit; if (drv->validate) { u64 features = dev->features; - err = drv->validate(dev); - if (err) - goto err; + ret = drv->validate(dev); + if (ret) + goto exit; /* Did validation change any features? Then write them again. */ if (features != dev->features) { - err = dev->config->finalize_features(dev); - if (err) - goto err; + ret = dev->config->finalize_features(dev); + if (ret) + goto exit; } } - err = virtio_features_ok(dev); - if (err) - goto err; + ret = virtio_features_ok(dev); +exit: + return ret; +} + +static int virtio_dev_probe(struct device *_d) +{ + int err; + struct virtio_device *dev = dev_to_virtio(_d); + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + u64 blocked_features; + bool renegotiate = true; + + /* We have a driver! */ + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); + + /* Store blocked features and attempt to negotiate features & probe. + * If the probe fails, we check if the driver has blocked any new features. + * If it has, we reset the device and try again with the new features. + */ + while (renegotiate) { + blocked_features = dev->blocked_features; + err = virtio_negotiate_features(dev); + if (err) + break; + + err = drv->probe(dev); + if (err && blocked_features != dev->blocked_features) + virtio_reset_device(dev); + else + renegotiate = false; + } - err = drv->probe(dev); if (err) goto err; @@ -319,7 +353,6 @@ static int virtio_dev_probe(struct device *_d) err: virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); return err; - } static void virtio_dev_remove(struct device *_d) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index b93238db94e..2de9b2d3ca4 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -109,6 +109,7 @@ int virtqueue_resize(struct virtqueue *vq, u32 num, * @vringh_config: configuration ops for host vrings. * @vqs: the list of virtqueues for this device. * @features: the features supported by both driver and device. + * @blocked_features: the features blocked by the driver that can't be negotiated. * @priv: private pointer for the driver's use. */ struct virtio_device { @@ -124,6 +125,7 @@ struct virtio_device { const struct vringh_config_ops *vringh_config; struct list_head vqs; u64 features; + u64 blocked_features; void *priv; }; @@ -133,6 +135,7 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status); int register_virtio_device(struct virtio_device *dev); void unregister_virtio_device(struct virtio_device *dev); bool is_virtio_device(struct device *dev); +void virtio_block_feature(struct virtio_device *dev, unsigned int f); void virtio_break_device(struct virtio_device *dev); void __virtio_unbreak_device(struct virtio_device *dev);
This patch exports a new virtio core function: virtio_block_feature. The function should be called during a virtio driver probe. If a virtio driver blocks features during probe and fails probe, virtio core will reset the device, try to re-negotiate the new features and probe again. Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com> --- drivers/virtio/virtio.c | 73 ++++++++++++++++++++++++++++++----------- include/linux/virtio.h | 3 ++ 2 files changed, 56 insertions(+), 20 deletions(-)