vfio-cdx driver enables IOCTLs for user space to query
MMIO regions for CDX devices and mmap them. This change
also adds support for reset of CDX devices.
This change adds the VFIO CDX driver and enables the following
ioctls for CDX devices:
- VFIO_DEVICE_GET_INFO:
- VFIO_DEVICE_GET_REGION_INFO
- VFIO_DEVICE_RESET
Signed-off-by: Nipun Gupta <[email protected]>
Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
Tested-by: Nikhil Agarwal <[email protected]>
---
Changes v1->v2:
- Updated file2alias to support vfio_cdx
- removed some un-necessary checks in mmap
- removed vfio reset wrapper API
- converted complex macros to static APIs
- used pgprot_device and io_remap_pfn_range
MAINTAINERS | 7 +
drivers/vfio/Kconfig | 1 +
drivers/vfio/Makefile | 1 +
drivers/vfio/cdx/Kconfig | 17 ++
drivers/vfio/cdx/Makefile | 8 +
drivers/vfio/cdx/vfio_cdx.c | 290 ++++++++++++++++++++++++++++
drivers/vfio/cdx/vfio_cdx_private.h | 31 +++
include/linux/cdx/cdx_bus.h | 1 -
include/linux/mod_devicetable.h | 6 +
scripts/mod/devicetable-offsets.c | 1 +
scripts/mod/file2alias.c | 17 +-
11 files changed, 378 insertions(+), 2 deletions(-)
create mode 100644 drivers/vfio/cdx/Kconfig
create mode 100644 drivers/vfio/cdx/Makefile
create mode 100644 drivers/vfio/cdx/vfio_cdx.c
create mode 100644 drivers/vfio/cdx/vfio_cdx_private.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f74d8571ac9..c4fd42ba8f46 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22064,6 +22064,13 @@ F: Documentation/filesystems/vfat.rst
F: fs/fat/
F: tools/testing/selftests/filesystems/fat/
+VFIO CDX DRIVER
+M: Nipun Gupta <[email protected]>
+M: Nikhil Agarwal <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/vfio/cdx/*
+
VFIO DRIVER
M: Alex Williamson <[email protected]>
L: [email protected]
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 89e06c981e43..aba36f5be4ec 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -57,6 +57,7 @@ source "drivers/vfio/pci/Kconfig"
source "drivers/vfio/platform/Kconfig"
source "drivers/vfio/mdev/Kconfig"
source "drivers/vfio/fsl-mc/Kconfig"
+source "drivers/vfio/cdx/Kconfig"
endif
source "virt/lib/Kconfig"
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 70e7dcb302ef..1a27b2516612 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_VFIO_PCI) += pci/
obj-$(CONFIG_VFIO_PLATFORM) += platform/
obj-$(CONFIG_VFIO_MDEV) += mdev/
obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
+obj-$(CONFIG_VFIO_CDX) += cdx/
diff --git a/drivers/vfio/cdx/Kconfig b/drivers/vfio/cdx/Kconfig
new file mode 100644
index 000000000000..e6de0a0caa32
--- /dev/null
+++ b/drivers/vfio/cdx/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# VFIO CDX configuration
+#
+# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
+#
+
+config VFIO_CDX
+ tristate "VFIO support for CDX bus devices"
+ depends on CDX_BUS
+ select EVENTFD
+ help
+ Driver to enable VFIO support for the devices on CDX bus.
+ This is required to make use of CDX devices present in
+ the system using the VFIO framework.
+
+ If you don't know what to do here, say N.
diff --git a/drivers/vfio/cdx/Makefile b/drivers/vfio/cdx/Makefile
new file mode 100644
index 000000000000..82e4ef412c0f
--- /dev/null
+++ b/drivers/vfio/cdx/Makefile
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
+#
+
+obj-$(CONFIG_VFIO_CDX) += vfio-cdx.o
+
+vfio-cdx-objs := vfio_cdx.o
diff --git a/drivers/vfio/cdx/vfio_cdx.c b/drivers/vfio/cdx/vfio_cdx.c
new file mode 100644
index 000000000000..c19062ce7680
--- /dev/null
+++ b/drivers/vfio/cdx/vfio_cdx.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/vfio.h>
+#include <linux/cdx/cdx_bus.h>
+
+#include "vfio_cdx_private.h"
+
+static struct cdx_driver vfio_cdx_driver;
+
+static int vfio_cdx_init_device(struct vfio_device *core_vdev)
+{
+ struct vfio_cdx_device *vdev =
+ container_of(core_vdev, struct vfio_cdx_device, vdev);
+ struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
+
+ vdev->cdx_dev = cdx_dev;
+ vdev->dev = &cdx_dev->dev;
+
+ return 0;
+}
+
+static void vfio_cdx_release_device(struct vfio_device *core_vdev)
+{
+ struct vfio_cdx_device *vdev =
+ container_of(core_vdev, struct vfio_cdx_device, vdev);
+
+ vdev->cdx_dev = NULL;
+ vdev->dev = NULL;
+}
+
+/**
+ * CDX_DRIVER_OVERRIDE_DEVICE_VFIO - macro used to describe a VFIO
+ * "driver_override" CDX device.
+ * @vend: the 16 bit CDX Vendor ID
+ * @dev: the 16 bit CDX Device ID
+ *
+ * This macro is used to create a struct cdx_device_id that matches a
+ * specific device. driver_override will be set to
+ * CDX_ID_F_VFIO_DRIVER_OVERRIDE.
+ */
+#define CDX_DRIVER_OVERRIDE_DEVICE_VFIO(vend, dev) \
+ CDX_DEVICE_DRIVER_OVERRIDE(vend, dev, CDX_ID_F_VFIO_DRIVER_OVERRIDE)
+
+static int vfio_cdx_open_device(struct vfio_device *core_vdev)
+{
+ struct vfio_cdx_device *vdev =
+ container_of(core_vdev, struct vfio_cdx_device, vdev);
+ struct cdx_device *cdx_dev = vdev->cdx_dev;
+ int count = cdx_dev->res_count;
+ int i;
+
+ vdev->regions = kcalloc(count, sizeof(struct vfio_cdx_region),
+ GFP_KERNEL);
+ if (!vdev->regions)
+ return -ENOMEM;
+
+ for (i = 0; i < count; i++) {
+ struct resource *res = &cdx_dev->res[i];
+
+ vdev->regions[i].addr = res->start;
+ vdev->regions[i].size = resource_size(res);
+ vdev->regions[i].type = res->flags;
+ /*
+ * Only regions addressed with PAGE granularity may be
+ * MMAP'ed securely.
+ */
+ if (!(vdev->regions[i].addr & ~PAGE_MASK) &&
+ !(vdev->regions[i].size & ~PAGE_MASK))
+ vdev->regions[i].flags |=
+ VFIO_REGION_INFO_FLAG_MMAP;
+ vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
+ if (!(cdx_dev->res[i].flags & IORESOURCE_READONLY))
+ vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
+ }
+
+ return 0;
+}
+
+static void vfio_cdx_regions_cleanup(struct vfio_cdx_device *vdev)
+{
+ kfree(vdev->regions);
+}
+
+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 = cdx_dev_reset(&vdev->cdx_dev->dev);
+ if (WARN_ON(ret))
+ dev_warn(core_vdev->dev,
+ "VFIO_CDX: reset device has failed (%d)\n", ret);
+}
+
+static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
+ unsigned int cmd, unsigned long arg)
+{
+ struct vfio_cdx_device *vdev =
+ container_of(core_vdev, struct vfio_cdx_device, vdev);
+ struct cdx_device *cdx_dev = vdev->cdx_dev;
+ unsigned long minsz;
+
+ switch (cmd) {
+ case VFIO_DEVICE_GET_INFO:
+ {
+ struct vfio_device_info info;
+
+ minsz = offsetofend(struct vfio_device_info, num_irqs);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ info.flags = VFIO_DEVICE_FLAGS_RESET;
+
+ info.num_regions = cdx_dev->res_count;
+ info.num_irqs = 0;
+
+ return copy_to_user((void __user *)arg, &info, minsz) ?
+ -EFAULT : 0;
+ }
+ case VFIO_DEVICE_GET_REGION_INFO:
+ {
+ struct vfio_region_info info;
+
+ minsz = offsetofend(struct vfio_region_info, offset);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ if (info.index >= cdx_dev->res_count)
+ return -EINVAL;
+
+ /* map offset to the physical address */
+ info.offset = vfio_cdx_index_to_offset(info.index);
+ info.size = vdev->regions[info.index].size;
+ info.flags = vdev->regions[info.index].flags;
+
+ if (copy_to_user((void __user *)arg, &info, minsz))
+ return -EFAULT;
+ return 0;
+ }
+ case VFIO_DEVICE_RESET:
+ {
+ return cdx_dev_reset(&vdev->cdx_dev->dev);
+ }
+ default:
+ return -ENOTTY;
+ }
+}
+
+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_device(vma->vm_page_prot);
+
+ return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+ size, vma->vm_page_prot);
+}
+
+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 (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;
+
+ return vfio_cdx_mmap_mmio(vdev->regions[index], vma);
+}
+
+static const struct vfio_device_ops vfio_cdx_ops = {
+ .name = "vfio-cdx",
+ .init = vfio_cdx_init_device,
+ .release = vfio_cdx_release_device,
+ .open_device = vfio_cdx_open_device,
+ .close_device = vfio_cdx_close_device,
+ .ioctl = vfio_cdx_ioctl,
+ .mmap = vfio_cdx_mmap,
+};
+
+static int vfio_cdx_probe(struct cdx_device *cdx_dev)
+{
+ struct vfio_cdx_device *vdev = NULL;
+ struct device *dev = &cdx_dev->dev;
+ int ret;
+
+ vdev = vfio_alloc_device(vfio_cdx_device, vdev, dev,
+ &vfio_cdx_ops);
+ if (IS_ERR(vdev))
+ return PTR_ERR(vdev);
+
+ ret = vfio_register_group_dev(&vdev->vdev);
+ if (ret) {
+ dev_err(dev, "VFIO_CDX: Failed to add to vfio group\n");
+ goto out_uninit;
+ }
+
+ dev_set_drvdata(dev, vdev);
+ return 0;
+
+out_uninit:
+ vfio_put_device(&vdev->vdev);
+ return ret;
+}
+
+static int vfio_cdx_remove(struct cdx_device *cdx_dev)
+{
+ struct device *dev = &cdx_dev->dev;
+ struct vfio_cdx_device *vdev;
+
+ vdev = dev_get_drvdata(dev);
+ vfio_unregister_group_dev(&vdev->vdev);
+ vfio_put_device(&vdev->vdev);
+
+ return 0;
+}
+
+static const struct cdx_device_id vfio_cdx_table[] = {
+ { CDX_DRIVER_OVERRIDE_DEVICE_VFIO(CDX_ANY_ID, CDX_ANY_ID) }, /* match all by default */
+ {}
+};
+
+MODULE_DEVICE_TABLE(cdx, vfio_cdx_table);
+
+static struct cdx_driver vfio_cdx_driver = {
+ .probe = vfio_cdx_probe,
+ .remove = vfio_cdx_remove,
+ .match_id_table = vfio_cdx_table,
+ .driver = {
+ .name = "vfio-cdx",
+ .owner = THIS_MODULE,
+ },
+ .driver_managed_dma = true,
+};
+
+static int __init vfio_cdx_driver_init(void)
+{
+ return cdx_driver_register(&vfio_cdx_driver);
+}
+
+static void __exit vfio_cdx_driver_exit(void)
+{
+ cdx_driver_unregister(&vfio_cdx_driver);
+}
+
+module_init(vfio_cdx_driver_init);
+module_exit(vfio_cdx_driver_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("VFIO for CDX devices - User Level meta-driver");
diff --git a/drivers/vfio/cdx/vfio_cdx_private.h b/drivers/vfio/cdx/vfio_cdx_private.h
new file mode 100644
index 000000000000..5f143736eb7a
--- /dev/null
+++ b/drivers/vfio/cdx/vfio_cdx_private.h
@@ -0,0 +1,31 @@
+/* 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
+
+static inline u64 vfio_cdx_index_to_offset(u32 index)
+{
+ return ((u64)(index) << VFIO_CDX_OFFSET_SHIFT);
+}
+
+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;
+};
+
+#endif /* VFIO_CDX_PRIVATE_H */
diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
index 35ef41d8a61a..bead71b7bc73 100644
--- a/include/linux/cdx/cdx_bus.h
+++ b/include/linux/cdx/cdx_bus.h
@@ -14,7 +14,6 @@
#include <linux/mod_devicetable.h>
#define MAX_CDX_DEV_RESOURCES 4
-#define CDX_ANY_ID (0xFFFF)
#define CDX_CONTROLLER_ID_SHIFT 4
#define CDX_BUS_NUM_MASK 0xF
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index ccaaeda792c0..ccf017353bb6 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -912,6 +912,12 @@ struct ishtp_device_id {
kernel_ulong_t driver_data;
};
+#define CDX_ANY_ID (0xFFFF)
+
+enum {
+ CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
+};
+
/**
* struct cdx_device_id - CDX device identifier
* @vendor: Vendor ID
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 62dc988df84d..abe65f8968dd 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -265,6 +265,7 @@ int main(void)
DEVID(cdx_device_id);
DEVID_FIELD(cdx_device_id, vendor);
DEVID_FIELD(cdx_device_id, device);
+ DEVID_FIELD(cdx_device_id, override_only);
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 28da34ba4359..38120f932b0d 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1458,8 +1458,23 @@ static int do_cdx_entry(const char *filename, void *symval,
{
DEF_FIELD(symval, cdx_device_id, vendor);
DEF_FIELD(symval, cdx_device_id, device);
+ DEF_FIELD(symval, cdx_device_id, override_only);
- sprintf(alias, "cdx:v%08Xd%08Xd", vendor, device);
+ switch (override_only) {
+ case 0:
+ strcpy(alias, "cdx:");
+ break;
+ case CDX_ID_F_VFIO_DRIVER_OVERRIDE:
+ strcpy(alias, "vfio_cdx:");
+ break;
+ default:
+ warn("Unknown CDX driver_override alias %08X\n",
+ override_only);
+ return 0;
+ }
+
+ ADD(alias, "v", vendor != CDX_ANY_ID, vendor);
+ ADD(alias, "d", device != CDX_ANY_ID, device);
return 1;
}
--
2.17.1
On Fri, 14 Apr 2023 18:04:14 +0530
Nipun Gupta <[email protected]> wrote:
> vfio-cdx driver enables IOCTLs for user space to query
> MMIO regions for CDX devices and mmap them. This change
> also adds support for reset of CDX devices.
>
> This change adds the VFIO CDX driver and enables the following
> ioctls for CDX devices:
> - VFIO_DEVICE_GET_INFO:
> - VFIO_DEVICE_GET_REGION_INFO
> - VFIO_DEVICE_RESET
>
> Signed-off-by: Nipun Gupta <[email protected]>
> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> Tested-by: Nikhil Agarwal <[email protected]>
> ---
>
> Changes v1->v2:
> - Updated file2alias to support vfio_cdx
> - removed some un-necessary checks in mmap
> - removed vfio reset wrapper API
> - converted complex macros to static APIs
> - used pgprot_device and io_remap_pfn_range
>
> MAINTAINERS | 7 +
> drivers/vfio/Kconfig | 1 +
> drivers/vfio/Makefile | 1 +
> drivers/vfio/cdx/Kconfig | 17 ++
> drivers/vfio/cdx/Makefile | 8 +
> drivers/vfio/cdx/vfio_cdx.c | 290 ++++++++++++++++++++++++++++
> drivers/vfio/cdx/vfio_cdx_private.h | 31 +++
> include/linux/cdx/cdx_bus.h | 1 -
> include/linux/mod_devicetable.h | 6 +
> scripts/mod/devicetable-offsets.c | 1 +
> scripts/mod/file2alias.c | 17 +-
> 11 files changed, 378 insertions(+), 2 deletions(-)
> create mode 100644 drivers/vfio/cdx/Kconfig
> create mode 100644 drivers/vfio/cdx/Makefile
> create mode 100644 drivers/vfio/cdx/vfio_cdx.c
> create mode 100644 drivers/vfio/cdx/vfio_cdx_private.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7f74d8571ac9..c4fd42ba8f46 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22064,6 +22064,13 @@ F: Documentation/filesystems/vfat.rst
> F: fs/fat/
> F: tools/testing/selftests/filesystems/fat/
>
> +VFIO CDX DRIVER
> +M: Nipun Gupta <[email protected]>
> +M: Nikhil Agarwal <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/vfio/cdx/*
> +
> VFIO DRIVER
> M: Alex Williamson <[email protected]>
> L: [email protected]
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 89e06c981e43..aba36f5be4ec 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -57,6 +57,7 @@ source "drivers/vfio/pci/Kconfig"
> source "drivers/vfio/platform/Kconfig"
> source "drivers/vfio/mdev/Kconfig"
> source "drivers/vfio/fsl-mc/Kconfig"
> +source "drivers/vfio/cdx/Kconfig"
> endif
>
> source "virt/lib/Kconfig"
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 70e7dcb302ef..1a27b2516612 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_VFIO_PCI) += pci/
> obj-$(CONFIG_VFIO_PLATFORM) += platform/
> obj-$(CONFIG_VFIO_MDEV) += mdev/
> obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> +obj-$(CONFIG_VFIO_CDX) += cdx/
> diff --git a/drivers/vfio/cdx/Kconfig b/drivers/vfio/cdx/Kconfig
> new file mode 100644
> index 000000000000..e6de0a0caa32
> --- /dev/null
> +++ b/drivers/vfio/cdx/Kconfig
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# VFIO CDX configuration
> +#
> +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> +#
> +
> +config VFIO_CDX
> + tristate "VFIO support for CDX bus devices"
> + depends on CDX_BUS
> + select EVENTFD
> + help
> + Driver to enable VFIO support for the devices on CDX bus.
> + This is required to make use of CDX devices present in
> + the system using the VFIO framework.
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/vfio/cdx/Makefile b/drivers/vfio/cdx/Makefile
> new file mode 100644
> index 000000000000..82e4ef412c0f
> --- /dev/null
> +++ b/drivers/vfio/cdx/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> +#
> +
> +obj-$(CONFIG_VFIO_CDX) += vfio-cdx.o
> +
> +vfio-cdx-objs := vfio_cdx.o
> diff --git a/drivers/vfio/cdx/vfio_cdx.c b/drivers/vfio/cdx/vfio_cdx.c
> new file mode 100644
> index 000000000000..c19062ce7680
> --- /dev/null
> +++ b/drivers/vfio/cdx/vfio_cdx.c
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/vfio.h>
> +#include <linux/cdx/cdx_bus.h>
> +
> +#include "vfio_cdx_private.h"
> +
> +static struct cdx_driver vfio_cdx_driver;
> +
> +static int vfio_cdx_init_device(struct vfio_device *core_vdev)
> +{
> + struct vfio_cdx_device *vdev =
> + container_of(core_vdev, struct vfio_cdx_device, vdev);
> + struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> +
> + vdev->cdx_dev = cdx_dev;
> + vdev->dev = &cdx_dev->dev;
Both of these seem trivial to factor out of this patch, cdx_device is
always available via to_cdx_device(core_vdev->dev) and the struct
device is always available via core_vdev->dev. vdev->dev isn't even
used anywhere yet. Both the init and release functions here could be
dropped afaict.
> +
> + return 0;
> +}
> +
> +static void vfio_cdx_release_device(struct vfio_device *core_vdev)
> +{
> + struct vfio_cdx_device *vdev =
> + container_of(core_vdev, struct vfio_cdx_device, vdev);
> +
> + vdev->cdx_dev = NULL;
> + vdev->dev = NULL;
> +}
> +
> +/**
> + * CDX_DRIVER_OVERRIDE_DEVICE_VFIO - macro used to describe a VFIO
> + * "driver_override" CDX device.
> + * @vend: the 16 bit CDX Vendor ID
> + * @dev: the 16 bit CDX Device ID
> + *
> + * This macro is used to create a struct cdx_device_id that matches a
> + * specific device. driver_override will be set to
> + * CDX_ID_F_VFIO_DRIVER_OVERRIDE.
> + */
> +#define CDX_DRIVER_OVERRIDE_DEVICE_VFIO(vend, dev) \
> + CDX_DEVICE_DRIVER_OVERRIDE(vend, dev, CDX_ID_F_VFIO_DRIVER_OVERRIDE)
> +
> +static int vfio_cdx_open_device(struct vfio_device *core_vdev)
> +{
> + struct vfio_cdx_device *vdev =
> + container_of(core_vdev, struct vfio_cdx_device, vdev);
> + struct cdx_device *cdx_dev = vdev->cdx_dev;
> + int count = cdx_dev->res_count;
> + int i;
> +
> + vdev->regions = kcalloc(count, sizeof(struct vfio_cdx_region),
> + GFP_KERNEL);
> + if (!vdev->regions)
> + return -ENOMEM;
> +
> + for (i = 0; i < count; i++) {
> + struct resource *res = &cdx_dev->res[i];
> +
> + vdev->regions[i].addr = res->start;
> + vdev->regions[i].size = resource_size(res);
> + vdev->regions[i].type = res->flags;
> + /*
> + * Only regions addressed with PAGE granularity may be
> + * MMAP'ed securely.
> + */
> + if (!(vdev->regions[i].addr & ~PAGE_MASK) &&
> + !(vdev->regions[i].size & ~PAGE_MASK))
> + vdev->regions[i].flags |=
> + VFIO_REGION_INFO_FLAG_MMAP;
> + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
> + if (!(cdx_dev->res[i].flags & IORESOURCE_READONLY))
> + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
> + }
> +
> + return 0;
> +}
> +
> +static void vfio_cdx_regions_cleanup(struct vfio_cdx_device *vdev)
> +{
> + kfree(vdev->regions);
> +}
> +
> +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 = cdx_dev_reset(&vdev->cdx_dev->dev);
> + if (WARN_ON(ret))
> + dev_warn(core_vdev->dev,
> + "VFIO_CDX: reset device has failed (%d)\n", ret);
WARN_ON() + dev_warn(), do we need both? Curious we're not even using
vdev->dev here ;)
> +}
> +
> +static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct vfio_cdx_device *vdev =
> + container_of(core_vdev, struct vfio_cdx_device, vdev);
> + struct cdx_device *cdx_dev = vdev->cdx_dev;
> + unsigned long minsz;
> +
> + switch (cmd) {
> + case VFIO_DEVICE_GET_INFO:
> + {
> + struct vfio_device_info info;
> +
> + minsz = offsetofend(struct vfio_device_info, num_irqs);
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + info.flags = VFIO_DEVICE_FLAGS_RESET;
There ought to be a flag here indicating the vfio device exposed is
vfio-cdx.
> +
> + info.num_regions = cdx_dev->res_count;
> + info.num_irqs = 0;
> +
> + return copy_to_user((void __user *)arg, &info, minsz) ?
> + -EFAULT : 0;
> + }
> + case VFIO_DEVICE_GET_REGION_INFO:
> + {
> + struct vfio_region_info info;
> +
> + minsz = offsetofend(struct vfio_region_info, offset);
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + if (info.index >= cdx_dev->res_count)
> + return -EINVAL;
> +
> + /* map offset to the physical address */
> + info.offset = vfio_cdx_index_to_offset(info.index);
> + info.size = vdev->regions[info.index].size;
> + info.flags = vdev->regions[info.index].flags;
> +
> + if (copy_to_user((void __user *)arg, &info, minsz))
> + return -EFAULT;
> + return 0;
> + }
> + case VFIO_DEVICE_RESET:
> + {
> + return cdx_dev_reset(&vdev->cdx_dev->dev);
> + }
> + default:
> + return -ENOTTY;
> + }
> +}
> +
> +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_device(vma->vm_page_prot);
> +
> + return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> + size, vma->vm_page_prot);
> +}
> +
> +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 (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;
> +
> + return vfio_cdx_mmap_mmio(vdev->regions[index], vma);
> +}
> +
> +static const struct vfio_device_ops vfio_cdx_ops = {
> + .name = "vfio-cdx",
> + .init = vfio_cdx_init_device,
> + .release = vfio_cdx_release_device,
> + .open_device = vfio_cdx_open_device,
> + .close_device = vfio_cdx_close_device,
> + .ioctl = vfio_cdx_ioctl,
> + .mmap = vfio_cdx_mmap,
Missing bind_iommufd, unbind_iommufd, and attach_ioas, which will be
necessary for iommufd support.
> +};
> +
> +static int vfio_cdx_probe(struct cdx_device *cdx_dev)
> +{
> + struct vfio_cdx_device *vdev = NULL;
> + struct device *dev = &cdx_dev->dev;
> + int ret;
> +
> + vdev = vfio_alloc_device(vfio_cdx_device, vdev, dev,
> + &vfio_cdx_ops);
> + if (IS_ERR(vdev))
> + return PTR_ERR(vdev);
> +
> + ret = vfio_register_group_dev(&vdev->vdev);
> + if (ret) {
> + dev_err(dev, "VFIO_CDX: Failed to add to vfio group\n");
> + goto out_uninit;
> + }
> +
> + dev_set_drvdata(dev, vdev);
> + return 0;
> +
> +out_uninit:
> + vfio_put_device(&vdev->vdev);
> + return ret;
> +}
> +
> +static int vfio_cdx_remove(struct cdx_device *cdx_dev)
> +{
> + struct device *dev = &cdx_dev->dev;
> + struct vfio_cdx_device *vdev;
> +
> + vdev = dev_get_drvdata(dev);
> + vfio_unregister_group_dev(&vdev->vdev);
> + vfio_put_device(&vdev->vdev);
> +
> + return 0;
> +}
> +
> +static const struct cdx_device_id vfio_cdx_table[] = {
> + { CDX_DRIVER_OVERRIDE_DEVICE_VFIO(CDX_ANY_ID, CDX_ANY_ID) }, /* match all by default */
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(cdx, vfio_cdx_table);
> +
> +static struct cdx_driver vfio_cdx_driver = {
> + .probe = vfio_cdx_probe,
> + .remove = vfio_cdx_remove,
> + .match_id_table = vfio_cdx_table,
> + .driver = {
> + .name = "vfio-cdx",
> + .owner = THIS_MODULE,
> + },
> + .driver_managed_dma = true,
> +};
> +
> +static int __init vfio_cdx_driver_init(void)
> +{
> + return cdx_driver_register(&vfio_cdx_driver);
> +}
> +
> +static void __exit vfio_cdx_driver_exit(void)
> +{
> + cdx_driver_unregister(&vfio_cdx_driver);
> +}
> +
> +module_init(vfio_cdx_driver_init);
> +module_exit(vfio_cdx_driver_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("VFIO for CDX devices - User Level meta-driver");
> diff --git a/drivers/vfio/cdx/vfio_cdx_private.h b/drivers/vfio/cdx/vfio_cdx_private.h
> new file mode 100644
> index 000000000000..5f143736eb7a
> --- /dev/null
> +++ b/drivers/vfio/cdx/vfio_cdx_private.h
> @@ -0,0 +1,31 @@
> +/* 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
> +
> +static inline u64 vfio_cdx_index_to_offset(u32 index)
> +{
> + return ((u64)(index) << VFIO_CDX_OFFSET_SHIFT);
> +}
> +
> +struct vfio_cdx_region {
> + u32 flags;
> + u32 type;
> + u64 addr;
> + resource_size_t size;
> + void __iomem *ioaddr;
I don't see that ioaddr is used in this patch. Future r/w support?
Thanks,
Alex
> +};
> +
> +struct vfio_cdx_device {
> + struct vfio_device vdev;
> + struct cdx_device *cdx_dev;
> + struct device *dev;
> + struct vfio_cdx_region *regions;
> +};
> +
> +#endif /* VFIO_CDX_PRIVATE_H */
> diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
> index 35ef41d8a61a..bead71b7bc73 100644
> --- a/include/linux/cdx/cdx_bus.h
> +++ b/include/linux/cdx/cdx_bus.h
> @@ -14,7 +14,6 @@
> #include <linux/mod_devicetable.h>
>
> #define MAX_CDX_DEV_RESOURCES 4
> -#define CDX_ANY_ID (0xFFFF)
> #define CDX_CONTROLLER_ID_SHIFT 4
> #define CDX_BUS_NUM_MASK 0xF
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index ccaaeda792c0..ccf017353bb6 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -912,6 +912,12 @@ struct ishtp_device_id {
> kernel_ulong_t driver_data;
> };
>
> +#define CDX_ANY_ID (0xFFFF)
> +
> +enum {
> + CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
> +};
> +
> /**
> * struct cdx_device_id - CDX device identifier
> * @vendor: Vendor ID
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index 62dc988df84d..abe65f8968dd 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -265,6 +265,7 @@ int main(void)
> DEVID(cdx_device_id);
> DEVID_FIELD(cdx_device_id, vendor);
> DEVID_FIELD(cdx_device_id, device);
> + DEVID_FIELD(cdx_device_id, override_only);
>
> return 0;
> }
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 28da34ba4359..38120f932b0d 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1458,8 +1458,23 @@ static int do_cdx_entry(const char *filename, void *symval,
> {
> DEF_FIELD(symval, cdx_device_id, vendor);
> DEF_FIELD(symval, cdx_device_id, device);
> + DEF_FIELD(symval, cdx_device_id, override_only);
>
> - sprintf(alias, "cdx:v%08Xd%08Xd", vendor, device);
> + switch (override_only) {
> + case 0:
> + strcpy(alias, "cdx:");
> + break;
> + case CDX_ID_F_VFIO_DRIVER_OVERRIDE:
> + strcpy(alias, "vfio_cdx:");
> + break;
> + default:
> + warn("Unknown CDX driver_override alias %08X\n",
> + override_only);
> + return 0;
> + }
> +
> + ADD(alias, "v", vendor != CDX_ANY_ID, vendor);
> + ADD(alias, "d", device != CDX_ANY_ID, device);
> return 1;
> }
>
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Saturday, April 15, 2023 3:06 AM
> To: Gupta, Nipun <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; git (AMD-Xilinx) <[email protected]>; Anand, Harpreet
> <[email protected]>; Jansen Van Vuuren, Pieter <pieter.jansen-van-
> [email protected]>; Agarwal, Nikhil <[email protected]>; Simek, Michal
> <[email protected]>
> Subject: Re: [PATCH v2] vfio/cdx: add support for CDX bus
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Fri, 14 Apr 2023 18:04:14 +0530
> Nipun Gupta <[email protected]> wrote:
>
> > vfio-cdx driver enables IOCTLs for user space to query
> > MMIO regions for CDX devices and mmap them. This change
> > also adds support for reset of CDX devices.
> >
> > This change adds the VFIO CDX driver and enables the following
> > ioctls for CDX devices:
> > - VFIO_DEVICE_GET_INFO:
> > - VFIO_DEVICE_GET_REGION_INFO
> > - VFIO_DEVICE_RESET
> >
> > Signed-off-by: Nipun Gupta <[email protected]>
> > Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> > Tested-by: Nikhil Agarwal <[email protected]>
> > ---
> >
> > Changes v1->v2:
> > - Updated file2alias to support vfio_cdx
> > - removed some un-necessary checks in mmap
> > - removed vfio reset wrapper API
> > - converted complex macros to static APIs
> > - used pgprot_device and io_remap_pfn_range
> >
> > MAINTAINERS | 7 +
> > drivers/vfio/Kconfig | 1 +
> > drivers/vfio/Makefile | 1 +
> > drivers/vfio/cdx/Kconfig | 17 ++
> > drivers/vfio/cdx/Makefile | 8 +
> > drivers/vfio/cdx/vfio_cdx.c | 290 ++++++++++++++++++++++++++++
> > drivers/vfio/cdx/vfio_cdx_private.h | 31 +++
> > include/linux/cdx/cdx_bus.h | 1 -
> > include/linux/mod_devicetable.h | 6 +
> > scripts/mod/devicetable-offsets.c | 1 +
> > scripts/mod/file2alias.c | 17 +-
> > 11 files changed, 378 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/vfio/cdx/Kconfig
> > create mode 100644 drivers/vfio/cdx/Makefile
> > create mode 100644 drivers/vfio/cdx/vfio_cdx.c
> > create mode 100644 drivers/vfio/cdx/vfio_cdx_private.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7f74d8571ac9..c4fd42ba8f46 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -22064,6 +22064,13 @@ F: Documentation/filesystems/vfat.rst
> > F: fs/fat/
> > F: tools/testing/selftests/filesystems/fat/
> >
> > +VFIO CDX DRIVER
> > +M: Nipun Gupta <[email protected]>
> > +M: Nikhil Agarwal <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/vfio/cdx/*
> > +
> > VFIO DRIVER
> > M: Alex Williamson <[email protected]>
> > L: [email protected]
> > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> > index 89e06c981e43..aba36f5be4ec 100644
> > --- a/drivers/vfio/Kconfig
> > +++ b/drivers/vfio/Kconfig
> > @@ -57,6 +57,7 @@ source "drivers/vfio/pci/Kconfig"
> > source "drivers/vfio/platform/Kconfig"
> > source "drivers/vfio/mdev/Kconfig"
> > source "drivers/vfio/fsl-mc/Kconfig"
> > +source "drivers/vfio/cdx/Kconfig"
> > endif
> >
> > source "virt/lib/Kconfig"
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > index 70e7dcb302ef..1a27b2516612 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -14,3 +14,4 @@ obj-$(CONFIG_VFIO_PCI) += pci/
> > obj-$(CONFIG_VFIO_PLATFORM) += platform/
> > obj-$(CONFIG_VFIO_MDEV) += mdev/
> > obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> > +obj-$(CONFIG_VFIO_CDX) += cdx/
> > diff --git a/drivers/vfio/cdx/Kconfig b/drivers/vfio/cdx/Kconfig
> > new file mode 100644
> > index 000000000000..e6de0a0caa32
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/Kconfig
> > @@ -0,0 +1,17 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# VFIO CDX configuration
> > +#
> > +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > +#
> > +
> > +config VFIO_CDX
> > + tristate "VFIO support for CDX bus devices"
> > + depends on CDX_BUS
> > + select EVENTFD
> > + help
> > + Driver to enable VFIO support for the devices on CDX bus.
> > + This is required to make use of CDX devices present in
> > + the system using the VFIO framework.
> > +
> > + If you don't know what to do here, say N.
> > diff --git a/drivers/vfio/cdx/Makefile b/drivers/vfio/cdx/Makefile
> > new file mode 100644
> > index 000000000000..82e4ef412c0f
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/Makefile
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > +#
> > +
> > +obj-$(CONFIG_VFIO_CDX) += vfio-cdx.o
> > +
> > +vfio-cdx-objs := vfio_cdx.o
> > diff --git a/drivers/vfio/cdx/vfio_cdx.c b/drivers/vfio/cdx/vfio_cdx.c
> > new file mode 100644
> > index 000000000000..c19062ce7680
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/vfio_cdx.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > + */
> > +
> > +#include <linux/vfio.h>
> > +#include <linux/cdx/cdx_bus.h>
> > +
> > +#include "vfio_cdx_private.h"
> > +
> > +static struct cdx_driver vfio_cdx_driver;
> > +
> > +static int vfio_cdx_init_device(struct vfio_device *core_vdev)
> > +{
> > + struct vfio_cdx_device *vdev =
> > + container_of(core_vdev, struct vfio_cdx_device, vdev);
> > + struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> > +
> > + vdev->cdx_dev = cdx_dev;
> > + vdev->dev = &cdx_dev->dev;
>
> Both of these seem trivial to factor out of this patch, cdx_device is
> always available via to_cdx_device(core_vdev->dev) and the struct
> device is always available via core_vdev->dev. vdev->dev isn't even
> used anywhere yet. Both the init and release functions here could be
> dropped afaict.
Agree, these functions can be removed. Will update, test, and send in next spin.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void vfio_cdx_release_device(struct vfio_device *core_vdev)
> > +{
> > + struct vfio_cdx_device *vdev =
> > + container_of(core_vdev, struct vfio_cdx_device, vdev);
> > +
> > + vdev->cdx_dev = NULL;
> > + vdev->dev = NULL;
> > +}
> > +
> > +/**
> > + * CDX_DRIVER_OVERRIDE_DEVICE_VFIO - macro used to describe a VFIO
> > + * "driver_override" CDX device.
> > + * @vend: the 16 bit CDX Vendor ID
> > + * @dev: the 16 bit CDX Device ID
> > + *
> > + * This macro is used to create a struct cdx_device_id that matches a
> > + * specific device. driver_override will be set to
> > + * CDX_ID_F_VFIO_DRIVER_OVERRIDE.
> > + */
> > +#define CDX_DRIVER_OVERRIDE_DEVICE_VFIO(vend, dev) \
> > + CDX_DEVICE_DRIVER_OVERRIDE(vend, dev,
> CDX_ID_F_VFIO_DRIVER_OVERRIDE)
> > +
> > +static int vfio_cdx_open_device(struct vfio_device *core_vdev)
> > +{
> > + struct vfio_cdx_device *vdev =
> > + container_of(core_vdev, struct vfio_cdx_device, vdev);
> > + struct cdx_device *cdx_dev = vdev->cdx_dev;
> > + int count = cdx_dev->res_count;
> > + int i;
> > +
> > + vdev->regions = kcalloc(count, sizeof(struct vfio_cdx_region),
> > + GFP_KERNEL);
> > + if (!vdev->regions)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < count; i++) {
> > + struct resource *res = &cdx_dev->res[i];
> > +
> > + vdev->regions[i].addr = res->start;
> > + vdev->regions[i].size = resource_size(res);
> > + vdev->regions[i].type = res->flags;
> > + /*
> > + * Only regions addressed with PAGE granularity may be
> > + * MMAP'ed securely.
> > + */
> > + if (!(vdev->regions[i].addr & ~PAGE_MASK) &&
> > + !(vdev->regions[i].size & ~PAGE_MASK))
> > + vdev->regions[i].flags |=
> > + VFIO_REGION_INFO_FLAG_MMAP;
> > + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
> > + if (!(cdx_dev->res[i].flags & IORESOURCE_READONLY))
> > + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void vfio_cdx_regions_cleanup(struct vfio_cdx_device *vdev)
> > +{
> > + kfree(vdev->regions);
> > +}
> > +
> > +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 = cdx_dev_reset(&vdev->cdx_dev->dev);
> > + if (WARN_ON(ret))
> > + dev_warn(core_vdev->dev,
> > + "VFIO_CDX: reset device has failed (%d)\n", ret);
>
> WARN_ON() + dev_warn(), do we need both? Curious we're not even using
> vdev->dev here ;)
WARN_ON will be removed and we will keep using core_vdev->dev as vdev->dev
will be removed :)
>
> > +}
> > +
> > +static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
> > + unsigned int cmd, unsigned long arg)
> > +{
> > + struct vfio_cdx_device *vdev =
> > + container_of(core_vdev, struct vfio_cdx_device, vdev);
> > + struct cdx_device *cdx_dev = vdev->cdx_dev;
> > + unsigned long minsz;
> > +
> > + switch (cmd) {
> > + case VFIO_DEVICE_GET_INFO:
> > + {
> > + struct vfio_device_info info;
> > +
> > + minsz = offsetofend(struct vfio_device_info, num_irqs);
> > +
> > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > + return -EFAULT;
> > +
> > + if (info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + info.flags = VFIO_DEVICE_FLAGS_RESET;
>
> There ought to be a flag here indicating the vfio device exposed is
> vfio-cdx.
Sure, will add.
>
> > +
> > + info.num_regions = cdx_dev->res_count;
> > + info.num_irqs = 0;
> > +
> > + return copy_to_user((void __user *)arg, &info, minsz) ?
> > + -EFAULT : 0;
> > + }
> > + case VFIO_DEVICE_GET_REGION_INFO:
> > + {
> > + struct vfio_region_info info;
> > +
> > + minsz = offsetofend(struct vfio_region_info, offset);
> > +
> > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > + return -EFAULT;
> > +
> > + if (info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + if (info.index >= cdx_dev->res_count)
> > + return -EINVAL;
> > +
> > + /* map offset to the physical address */
> > + info.offset = vfio_cdx_index_to_offset(info.index);
> > + info.size = vdev->regions[info.index].size;
> > + info.flags = vdev->regions[info.index].flags;
> > +
> > + if (copy_to_user((void __user *)arg, &info, minsz))
> > + return -EFAULT;
> > + return 0;
> > + }
> > + case VFIO_DEVICE_RESET:
> > + {
> > + return cdx_dev_reset(&vdev->cdx_dev->dev);
> > + }
> > + default:
> > + return -ENOTTY;
> > + }
> > +}
> > +
> > +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_device(vma->vm_page_prot);
> > +
> > + return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > + size, vma->vm_page_prot);
> > +}
> > +
> > +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 (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;
> > +
> > + return vfio_cdx_mmap_mmio(vdev->regions[index], vma);
> > +}
> > +
> > +static const struct vfio_device_ops vfio_cdx_ops = {
> > + .name = "vfio-cdx",
> > + .init = vfio_cdx_init_device,
> > + .release = vfio_cdx_release_device,
> > + .open_device = vfio_cdx_open_device,
> > + .close_device = vfio_cdx_close_device,
> > + .ioctl = vfio_cdx_ioctl,
> > + .mmap = vfio_cdx_mmap,
>
> Missing bind_iommufd, unbind_iommufd, and attach_ioas, which will be
> necessary for iommufd support.
Sure, will add these too.
>
> > +};
> > +
> > +static int vfio_cdx_probe(struct cdx_device *cdx_dev)
> > +{
> > + struct vfio_cdx_device *vdev = NULL;
> > + struct device *dev = &cdx_dev->dev;
> > + int ret;
> > +
> > + vdev = vfio_alloc_device(vfio_cdx_device, vdev, dev,
> > + &vfio_cdx_ops);
> > + if (IS_ERR(vdev))
> > + return PTR_ERR(vdev);
> > +
> > + ret = vfio_register_group_dev(&vdev->vdev);
> > + if (ret) {
> > + dev_err(dev, "VFIO_CDX: Failed to add to vfio group\n");
> > + goto out_uninit;
> > + }
> > +
> > + dev_set_drvdata(dev, vdev);
> > + return 0;
> > +
> > +out_uninit:
> > + vfio_put_device(&vdev->vdev);
> > + return ret;
> > +}
> > +
> > +static int vfio_cdx_remove(struct cdx_device *cdx_dev)
> > +{
> > + struct device *dev = &cdx_dev->dev;
> > + struct vfio_cdx_device *vdev;
> > +
> > + vdev = dev_get_drvdata(dev);
> > + vfio_unregister_group_dev(&vdev->vdev);
> > + vfio_put_device(&vdev->vdev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct cdx_device_id vfio_cdx_table[] = {
> > + { CDX_DRIVER_OVERRIDE_DEVICE_VFIO(CDX_ANY_ID, CDX_ANY_ID) }, /*
> match all by default */
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(cdx, vfio_cdx_table);
> > +
> > +static struct cdx_driver vfio_cdx_driver = {
> > + .probe = vfio_cdx_probe,
> > + .remove = vfio_cdx_remove,
> > + .match_id_table = vfio_cdx_table,
> > + .driver = {
> > + .name = "vfio-cdx",
> > + .owner = THIS_MODULE,
> > + },
> > + .driver_managed_dma = true,
> > +};
> > +
> > +static int __init vfio_cdx_driver_init(void)
> > +{
> > + return cdx_driver_register(&vfio_cdx_driver);
> > +}
> > +
> > +static void __exit vfio_cdx_driver_exit(void)
> > +{
> > + cdx_driver_unregister(&vfio_cdx_driver);
> > +}
> > +
> > +module_init(vfio_cdx_driver_init);
> > +module_exit(vfio_cdx_driver_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("VFIO for CDX devices - User Level meta-driver");
> > diff --git a/drivers/vfio/cdx/vfio_cdx_private.h
> b/drivers/vfio/cdx/vfio_cdx_private.h
> > new file mode 100644
> > index 000000000000..5f143736eb7a
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/vfio_cdx_private.h
> > @@ -0,0 +1,31 @@
> > +/* 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
> > +
> > +static inline u64 vfio_cdx_index_to_offset(u32 index)
> > +{
> > + return ((u64)(index) << VFIO_CDX_OFFSET_SHIFT);
> > +}
> > +
> > +struct vfio_cdx_region {
> > + u32 flags;
> > + u32 type;
> > + u64 addr;
> > + resource_size_t size;
> > + void __iomem *ioaddr;
>
> I don't see that ioaddr is used in this patch. Future r/w support?
Agree, will remove this from this patch.
Thanks,
Nipun
> Thanks,
>
> Alex
>
> > +};
> > +
> > +struct vfio_cdx_device {
> > + struct vfio_device vdev;
> > + struct cdx_device *cdx_dev;
> > + struct device *dev;
> > + struct vfio_cdx_region *regions;
> > +};
> > +
> > +#endif /* VFIO_CDX_PRIVATE_H */
> > diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
> > index 35ef41d8a61a..bead71b7bc73 100644
> > --- a/include/linux/cdx/cdx_bus.h
> > +++ b/include/linux/cdx/cdx_bus.h
> > @@ -14,7 +14,6 @@
> > #include <linux/mod_devicetable.h>
> >
> > #define MAX_CDX_DEV_RESOURCES 4
> > -#define CDX_ANY_ID (0xFFFF)
> > #define CDX_CONTROLLER_ID_SHIFT 4
> > #define CDX_BUS_NUM_MASK 0xF
> >
> > diff --git a/include/linux/mod_devicetable.h
> b/include/linux/mod_devicetable.h
> > index ccaaeda792c0..ccf017353bb6 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -912,6 +912,12 @@ struct ishtp_device_id {
> > kernel_ulong_t driver_data;
> > };
> >
> > +#define CDX_ANY_ID (0xFFFF)
> > +
> > +enum {
> > + CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
> > +};
> > +
> > /**
> > * struct cdx_device_id - CDX device identifier
> > * @vendor: Vendor ID
> > diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-
> offsets.c
> > index 62dc988df84d..abe65f8968dd 100644
> > --- a/scripts/mod/devicetable-offsets.c
> > +++ b/scripts/mod/devicetable-offsets.c
> > @@ -265,6 +265,7 @@ int main(void)
> > DEVID(cdx_device_id);
> > DEVID_FIELD(cdx_device_id, vendor);
> > DEVID_FIELD(cdx_device_id, device);
> > + DEVID_FIELD(cdx_device_id, override_only);
> >
> > return 0;
> > }
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index 28da34ba4359..38120f932b0d 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1458,8 +1458,23 @@ static int do_cdx_entry(const char *filename, void
> *symval,
> > {
> > DEF_FIELD(symval, cdx_device_id, vendor);
> > DEF_FIELD(symval, cdx_device_id, device);
> > + DEF_FIELD(symval, cdx_device_id, override_only);
> >
> > - sprintf(alias, "cdx:v%08Xd%08Xd", vendor, device);
> > + switch (override_only) {
> > + case 0:
> > + strcpy(alias, "cdx:");
> > + break;
> > + case CDX_ID_F_VFIO_DRIVER_OVERRIDE:
> > + strcpy(alias, "vfio_cdx:");
> > + break;
> > + default:
> > + warn("Unknown CDX driver_override alias %08X\n",
> > + override_only);
> > + return 0;
> > + }
> > +
> > + ADD(alias, "v", vendor != CDX_ANY_ID, vendor);
> > + ADD(alias, "d", device != CDX_ANY_ID, device);
> > return 1;
> > }
> >
On Fri, Apr 14, 2023 at 03:36:14PM -0600, Alex Williamson wrote:
> > +static int vfio_cdx_init_device(struct vfio_device *core_vdev)
> > +{
> > + struct vfio_cdx_device *vdev =
> > + container_of(core_vdev, struct vfio_cdx_device, vdev);
> > + struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> > +
> > + vdev->cdx_dev = cdx_dev;
> > + vdev->dev = &cdx_dev->dev;
>
> Both of these seem trivial to factor out of this patch, cdx_device is
> always available via to_cdx_device(core_vdev->dev) and the struct
> device is always available via core_vdev->dev. vdev->dev isn't even
> used anywhere yet. Both the init and release functions here could be
> dropped afaict.
Yes please, I have a series someplace that gets rid of all these
redundent ->devs we keep around everwhere.
to_cdx_device(core_vdev->dev) is a good solution, maybe with a static
inline.
Jason
[AMD Official Use Only - General]
> -----Original Message-----
> From: Jason Gunthorpe <[email protected]>
> Sent: Monday, April 17, 2023 6:49 PM
> To: Alex Williamson <[email protected]>
> Cc: Gupta, Nipun <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; git (AMD-Xilinx) <[email protected]>;
> Anand, Harpreet <[email protected]>; Jansen Van Vuuren, Pieter
> <[email protected]>; Agarwal, Nikhil
> <[email protected]>; Simek, Michal <[email protected]>
> Subject: Re: [PATCH v2] vfio/cdx: add support for CDX bus
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Fri, Apr 14, 2023 at 03:36:14PM -0600, Alex Williamson wrote:
>
> > > +static int vfio_cdx_init_device(struct vfio_device *core_vdev)
> > > +{
> > > + struct vfio_cdx_device *vdev =
> > > + container_of(core_vdev, struct vfio_cdx_device, vdev);
> > > + struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> > > +
> > > + vdev->cdx_dev = cdx_dev;
> > > + vdev->dev = &cdx_dev->dev;
> >
> > Both of these seem trivial to factor out of this patch, cdx_device is
> > always available via to_cdx_device(core_vdev->dev) and the struct
> > device is always available via core_vdev->dev. vdev->dev isn't even
> > used anywhere yet. Both the init and release functions here could be
> > dropped afaict.
>
> Yes please, I have a series someplace that gets rid of all these
> redundent ->devs we keep around everwhere.
>
> to_cdx_device(core_vdev->dev) is a good solution, maybe with a static
> inline.
Agree, we have already rolled out v3 which has this change i.e using
to_cdx_dev(core_vdev->dev) instead of storing these again.
Thanks,
Nipun
>
> Jason