Re: Re: [PATCH v7 11/12] vduse: Introduce VDUSE - vDPA Device in Userspace

Hi Greg,

Thanks a lot for the review!

On Mon, May 31, 2021 at 12:56 PM Greg KH [off-list ref] wrote:

On Mon, May 17, 2021 at 05:55:12PM +0800, Xie Yongji wrote:

quoted

+struct vduse_dev {
+     struct vduse_vdpa *vdev;
+     struct device dev;
+     struct cdev cdev;

You now have 2 reference counted devices controling the lifespace of a
single structure.  A mess that is guaranteed to go wrong.  Please never
do this.

These two are both used by cdev_device_add(). Looks like I didn't find
any problem. Any suggestions?

quoted

+     struct vduse_virtqueue *vqs;
+     struct vduse_iova_domain *domain;
+     char *name;
+     struct mutex lock;
+     spinlock_t msg_lock;
+     atomic64_t msg_unique;

Why do you need an atomic and a lock?

You are right. We don't need an atomic here.

quoted

+     wait_queue_head_t waitq;
+     struct list_head send_list;
+     struct list_head recv_list;
+     struct list_head list;
+     struct vdpa_callback config_cb;
+     struct work_struct inject;
+     spinlock_t irq_lock;
+     unsigned long api_version;
+     bool connected;
+     int minor;
+     u16 vq_size_max;
+     u32 vq_num;
+     u32 vq_align;
+     u32 config_size;
+     u32 device_id;
+     u32 vendor_id;
+};
+
+struct vduse_dev_msg {
+     struct vduse_dev_request req;
+     struct vduse_dev_response resp;
+     struct list_head list;
+     wait_queue_head_t waitq;
+     bool completed;
+};
+
+struct vduse_control {
+     unsigned long api_version;

u64?

OK.

quoted

+};
+
+static unsigned long max_bounce_size = (64 * 1024 * 1024);
+module_param(max_bounce_size, ulong, 0444);
+MODULE_PARM_DESC(max_bounce_size, "Maximum bounce buffer size. (default: 64M)");
+
+static unsigned long max_iova_size = (128 * 1024 * 1024);
+module_param(max_iova_size, ulong, 0444);
+MODULE_PARM_DESC(max_iova_size, "Maximum iova space size (default: 128M)");
+
+static bool allow_unsafe_device_emulation;
+module_param(allow_unsafe_device_emulation, bool, 0444);
+MODULE_PARM_DESC(allow_unsafe_device_emulation, "Allow emulating unsafe device."
+     " We must make sure the userspace device emulation process is trusted."
+     " Otherwise, don't enable this option. (default: false)");
+

This is not the 1990's anymore, please never use module parameters, make
these per-device attributes if you really need them.

These parameters will be used before the device is created. Or do you
mean add some attributes to the control device?

quoted

+static int vduse_init(void)
+{
+     int ret;
+
+     if (max_bounce_size >= max_iova_size)
+             return -EINVAL;
+
+     ret = misc_register(&vduse_misc);
+     if (ret)
+             return ret;
+
+     vduse_class = class_create(THIS_MODULE, "vduse");

If you have a misc device, you do not need to create a class at the same
time.  Why are you doing both here?  Just stick with the misc device, no
need for anything else.

The misc device is the control device represented by
/dev/vduse/control. Then a VDUSE device represented by
/dev/vduse/$NAME can be created by the ioctl(VDUSE_CREATE_DEV) on this
control device.

quoted

+     if (IS_ERR(vduse_class)) {
+             ret = PTR_ERR(vduse_class);
+             goto err_class;
+     }
+     vduse_class->devnode = vduse_devnode;
+
+     ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");

Wait, you want a whole major?  What is the misc device for?

The misc device is used for controlling. And VDUSE devices are
allocated in a dynamic chardev range. Or do I need to reserve the
first minor for the control device instead?

quoted

+MODULE_VERSION(DRV_VERSION);

MODULE_VERSION() makes no sense when the code is merged into the kernel
tree, so you can just drop that.

Will do it.

Thanks,
Yongji