On Wed, Dec 06, 2023 at 10:38:57AM +0200, Yishai Hadas wrote:
> +static ssize_t
> +virtiovf_pci_core_read(struct vfio_device *core_vdev, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct virtiovf_pci_core_device *virtvdev = container_of(
> + core_vdev, struct virtiovf_pci_core_device, core_device.vdev);
> + struct pci_dev *pdev = virtvdev->core_device.pdev;
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> + int ret;
> +
> + if (!count)
> + return 0;
> +
> + if (index == VFIO_PCI_CONFIG_REGION_INDEX)
> + return virtiovf_pci_read_config(core_vdev, buf, count, ppos);
> +
> + if (index != VFIO_PCI_BAR0_REGION_INDEX)
> + return vfio_pci_core_read(core_vdev, buf, count, ppos);
> +
> + ret = pm_runtime_resume_and_get(&pdev->dev);
> + if (ret) {
> + pci_info_ratelimited(pdev, "runtime resume failed %d\n",
> + ret);
> + return -EIO;
> + }
> +
> + ret = translate_io_bar_to_mem_bar(virtvdev, pos, buf, count, true);
> + pm_runtime_put(&pdev->dev);
There is two copies of this pm_runtime sequence, I'd put the
pm_runtime stuff into translate_io_bar_to_mem_bar() and organize these
to be more success oriented:
if (index == VFIO_PCI_BAR0_REGION_INDEX)
return translate_io_bar_to_mem_bar();
return vfio_pci_core_read();
> +static ssize_t
> +virtiovf_pci_core_write(struct vfio_device *core_vdev, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct virtiovf_pci_core_device *virtvdev = container_of(
> + core_vdev, struct virtiovf_pci_core_device, core_device.vdev);
> + struct pci_dev *pdev = virtvdev->core_device.pdev;
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> + int ret;
> +
> + if (!count)
> + return 0;
> +
> + if (index == VFIO_PCI_CONFIG_REGION_INDEX) {
> + size_t register_offset;
> + loff_t copy_offset;
> + size_t copy_count;
> +
> + if (range_intersect_range(pos, count, PCI_COMMAND, sizeof(virtvdev->pci_cmd),
> + ©_offset, ©_count,
> + ®ister_offset)) {
> + if (copy_from_user((void *)&virtvdev->pci_cmd + register_offset,
> + buf + copy_offset,
> + copy_count))
> + return -EFAULT;
> + }
> +
> + if (range_intersect_range(pos, count, PCI_BASE_ADDRESS_0,
> + sizeof(virtvdev->pci_base_addr_0),
> + ©_offset, ©_count,
> + ®ister_offset)) {
> + if (copy_from_user((void *)&virtvdev->pci_base_addr_0 + register_offset,
> + buf + copy_offset,
> + copy_count))
> + return -EFAULT;
> + }
> + }
> +
> + if (index != VFIO_PCI_BAR0_REGION_INDEX)
> + return vfio_pci_core_write(core_vdev, buf, count, ppos);
> +
> + ret = pm_runtime_resume_and_get(&pdev->dev);
> + if (ret) {
> + pci_info_ratelimited(pdev, "runtime resume failed %d\n", ret);
> + return -EIO;
> + }
> +
> + ret = translate_io_bar_to_mem_bar(virtvdev, pos, (char __user *)buf, count, false);
> + pm_runtime_put(&pdev->dev);
Same
> +static const struct vfio_device_ops virtiovf_vfio_pci_tran_ops = {
> + .name = "virtio-vfio-pci-trans",
> + .init = virtiovf_pci_init_device,
> + .release = virtiovf_pci_core_release_dev,
> + .open_device = virtiovf_pci_open_device,
> + .close_device = vfio_pci_core_close_device,
> + .ioctl = virtiovf_vfio_pci_core_ioctl,
> + .read = virtiovf_pci_core_read,
> + .write = virtiovf_pci_core_write,
> + .mmap = vfio_pci_core_mmap,
> + .request = vfio_pci_core_request,
> + .match = vfio_pci_core_match,
> + .bind_iommufd = vfio_iommufd_physical_bind,
> + .unbind_iommufd = vfio_iommufd_physical_unbind,
> + .attach_ioas = vfio_iommufd_physical_attach_ioas,
Missing detach_ioas
> +static bool virtiovf_bar0_exists(struct pci_dev *pdev)
> +{
> + struct resource *res = pdev->resource;
> +
> + return res->flags ? true : false;
?: isn't necessary cast to bool does this expression automatically
I didn't try to check the virtio parts of this, but the construction
of the variant driver looks OK, so
Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Jason
