by Wei Liu

[permalink] [raw]

Subject: Re: [RFC v1 5/8] mshv: add paravirtualized IOMMU support

On Wed, Aug 04, 2021 at 12:13:42PM +0530, Praveen Kumar wrote:
> On 04-08-2021 03:17, Wei Liu wrote:
> >>> +static size_t hv_iommu_unmap(struct iommu_domain *d, unsigned long iova,
> >>> + size_t size, struct iommu_iotlb_gather *gather)
> >>> +{
> >>> + size_t unmapped;
> >>> + struct hv_iommu_domain *domain = to_hv_iommu_domain(d);
> >>> + unsigned long flags, npages;
> >>> + struct hv_input_unmap_device_gpa_pages *input;
> >>> + u64 status;
> >>> +
> >>> + unmapped = hv_iommu_del_mappings(domain, iova, size);
> >>> + if (unmapped < size)
> >>> + return 0;
> >> Is there a case where unmapped > 0 && unmapped < size ?
> >>
> > There could be such a case -- hv_iommu_del_mappings' return value is >= 0.
> > Is there a problem with this predicate?
>
> What I understand, if we are unmapping and return 0, means nothing was
> unmapped, and will that not cause any corruption or illegal access of
> unmapped memory later? From __iommu_unmap

Those pages are not really unmapped. The hypercall is skipped.

> ...
> 13 while (unmapped < size) {
> 12 size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
> 11
> 10 unmapped_page = ops->unmap(domain, iova, pgsize, iotlb_gather);
> 9 if (!unmapped_page)
> 8 break; <<< we just break here, thinking there is nothing unmapped, but actually hv_iommu_del_mappings has removed some pages.
> 7
> 6 pr_debug("unmapped: iova 0x%lx size 0x%zx\n",
> 5 ?iova, unmapped_page);
> 4
> 3 iova += unmapped_page;
> 2 unmapped += unmapped_page;
> 1 }
> ...
>
> Am I missing something ?
>
> Regards,
>
> ~Praveen.

2021-08-10 16:24:08

by Wei Liu

[permalink] [raw]

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

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.