Re: [PATCH virtio v2 04/19] virtio: introduce virtio_queue_info struct and find_vqs_info() config op

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 04, 2024 at 03:00:01PM +0200, Jiri Pirko wrote:
> Thu, Jul 04, 2024 at 01:28:03PM CEST, mst@redhat.com wrote:
> >On Thu, Jul 04, 2024 at 01:06:12PM +0200, Jiri Pirko wrote:
> >> Thu, Jul 04, 2024 at 09:47:09AM CEST, mst@redhat.com wrote:
> >> >On Thu, Jul 04, 2024 at 08:43:35AM +0200, Jiri Pirko wrote:
> >> >> From: Jiri Pirko <jiri@nvidia.com>
> >> >> 
> >> >> Introduce a structure virtio_queue_info to carry name, callback and ctx
> >> >> together. In order to allow config implementations to accept config op
> >> >> with array of virtio_queue_info structures, introduce a new
> >> >> find_vqs_info() op. Do the needed conversion in virtio_find_vqs_ctx().
> >> >> Note that whole virtio_find_vqs_ctx() is going to be eventually removed
> >> >> at the and of this patchset.
> >> >> 
> >> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> >> ---
> >> >> v1->v2:
> >> >> - fixed comments for struct virtqueue_info
> >> >> - s/virtio_queue_info/virtqueue_info/
> >> >> ---
> >> >>  include/linux/virtio_config.h | 51 ++++++++++++++++++++++++++++++++---
> >> >>  1 file changed, 48 insertions(+), 3 deletions(-)
> >> >> 
> >> >> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> >> >> index 82a1d798b2f1..9b975c2830f6 100644
> >> >> --- a/include/linux/virtio_config.h
> >> >> +++ b/include/linux/virtio_config.h
> >> >> @@ -18,6 +18,16 @@ struct virtio_shm_region {
> >> >>  
> >> >>  typedef void vq_callback_t(struct virtqueue *);
> >> >>  
> >> >> +struct virtqueue_info {
> >> >> +	const char *name; /* Mainly for debugging, may be NULL for a virtqueue
> >> >> +			   * unused by driver.
> >> >> +			   */
> >> >> +	vq_callback_t *callback; /* May be NULL for vq that does
> >> >> +				  * not need a callback.
> >> >> +				  */
> >> >> +	bool ctx;
> >> >
> >> >
> >> >	/*
> >> >	 * Always
> >> >	 * like this
> >> >	 * outside of netdev code
> >> >	 * and it comes before the code being documented
> >> >	 */
> >> >
> >> >	/* Never
> >> >	 * like this
> >> >	 */
> >> 
> >> 
> >> How about to properly document the struct instead like this:
> >> /**
> >>  * struct virtqueue_info - info for a virtqueue passed to find_vqs()
> >>  * @name: mainly for debugging, NULL for a virtqueue
> >>  *        unused by the driver
> >>  * @callback: NULL for a virtqueue that does not need a callback
> >>  * @ctx: true to allow to maintain extra context per virtqueue
> >>  */
> >> struct virtqueue_info {
> >>         const char *name;
> >>         vq_callback_t *callback;
> >>         bool ctx;
> >> };
> >
> >Sure. And actually include the explanation what the fields are then?
> >@name: virtqueue description
> >@callback: a callback to invoke on a used buffer notification
> 
> Okay, how about:
> 
> /**
>  * struct virtqueue_info - Info for a virtqueue passed to find_vqs().
>  * @name: virtqueue description. Used mainly for debugging, NULL for
>  *        a virtqueue unused by the driver.
>  * @callback: A callback to invoke on a used buffer notification.
>  *            NULL for a virtqueue that does not need a callback.
>  * @ctx: A flag to indicate to maintain an extra context per virtqueue.
>  */
> struct virtqueue_info {
>         const char *name;
>         vq_callback_t *callback;
>         bool ctx;
> };
> 
> 
> ?


Looks good to me.

> 
> 
> >
> >
> >> 
> >> 
> >> >
> >> >
> >> >Also "may be null" is a bit confusing - it has to be null
> >> >for such a vq.
> >> >
> >> >Thus:
> >> >
> >> >	/*
> >> >	 * name: mainly for debugging, NULL for a virtqueue
> >> >         * unused by the driver.
> >> >	 */
> >> >	const char *name;
> >> >
> >> >and similarly for the callback.
> >> >
> >> >
> >> >
> >> >> +};
> >> >> +
> >> >>  /**
> >> >>   * struct virtio_config_ops - operations for configuring a virtio device
> >> >>   * Note: Do not assume that a transport implements all of the operations
> >> >> @@ -58,6 +68,12 @@ typedef void vq_callback_t(struct virtqueue *);
> >> >>   *	names: array of virtqueue names (mainly for debugging)
> >> >>   *		include a NULL entry for vqs unused by driver
> >> >>   *	Returns 0 on success or error status
> >> >> + * @find_vqs_info: find virtqueues and instantiate them.
> >> >> + *	vdev: the virtio_device
> >> >> + *	nvqs: the number of virtqueues to find
> >> >> + *	vqs: on success, includes new virtqueues
> >> >> + *	vqs_info: array of virtqueue info structures
> >> >> + *	Returns 0 on success or error status
> >> >>   * @del_vqs: free virtqueues found by find_vqs().
> >> >>   * @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
> >> >>   *      The function guarantees that all memory operations on the
> >> >> @@ -109,6 +125,10 @@ struct virtio_config_ops {
> >> >>  			struct virtqueue *vqs[], vq_callback_t *callbacks[],
> >> >>  			const char * const names[], const bool *ctx,
> >> >>  			struct irq_affinity *desc);
> >> >> +	int (*find_vqs_info)(struct virtio_device *vdev, unsigned int nvqs,
> >> >> +			     struct virtqueue *vqs[],
> >> >> +			     struct virtqueue_info vqs_info[],
> >> >> +			     struct irq_affinity *desc);
> >> >>  	void (*del_vqs)(struct virtio_device *);
> >> >>  	void (*synchronize_cbs)(struct virtio_device *);
> >> >>  	u64 (*get_features)(struct virtio_device *vdev);
> >> >> @@ -117,7 +137,7 @@ struct virtio_config_ops {
> >> >>  	int (*set_vq_affinity)(struct virtqueue *vq,
> >> >>  			       const struct cpumask *cpu_mask);
> >> >>  	const struct cpumask *(*get_vq_affinity)(struct virtio_device *vdev,
> >> >> -			int index);
> >> >> +						 int index);
> >> >>  	bool (*get_shm_region)(struct virtio_device *vdev,
> >> >>  			       struct virtio_shm_region *region, u8 id);
> >> >>  	int (*disable_vq_and_reset)(struct virtqueue *vq);
> >> >> @@ -210,14 +230,39 @@ static inline bool virtio_has_dma_quirk(const struct virtio_device *vdev)
> >> >>  	return !virtio_has_feature(vdev, VIRTIO_F_ACCESS_PLATFORM);
> >> >>  }
> >> >>  
> >> >> +static inline
> >> >> +int virtio_find_vqs_info(struct virtio_device *vdev, unsigned int nvqs,
> >> >> +			 struct virtqueue *vqs[],
> >> >> +			 struct virtqueue_info vqs_info[],
> >> >> +			 struct irq_affinity *desc)
> >> >> +{
> >> >> +	return vdev->config->find_vqs_info(vdev, nvqs, vqs, vqs_info, desc);
> >> >> +}
> >> >> +
> >> >>  static inline
> >> >>  int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
> >> >>  			struct virtqueue *vqs[], vq_callback_t *callbacks[],
> >> >>  			const char * const names[], const bool *ctx,
> >> >>  			struct irq_affinity *desc)
> >> >>  {
> >> >> -	return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, ctx,
> >> >> -				      desc);
> >> >> +	struct virtqueue_info *vqs_info;
> >> >> +	int err, i;
> >> >> +
> >> >> +	if (!vdev->config->find_vqs_info)
> >> >> +		return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks,
> >> >> +					      names, ctx, desc);
> >> >> +
> >> >> +	vqs_info = kmalloc_array(nvqs, sizeof(*vqs_info), GFP_KERNEL);
> >> >> +	if (!vqs_info)
> >> >> +		return -ENOMEM;
> >> >> +	for (i = 0; i < nvqs; i++) {
> >> >> +		vqs_info[i].name = names[i];
> >> >> +		vqs_info[i].callback = callbacks[i];
> >> >> +		vqs_info[i].ctx = ctx ? ctx[i] : false;
> >> >> +	}
> >> >> +	err = virtio_find_vqs_info(vdev, nvqs, vqs, vqs_info, desc);
> >> >> +	kfree(vqs_info);
> >> >> +	return err;
> >> >>  }
> >> >>  
> >> >>  static inline
> >> >> -- 
> >> >> 2.45.2
> >> >
> >