On Thu, Oct 30, 2025 at 02:38:36PM -0600, Alex Williamson wrote:
> On Mon, 13 Oct 2025 18:26:11 +0300
> Leon Romanovsky <[email protected]> wrote:
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index fe247d0e2831..56b1320238a9 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -1511,6 +1520,19 @@ int vfio_pci_core_ioctl_feature(struct vfio_device
> > *device, u32 flags,
> > return vfio_pci_core_pm_exit(vdev, flags, arg, argsz);
> > case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
> > return vfio_pci_core_feature_token(vdev, flags, arg, argsz);
> > + case VFIO_DEVICE_FEATURE_DMA_BUF:
> > + if (device->ops->ioctl != vfio_pci_core_ioctl)
> > + /*
> > + * Devices that overwrite general .ioctl() callback
> > + * usually do it to implement their own
> > + * VFIO_DEVICE_GET_REGION_INFO handlerm and they present
>
> Typo, "handlerm"
Thanks, this part of code is going to be different in v6.
>
<...>
> > @@ -2482,6 +2506,10 @@ static int vfio_pci_dev_set_hot_reset(struct
> > vfio_device_set *dev_set,
> >
> > ret = pci_reset_bus(pdev);
> >
> > + list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list)
> > + if (__vfio_pci_memory_enabled(vdev))
> > + vfio_pci_dma_buf_move(vdev, false);
> > +
> > vdev = list_last_entry(&dev_set->device_list,
> > struct vfio_pci_core_device, vdev.dev_set_list);
> >
>
> This needs to be placed in the existing undo loop with the up_write(),
> otherwise it can be missed in the error case.
I'll move, but it caused me to wonder what did you want to achieve with
this "vdev = list_last_entry ..." line? vdev is overwritten immediately
after that line.
>
> > diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c
> > b/drivers/vfio/pci/vfio_pci_dmabuf.c
> > new file mode 100644
> > index 000000000000..eaba010777f3
> > --- /dev/null
> > +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> > +static unsigned int calc_sg_nents(struct vfio_pci_dma_buf *priv,
> > + struct dma_iova_state *state)
> > +{
> > + struct phys_vec *phys_vec = priv->phys_vec;
> > + unsigned int nents = 0;
> > + u32 i;
> > +
> > + if (!state || !dma_use_iova(state))
> > + for (i = 0; i < priv->nr_ranges; i++)
> > + nents += DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);
> > + else
> > + /*
> > + * In IOVA case, there is only one SG entry which spans
> > + * for whole IOVA address space, but we need to make sure
> > + * that it fits sg->length, maybe we need more.
> > + */
> > + nents = DIV_ROUND_UP(priv->size, UINT_MAX);
>
> I think we're arguably running afoul of the coding style standard here
> that this is not a single simple statement and should use braces.
>
<...>
> > +err_unmap_dma:
> > + if (!i || !state)
> > + ; /* Do nothing */
> > + else if (dma_use_iova(state))
> > + dma_iova_destroy(attachment->dev, state, mapped_len, dir,
> > + attrs);
> > + else
> > + for_each_sgtable_dma_sg(sgt, sgl, i)
> > + dma_unmap_phys(attachment->dev, sg_dma_address(sgl),
> > + sg_dma_len(sgl), dir, attrs);
>
> Same, here for braces.
>
<...>
> > + if (!state)
> > + ; /* Do nothing */
> > + else if (dma_use_iova(state))
> > + dma_iova_destroy(attachment->dev, state, priv->size, dir,
> > + attrs);
> > + else
> > + for_each_sgtable_dma_sg(sgt, sgl, i)
> > + dma_unmap_phys(attachment->dev, sg_dma_address(sgl),
> > + sg_dma_len(sgl), dir, attrs);
> > +
>
> Here too.
I will change it, but it is worth to admit that I'm consistent in my
coding style.
>
> > + sg_free_table(sgt);
> > + kfree(sgt);
> > +}
> ...
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 75100bf009ba..63214467c875 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -1478,6 +1478,31 @@ struct vfio_device_feature_bus_master {
> > };
> > #define VFIO_DEVICE_FEATURE_BUS_MASTER 10
> >
> > +/**
> > + * Upon VFIO_DEVICE_FEATURE_GET create a dma_buf fd for the
> > + * regions selected.
> > + *
> > + * open_flags are the typical flags passed to open(2), eg O_RDWR,
> > O_CLOEXEC,
> > + * etc. offset/length specify a slice of the region to create the dmabuf
> > from.
> > + * nr_ranges is the total number of (P2P DMA) ranges that comprise the
> > dmabuf.
> > + *
>
> Probably worth noting that .flags should be zero, I see we enforce
> that. Thanks,
Added, thanks
>
> Alex
>
