Re: [PATCH vringh] virtio: Introduce vringh wrappers in virtio_config

[Rusty Russell <rusty@rustcorp.com.au>]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add wrappers for the host vrings to support loose
> coupling between the virtio device and driver.
>
> The functions find_vrhs() and del_vrhs() are added to
> struct virtio_config_ops to manage the host vrings.
> The function vringh_notify() is added so the guest
> can be kicked when buffers are added to the used-ring.
>
> This enables the virtio drivers to manage the virtio rings
> without knowledge of how the host vrings are managed.

Hmm, this is a bit weird.

> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 29b9104..88dd5ae 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -51,7 +51,17 @@
>   *      This returns a pointer to the bus name a la pci_name from which
>   *      the caller can then copy.
>   * @set_vq_affinity: set the affinity for a virtqueue.
> + * @find_vrhs: find the host vrings and instantiate them
> + *	vdev: the virtio_device
> + *	nhvrs: the number of host vrings to find
> + *	hvrs: on success, includes new host vrings
> + *	callbacks: array of driver callbacks, for each host vring
> + *		include a NULL entry for vqs that do not need a callback
> + *	Returns 0 on success or error status
> + * @del_vrhs: free the host vrings found by find_vrhs().
>   */
> +struct vringh;
> +typedef void vrh_callback_t(struct virtio_device *, struct vringh *);
>  typedef void vq_callback_t(struct virtqueue *);
>  struct virtio_config_ops {
>  	void (*get)(struct virtio_device *vdev, unsigned offset,
> @@ -70,6 +80,9 @@ struct virtio_config_ops {
>  	void (*finalize_features)(struct virtio_device *vdev);
>  	const char *(*bus_name)(struct virtio_device *vdev);
>  	int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
> +	int (*find_vrhs)(struct virtio_device *vdev, unsigned nhvrs,
> +			 struct vringh *vrhs[], vrh_callback_t *callbacks[]);
> +	void (*del_vrhs)(struct virtio_device *vdev);
>  };
>  
>  /* If driver didn't advertise the feature, it will never appear. */

It's weird that you conflate the host and guest ring sides in rpmsg, but
that might make sense if they're really bound together.  However, in
general they are not: it's normal to be a guest or host, not both.

This implies that you need a struct vringh_config, to put this in.

> diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> index ab41185..8156f51 100644
> --- a/include/linux/vringh.h
> +++ b/include/linux/vringh.h
> @@ -50,6 +50,12 @@ struct vringh {
>  
>  	/* The vring (note: it may contain user pointers!) */
>  	struct vring vring;
> +
> +	/* The function to call when buffers are available */
> +	void (*notify)(struct vringh *);
> +
> +	/* A pointer for the vringh clients to use. */
> +	void *priv;
>  };

Since the caller allocates the vringh, can it not use container_of() 
instead of a priv pointer?

Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization