Linux Virtualization - Apr 2023 - [RFC PATCH net 1/3] virtio: re-negotiate features if probe fails and features are blocked

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 at 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);
+
 /* 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);
-- 
2.34.1

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 at 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