Re: [RFC v1 8/8] mshv: add vfio bridge device

On 09-07-2021 17:13, Wei Liu wrote:

+
+static int mshv_vfio_set_group(struct mshv_device *dev, long attr, u64 arg)
+{
+	struct mshv_vfio *mv = dev->private;
+	struct vfio_group *vfio_group;
+	struct mshv_vfio_group *mvg;
+	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
+	struct fd f;
+	int32_t fd;
+	int ret;
+
+	switch (attr) {
+	case MSHV_DEV_VFIO_GROUP_ADD:
+		if (get_user(fd, argp))
+			return -EFAULT;
+
+		f = fdget(fd);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = mshv_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&mv->lock);
+
+		list_for_each_entry(mvg, &mv->group_list, node) {
+			if (mvg->vfio_group == vfio_group) {
+				mutex_unlock(&mv->lock);
+				mshv_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		mvg = kzalloc(sizeof(*mvg), GFP_KERNEL_ACCOUNT);
+		if (!mvg) {
+			mutex_unlock(&mv->lock);
+			mshv_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&mvg->node, &mv->group_list);
+		mvg->vfio_group = vfio_group;
+
+		mutex_unlock(&mv->lock);
+
+		return 0;
+
+	case MSHV_DEV_VFIO_GROUP_DEL:
+		if (get_user(fd, argp))
+			return -EFAULT;
+
+		f = fdget(fd);
+		if (!f.file)
+			return -EBADF;

Can we move these both checks above switch statement and do fdput accordingly under both case statement accordingly?

+
+		ret = -ENOENT;
+
+		mutex_lock(&mv->lock);
+
+		list_for_each_entry(mvg, &mv->group_list, node) {
+			if (!mshv_vfio_external_group_match_file(mvg->vfio_group,
+								 f.file))
+				continue;
+
+			list_del(&mvg->node);
+			mshv_vfio_group_put_external_user(mvg->vfio_group);
+			kfree(mvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&mv->lock);
+
+		fdput(f);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int mshv_vfio_set_attr(struct mshv_device *dev,
+			      struct mshv_device_attr *attr)
+{
+	switch (attr->group) {
+	case MSHV_DEV_VFIO_GROUP:
+		return mshv_vfio_set_group(dev, attr->attr, attr->addr);
+	}
+
+	return -ENXIO;
+}
+
+static int mshv_vfio_has_attr(struct mshv_device *dev,
+			      struct mshv_device_attr *attr)
+{
+	switch (attr->group) {
+	case MSHV_DEV_VFIO_GROUP:
+		switch (attr->attr) {
+		case MSHV_DEV_VFIO_GROUP_ADD:
+		case MSHV_DEV_VFIO_GROUP_DEL:
+			return 0;
+		}
+
+		break;

do we need this break statement ? If not, lets remove it.

+	}
+
+	return -ENXIO;
+}
+
+static void mshv_vfio_destroy(struct mshv_device *dev)
+{
+	struct mshv_vfio *mv = dev->private;
+	struct mshv_vfio_group *mvg, *tmp;
+
+	list_for_each_entry_safe(mvg, tmp, &mv->group_list, node) {
+		mshv_vfio_group_put_external_user(mvg->vfio_group);
+		list_del(&mvg->node);
+		kfree(mvg);
+	}
+
+	kfree(mv);
+	kfree(dev);

We are freeing up dev. Please ignore my comment in caller patch. Thanks.

+}
+
+static int mshv_vfio_create(struct mshv_device *dev, u32 type);
+
+static struct mshv_device_ops mshv_vfio_ops = {
+	.name = "mshv-vfio",
+	.create = mshv_vfio_create,
+	.destroy = mshv_vfio_destroy,
+	.set_attr = mshv_vfio_set_attr,
+	.has_attr = mshv_vfio_has_attr,
+};

Regards,

~Praveen.