On Wed, Aug 04, 2021 at 12:57:03AM +0530, Praveen Kumar wrote:
> 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?
Fair point. This can be done, albeit at the cost of having a rather
different code structure.
I was waiting to see if we should somehow merge this with KVM's
implementation so the code was deliberately kept close. If there is no
further comment I can of course make the change you suggested.
>
> > +
> > + 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.
Will do.
Wei.
