Re: [PATCH] vfio/cdx: add support for CDX bus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/5/2023 9:03 PM, Jason Gunthorpe wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


On Mon, Apr 03, 2023 at 07:55:25PM +0530, Nipun Gupta wrote:

+enum {
+     CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
+};

This seems to be missing the file2alias part.

Sure. Will add this.


+static void vfio_cdx_regions_cleanup(struct vfio_cdx_device *vdev)
+{
+     kfree(vdev->regions);
+}
+
+static int vfio_cdx_reset_device(struct vfio_cdx_device *vdev)
+{
+     return cdx_dev_reset(&vdev->cdx_dev->dev);
+}

Wrapper functions should be avoided.

Agree


+static void vfio_cdx_close_device(struct vfio_device *core_vdev)
+{
+     struct vfio_cdx_device *vdev =
+             container_of(core_vdev, struct vfio_cdx_device, vdev);
+     int ret;
+
+     vfio_cdx_regions_cleanup(vdev);
+
+     /* reset the device before cleaning up the interrupts */
+     ret = vfio_cdx_reset_device(vdev);
+     if (WARN_ON(ret))
+             dev_warn(core_vdev->dev,
+                      "VFIO_CDX: reset device has failed (%d)\n", ret);

This is pretty problematic.. if the reset can fail the device is
returned to the system in an unknown state and it seems pretty likely
that it can be a way to attack the kernel.

We will update the code to disable the device in case of failures.


+     case VFIO_DEVICE_RESET:
+     {
+             return vfio_cdx_reset_device(vdev);
+     }

What happens to MMIO access during this reset?

There is no config space here; and access to mmap space will go as usual but user may not see the expected behavior as the device is being reset. It is responsibility of user of VFIO device to serialize such access and reset.


+static int vfio_cdx_mmap_mmio(struct vfio_cdx_region region,
+                           struct vm_area_struct *vma)
+{
+     u64 size = vma->vm_end - vma->vm_start;
+     u64 pgoff, base;
+
+     pgoff = vma->vm_pgoff &
+             ((1U << (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
+     base = pgoff << PAGE_SHIFT;
+
+     if (region.size < PAGE_SIZE || base + size > region.size)
+             return -EINVAL;
+
+     vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
+     vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);

pgprot_device

Will update..


+     return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+                            size, vma->vm_page_prot);

io_remap_pfn_range
+static int vfio_cdx_mmap(struct vfio_device *core_vdev,
+                      struct vm_area_struct *vma)
+{
+     struct vfio_cdx_device *vdev =
+             container_of(core_vdev, struct vfio_cdx_device, vdev);
+     struct cdx_device *cdx_dev = vdev->cdx_dev;
+     unsigned int index;
+
+     index = vma->vm_pgoff >> (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT);
+
+     if (vma->vm_end < vma->vm_start)
+             return -EINVAL;
+     if (vma->vm_start & ~PAGE_MASK)
+             return -EINVAL;
+     if (vma->vm_end & ~PAGE_MASK)
+             return -EINVAL;

The core code already assures these checks.

Sure will remove these unnecessary checks.


+     if (!(vma->vm_flags & VM_SHARED))
+             return -EINVAL;
+     if (index >= cdx_dev->res_count)
+             return -EINVAL;
+
+     if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_MMAP))
+             return -EINVAL;
+
+     if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_READ) &&
+         (vma->vm_flags & VM_READ))
+             return -EINVAL;
+
+     if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE) &&
+         (vma->vm_flags & VM_WRITE))
+             return -EINVAL;
+
+     vma->vm_private_data = cdx_dev;

not needed

Okay


diff --git a/drivers/vfio/cdx/vfio_cdx_private.h b/drivers/vfio/cdx/vfio_cdx_private.h
new file mode 100644
index 000000000000..8b6f1ee3f5cd
--- /dev/null
+++ b/drivers/vfio/cdx/vfio_cdx_private.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
+ */
+
+#ifndef VFIO_CDX_PRIVATE_H
+#define VFIO_CDX_PRIVATE_H
+
+#define VFIO_CDX_OFFSET_SHIFT    40
+#define VFIO_CDX_OFFSET_MASK (((u64)(1) << VFIO_CDX_OFFSET_SHIFT) - 1)
+
+#define VFIO_CDX_OFFSET_TO_INDEX(off) ((off) >> VFIO_CDX_OFFSET_SHIFT)
+
+#define VFIO_CDX_INDEX_TO_OFFSET(index)      \
+     ((u64)(index) << VFIO_CDX_OFFSET_SHIFT)

use static inlines for function-line macros

Okay. Will update the code.


+struct vfio_cdx_region {
+     u32                     flags;
+     u32                     type;
+     u64                     addr;
+     resource_size_t         size;
+     void __iomem            *ioaddr;
+};
+
+struct vfio_cdx_device {
+     struct vfio_device      vdev;
+     struct cdx_device       *cdx_dev;
+     struct device           *dev;
+     struct vfio_cdx_region  *regions;
+};

This header file does not seem necessary right now

There is work in progress which would add support of MSI in VFIO-CDX driver, and where would be additional file/s. So we would like to keep the header file separate.

Thanks,
Nipun


Jason



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux