Re: [RFC v1 8/8] mshv: add vfio bridge device
flat view
From: Praveen Kumar <hidden>
Date: 2021-08-03 19:27:13
Also in:
lkml
On 09-07-2021 17:13, Wei Liu wrote:
quoted hunk ↗ jump to hunk
+ +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?
quoted hunk ↗ jump to hunk
+ + 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.
quoted hunk ↗ jump to hunk
+ } + + 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.
quoted hunk ↗ jump to hunk
+} + +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.