From: Ankit Agrawal <[email protected]>
NVIDIA's upcoming Grace Hopper Superchip provides a PCI-like device
for the on-chip GPU that is the logical OS representation of the
internal proprietary cache coherent interconnect.
This representation has a number of limitations compared to a real PCI
device, in particular, it does not model the coherent GPU memory
aperture as a PCI config space BAR, and PCI doesn't know anything
about cacheable memory types.
Provide a VFIO PCI variant driver that adapts the unique PCI
representation into a more standard PCI representation facing
userspace. The GPU memory aperture is obtained from ACPI using
device_property_read_u64(), according to the FW specification,
and exported to userspace as a separate VFIO_REGION. Since the device
implements only one 64-bit BAR (BAR0), the GPU memory aperture is mapped
to the next available PCI BAR (BAR2). Qemu will then naturally generate a
PCI device in the VM with two 64-bit BARs (where the cacheable aperture
reported in BAR2).
Since this memory region is actually cache coherent with the CPU, the
VFIO variant driver will mmap it into VMA using a cacheable mapping. The
mapping is done using remap_pfn_range().
PCI BAR are aligned to the power-of-2, but the actual memory on the
device may not. The physical address from the last device PFN up to the
next power-of-2 aligned PA thus is handled by the vfio-pci read/write
device ops which returns an error.
This goes along with a qemu series to provides the necessary
implementation of the Grace Hopper Superchip firmware specification so
that the guest operating system can see the correct ACPI modeling for
the coherent GPU device.
https://www.mail-archive.com/[email protected]/msg967557.html
This patch is split from a patch series being pursued separately:
https://lore.kernel.org/lkml/[email protected]/
Applied and tested over v6.4-rc6.
Signed-off-by: Ankit Agrawal <[email protected]>
---
v3 -> v4
- Mapping the available device memory using sparse mmap. The region outside
the device memory is handled by read/write ops.
- Removed the fault handler added in v3.
v2 -> v3
- Added fault handler to map the region outside the physical GPU memory
up to the next power-of-2 to a dummy PFN.
- Changed to select instead of "depends on" VFIO_PCI_CORE for all the
vfio-pci variant driver.
- Code cleanup based on feedback comments.
- Code implemented and tested against v6.4-rc4.
v1 -> v2
- Updated the wording of reference to BAR offset and replaced with
index.
- The GPU memory is exposed at the fixed BAR2_REGION_INDEX.
- Code cleanup based on feedback comments.
MAINTAINERS | 6 +
drivers/vfio/pci/Kconfig | 2 +
drivers/vfio/pci/Makefile | 2 +
drivers/vfio/pci/hisilicon/Kconfig | 2 +-
drivers/vfio/pci/mlx5/Kconfig | 2 +-
drivers/vfio/pci/nvgpu/Kconfig | 10 +
drivers/vfio/pci/nvgpu/Makefile | 3 +
drivers/vfio/pci/nvgpu/main.c | 311 +++++++++++++++++++++++++++++
8 files changed, 336 insertions(+), 2 deletions(-)
create mode 100644 drivers/vfio/pci/nvgpu/Kconfig
create mode 100644 drivers/vfio/pci/nvgpu/Makefile
create mode 100644 drivers/vfio/pci/nvgpu/main.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f794002a192e..f3b3115fdfed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22169,6 +22169,12 @@ L: [email protected]
S: Maintained
F: drivers/vfio/platform/
+VFIO NVIDIA PCI DRIVER
+M: Ankit Agrawal <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/vfio/pci/nvgpu/
+
VGA_SWITCHEROO
R: Lukas Wunner <[email protected]>
S: Maintained
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index f9d0c908e738..ade18b0ffb7b 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -59,4 +59,6 @@ source "drivers/vfio/pci/mlx5/Kconfig"
source "drivers/vfio/pci/hisilicon/Kconfig"
+source "drivers/vfio/pci/nvgpu/Kconfig"
+
endif
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 24c524224da5..0c93d452d0da 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5/
obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/
+
+obj-$(CONFIG_NVGPU_VFIO_PCI) += nvgpu/
diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
index 5daa0f45d2f9..38e90e05d68a 100644
--- a/drivers/vfio/pci/hisilicon/Kconfig
+++ b/drivers/vfio/pci/hisilicon/Kconfig
@@ -2,12 +2,12 @@
config HISI_ACC_VFIO_PCI
tristate "VFIO PCI support for HiSilicon ACC devices"
depends on ARM64 || (COMPILE_TEST && 64BIT)
- depends on VFIO_PCI_CORE
depends on PCI_MSI
depends on CRYPTO_DEV_HISI_QM
depends on CRYPTO_DEV_HISI_HPRE
depends on CRYPTO_DEV_HISI_SEC2
depends on CRYPTO_DEV_HISI_ZIP
+ select VFIO_PCI_CORE
help
This provides generic PCI support for HiSilicon ACC devices
using the VFIO framework.
diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
index 29ba9c504a75..7088edc4fb28 100644
--- a/drivers/vfio/pci/mlx5/Kconfig
+++ b/drivers/vfio/pci/mlx5/Kconfig
@@ -2,7 +2,7 @@
config MLX5_VFIO_PCI
tristate "VFIO support for MLX5 PCI devices"
depends on MLX5_CORE
- depends on VFIO_PCI_CORE
+ select VFIO_PCI_CORE
help
This provides migration support for MLX5 devices using the VFIO
framework.
diff --git a/drivers/vfio/pci/nvgpu/Kconfig b/drivers/vfio/pci/nvgpu/Kconfig
new file mode 100644
index 000000000000..066f764f7c5f
--- /dev/null
+++ b/drivers/vfio/pci/nvgpu/Kconfig
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config NVGPU_VFIO_PCI
+ tristate "VFIO support for the GPU in the NVIDIA Grace Hopper Superchip"
+ depends on ARM64 || (COMPILE_TEST && 64BIT)
+ select VFIO_PCI_CORE
+ help
+ VFIO support for the GPU in the NVIDIA Grace Hopper Superchip is
+ required to assign the GPU device to a VM using KVM/qemu/etc.
+
+ If you don't know what to do here, say N.
diff --git a/drivers/vfio/pci/nvgpu/Makefile b/drivers/vfio/pci/nvgpu/Makefile
new file mode 100644
index 000000000000..00fd3a078218
--- /dev/null
+++ b/drivers/vfio/pci/nvgpu/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_NVGPU_VFIO_PCI) += nvgpu-vfio-pci.o
+nvgpu-vfio-pci-y := main.o
diff --git a/drivers/vfio/pci/nvgpu/main.c b/drivers/vfio/pci/nvgpu/main.c
new file mode 100644
index 000000000000..ff68d0c5f865
--- /dev/null
+++ b/drivers/vfio/pci/nvgpu/main.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved
+ */
+
+#include <linux/pci.h>
+#include <linux/vfio_pci_core.h>
+#include <linux/vfio.h>
+
+struct dev_mem_properties {
+ uint64_t hpa;
+ uint64_t mem_length;
+};
+
+struct nvgpu_vfio_pci_core_device {
+ struct vfio_pci_core_device core_device;
+ struct dev_mem_properties mem_prop;
+};
+
+static int nvgpu_vfio_pci_open_device(struct vfio_device *core_vdev)
+{
+ struct vfio_pci_core_device *vdev =
+ container_of(core_vdev, struct vfio_pci_core_device, vdev);
+ int ret;
+
+ ret = vfio_pci_core_enable(vdev);
+ if (ret)
+ return ret;
+
+ vfio_pci_core_finish_enable(vdev);
+
+ return 0;
+}
+
+static int nvgpu_vfio_pci_mmap(struct vfio_device *core_vdev,
+ struct vm_area_struct *vma)
+{
+ struct nvgpu_vfio_pci_core_device *nvdev = container_of(
+ core_vdev, struct nvgpu_vfio_pci_core_device, core_device.vdev);
+
+ unsigned long start_pfn;
+ unsigned int index;
+ u64 req_len, pgoff;
+ int ret = 0;
+
+ index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
+ if (index != VFIO_PCI_BAR2_REGION_INDEX)
+ return vfio_pci_core_mmap(core_vdev, vma);
+
+ /*
+ * Request to mmap the BAR. Map to the CPU accessible memory on the
+ * GPU using the memory information gathered from the system ACPI
+ * tables.
+ */
+ start_pfn = nvdev->mem_prop.hpa >> PAGE_SHIFT;
+ req_len = vma->vm_end - vma->vm_start;
+ pgoff = vma->vm_pgoff &
+ ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
+ if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
+ return -EINVAL;
+
+ /*
+ * Perform a PFN map to the memory. The device BAR is backed by the
+ * GPU memory now. Check that the mapping does not overflow out of
+ * the GPU memory size.
+ *
+ * The available GPU memory size may not be power-of-2 aligned. Given
+ * that the memory is exposed as a BAR, the mapping request is of the
+ * power-of-2 aligned size. Map only up to the size of the GPU memory.
+ * If the memory access is beyond the actual GPU memory size, it will
+ * be handled by the vfio_device_ops read/write.
+ *
+ * During device reset, the GPU is safely disconnected to the CPU
+ * and access to the BAR will be immediately returned preventing
+ * machine check.
+ */
+ ret = remap_pfn_range(vma, vma->vm_start, start_pfn + pgoff,
+ min(req_len, nvdev->mem_prop.mem_length - pgoff),
+ vma->vm_page_prot);
+ if (ret)
+ return ret;
+
+ vma->vm_pgoff = start_pfn + pgoff;
+
+ return 0;
+}
+
+static long nvgpu_vfio_pci_ioctl(struct vfio_device *core_vdev,
+ unsigned int cmd, unsigned long arg)
+{
+ struct nvgpu_vfio_pci_core_device *nvdev = container_of(
+ core_vdev, struct nvgpu_vfio_pci_core_device, core_device.vdev);
+
+ unsigned long minsz = offsetofend(struct vfio_region_info, offset);
+ struct vfio_region_info info;
+
+ if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ if (info.index == VFIO_PCI_BAR2_REGION_INDEX) {
+ /*
+ * Request to determine the BAR region information. Send the
+ * GPU memory information.
+ */
+ uint32_t size;
+ struct vfio_region_info_cap_sparse_mmap *sparse;
+ struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+
+ size = sizeof(struct vfio_region_info_cap_sparse_mmap) +
+ (sizeof(struct vfio_region_sparse_mmap_area));
+
+ /*
+ * Setup for sparse mapping for the device memory. Only the
+ * available device memory on the hardware is shown as a
+ * mappable region.
+ */
+ sparse = kmalloc(size, GFP_KERNEL);
+ if (sparse == NULL)
+ return -ENOMEM;
+
+ sparse->nr_areas = 1;
+ sparse->areas[0].offset = 0;
+ sparse->areas[0].size = nvdev->mem_prop.mem_length;
+ sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
+ sparse->header.version = 1;
+
+ vfio_info_add_capability(&caps, &sparse->header, size);
+
+ info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
+ /*
+ * The available GPU memory size may not be power-of-2 aligned.
+ * Given that the memory is exposed as a BAR and may not be
+ * aligned, roundup to the next power-of-2.
+ */
+ info.size = is_power_of_2(nvdev->mem_prop.mem_length) ?
+ nvdev->mem_prop.mem_length :
+ roundup_pow_of_two(nvdev->mem_prop.mem_length);
+ info.flags = VFIO_REGION_INFO_FLAG_READ |
+ VFIO_REGION_INFO_FLAG_WRITE |
+ VFIO_REGION_INFO_FLAG_MMAP;
+
+ if (caps.size) {
+ info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
+ if (info.argsz < sizeof(info) + caps.size) {
+ info.argsz = sizeof(info) + caps.size;
+ info.cap_offset = 0;
+ } else {
+ vfio_info_cap_shift(&caps, sizeof(info));
+ if (copy_to_user((void __user *)arg +
+ sizeof(info), caps.buf,
+ caps.size)) {
+ kfree(caps.buf);
+ return -EFAULT;
+ }
+ info.cap_offset = sizeof(info);
+ }
+ kfree(caps.buf);
+ }
+
+ return copy_to_user((void __user *)arg, &info, minsz) ?
+ -EFAULT : 0;
+ }
+ }
+
+ return vfio_pci_core_ioctl(core_vdev, cmd, arg);
+}
+
+static ssize_t nvgpu_vfio_pci_read(struct vfio_device *core_vdev,
+ char __user *buf, size_t count, loff_t *ppos)
+{
+ unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
+
+ /*
+ * Only the device memory present on the hardware is mapped, which may
+ * not be power-of-2 aligned. A read to the BAR2 region implies an
+ * access outside the available device memory on the hardware.
+ */
+ if (index == VFIO_PCI_BAR2_REGION_INDEX)
+ return -EINVAL;
+
+ return vfio_pci_core_read(core_vdev, buf, count, ppos);
+
+}
+
+static ssize_t nvgpu_vfio_pci_write(struct vfio_device *core_vdev,
+ const char __user *buf, size_t count, loff_t *ppos)
+{
+ unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
+
+ /*
+ * Only the device memory present on the hardware is mapped, which may
+ * not be power-of-2 aligned. A write to the BAR2 region implies an
+ * access outside the available device memory on the hardware.
+ */
+ if (index == VFIO_PCI_BAR2_REGION_INDEX)
+ return -EINVAL;
+
+ return vfio_pci_core_write(core_vdev, buf, count, ppos);
+}
+
+static const struct vfio_device_ops nvgpu_vfio_pci_ops = {
+ .name = "nvgpu-vfio-pci",
+ .init = vfio_pci_core_init_dev,
+ .release = vfio_pci_core_release_dev,
+ .open_device = nvgpu_vfio_pci_open_device,
+ .close_device = vfio_pci_core_close_device,
+ .ioctl = nvgpu_vfio_pci_ioctl,
+ .read = nvgpu_vfio_pci_read,
+ .write = nvgpu_vfio_pci_write,
+ .mmap = nvgpu_vfio_pci_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,
+};
+
+static struct nvgpu_vfio_pci_core_device *nvgpu_drvdata(struct pci_dev *pdev)
+{
+ struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
+
+ return container_of(core_device, struct nvgpu_vfio_pci_core_device,
+ core_device);
+}
+
+static int
+nvgpu_vfio_pci_fetch_memory_property(struct pci_dev *pdev,
+ struct nvgpu_vfio_pci_core_device *nvdev)
+{
+ int ret;
+
+ /*
+ * The memory information is present in the system ACPI tables as DSD
+ * properties nvidia,gpu-mem-base-pa and nvidia,gpu-mem-size.
+ */
+ ret = device_property_read_u64(&(pdev->dev), "nvidia,gpu-mem-base-pa",
+ &(nvdev->mem_prop.hpa));
+ if (ret)
+ return ret;
+
+ ret = device_property_read_u64(&(pdev->dev), "nvidia,gpu-mem-size",
+ &(nvdev->mem_prop.mem_length));
+ return ret;
+}
+
+static int nvgpu_vfio_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ struct nvgpu_vfio_pci_core_device *nvdev;
+ int ret;
+
+ nvdev = vfio_alloc_device(nvgpu_vfio_pci_core_device, core_device.vdev,
+ &pdev->dev, &nvgpu_vfio_pci_ops);
+ if (IS_ERR(nvdev))
+ return PTR_ERR(nvdev);
+
+ dev_set_drvdata(&pdev->dev, nvdev);
+
+ ret = nvgpu_vfio_pci_fetch_memory_property(pdev, nvdev);
+ if (ret)
+ goto out_put_vdev;
+
+ ret = vfio_pci_core_register_device(&nvdev->core_device);
+ if (ret)
+ goto out_put_vdev;
+
+ return ret;
+
+out_put_vdev:
+ vfio_put_device(&nvdev->core_device.vdev);
+ return ret;
+}
+
+static void nvgpu_vfio_pci_remove(struct pci_dev *pdev)
+{
+ struct nvgpu_vfio_pci_core_device *nvdev = nvgpu_drvdata(pdev);
+ struct vfio_pci_core_device *vdev = &nvdev->core_device;
+
+ vfio_pci_core_unregister_device(vdev);
+ vfio_put_device(&vdev->vdev);
+}
+
+static const struct pci_device_id nvgpu_vfio_pci_table[] = {
+ { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2342) },
+ { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2343) },
+ { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2345) },
+ {}
+};
+
+MODULE_DEVICE_TABLE(pci, nvgpu_vfio_pci_table);
+
+static struct pci_driver nvgpu_vfio_pci_driver = {
+ .name = KBUILD_MODNAME,
+ .id_table = nvgpu_vfio_pci_table,
+ .probe = nvgpu_vfio_pci_probe,
+ .remove = nvgpu_vfio_pci_remove,
+ .err_handler = &vfio_pci_core_err_handlers,
+ .driver_managed_dma = true,
+};
+
+module_pci_driver(nvgpu_vfio_pci_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Ankit Agrawal <[email protected]>");
+MODULE_AUTHOR("Aniket Agashe <[email protected]>");
+MODULE_DESCRIPTION(
+ "VFIO NVGPU PF - User Level driver for NVIDIA devices with CPU coherently accessible device memory");
--
2.17.1
On 6/22/23 05:07, [email protected] wrote:
> From: Ankit Agrawal <[email protected]>
>
> NVIDIA's upcoming Grace Hopper Superchip provides a PCI-like device
> for the on-chip GPU that is the logical OS representation of the
> internal proprietary cache coherent interconnect.
>
> This representation has a number of limitations compared to a real PCI
> device, in particular, it does not model the coherent GPU memory
> aperture as a PCI config space BAR, and PCI doesn't know anything
> about cacheable memory types.
>
> Provide a VFIO PCI variant driver that adapts the unique PCI
> representation into a more standard PCI representation facing
> userspace. The GPU memory aperture is obtained from ACPI using
> device_property_read_u64(), according to the FW specification,
> and exported to userspace as a separate VFIO_REGION. Since the device
> implements only one 64-bit BAR (BAR0), the GPU memory aperture is mapped
> to the next available PCI BAR (BAR2). Qemu will then naturally generate a
> PCI device in the VM with two 64-bit BARs (where the cacheable aperture
> reported in BAR2).
>
> Since this memory region is actually cache coherent with the CPU, the
> VFIO variant driver will mmap it into VMA using a cacheable mapping. The
> mapping is done using remap_pfn_range().
>
> PCI BAR are aligned to the power-of-2, but the actual memory on the
> device may not. The physical address from the last device PFN up to the
> next power-of-2 aligned PA thus is handled by the vfio-pci read/write
> device ops which returns an error.
>
> This goes along with a qemu series to provides the necessary
> implementation of the Grace Hopper Superchip firmware specification so
> that the guest operating system can see the correct ACPI modeling for
> the coherent GPU device.
> https://www.mail-archive.com/[email protected]/msg967557.html
>
> This patch is split from a patch series being pursued separately:
> https://lore.kernel.org/lkml/[email protected]/
>
> Applied and tested over v6.4-rc6.
>
> Signed-off-by: Ankit Agrawal <[email protected]>
> ---
> v3 -> v4
> - Mapping the available device memory using sparse mmap. The region outside
> the device memory is handled by read/write ops.
> - Removed the fault handler added in v3.
>
> v2 -> v3
> - Added fault handler to map the region outside the physical GPU memory
> up to the next power-of-2 to a dummy PFN.
> - Changed to select instead of "depends on" VFIO_PCI_CORE for all the
> vfio-pci variant driver.
> - Code cleanup based on feedback comments.
> - Code implemented and tested against v6.4-rc4.
>
> v1 -> v2
> - Updated the wording of reference to BAR offset and replaced with
> index.
> - The GPU memory is exposed at the fixed BAR2_REGION_INDEX.
> - Code cleanup based on feedback comments.
>
> MAINTAINERS | 6 +
> drivers/vfio/pci/Kconfig | 2 +
> drivers/vfio/pci/Makefile | 2 +
> drivers/vfio/pci/hisilicon/Kconfig | 2 +-
> drivers/vfio/pci/mlx5/Kconfig | 2 +-
These changes are unexpected. May be you should rebase on :
https://github.com/awilliam/linux-vfio/commits/next
> drivers/vfio/pci/nvgpu/Kconfig | 10 +
> drivers/vfio/pci/nvgpu/Makefile | 3 +
> drivers/vfio/pci/nvgpu/main.c | 311 +++++++++++++++++++++++++++++
> 8 files changed, 336 insertions(+), 2 deletions(-)
> create mode 100644 drivers/vfio/pci/nvgpu/Kconfig
> create mode 100644 drivers/vfio/pci/nvgpu/Makefile
> create mode 100644 drivers/vfio/pci/nvgpu/main.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f794002a192e..f3b3115fdfed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22169,6 +22169,12 @@ L: [email protected]
> S: Maintained
> F: drivers/vfio/platform/
>
> +VFIO NVIDIA PCI DRIVER
> +M: Ankit Agrawal <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/vfio/pci/nvgpu/
> +
> VGA_SWITCHEROO
> R: Lukas Wunner <[email protected]>
> S: Maintained
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..ade18b0ffb7b 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -59,4 +59,6 @@ source "drivers/vfio/pci/mlx5/Kconfig"
>
> source "drivers/vfio/pci/hisilicon/Kconfig"
>
> +source "drivers/vfio/pci/nvgpu/Kconfig"
> +
> endif
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 24c524224da5..0c93d452d0da 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5/
>
> obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/
> +
> +obj-$(CONFIG_NVGPU_VFIO_PCI) += nvgpu/
> diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
> index 5daa0f45d2f9..38e90e05d68a 100644
> --- a/drivers/vfio/pci/hisilicon/Kconfig
> +++ b/drivers/vfio/pci/hisilicon/Kconfig
> @@ -2,12 +2,12 @@
> config HISI_ACC_VFIO_PCI
> tristate "VFIO PCI support for HiSilicon ACC devices"
> depends on ARM64 || (COMPILE_TEST && 64BIT)
> - depends on VFIO_PCI_CORE
> depends on PCI_MSI
> depends on CRYPTO_DEV_HISI_QM
> depends on CRYPTO_DEV_HISI_HPRE
> depends on CRYPTO_DEV_HISI_SEC2
> depends on CRYPTO_DEV_HISI_ZIP
> + select VFIO_PCI_CORE
> help
> This provides generic PCI support for HiSilicon ACC devices
> using the VFIO framework.
> diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
> index 29ba9c504a75..7088edc4fb28 100644
> --- a/drivers/vfio/pci/mlx5/Kconfig
> +++ b/drivers/vfio/pci/mlx5/Kconfig
> @@ -2,7 +2,7 @@
> config MLX5_VFIO_PCI
> tristate "VFIO support for MLX5 PCI devices"
> depends on MLX5_CORE
> - depends on VFIO_PCI_CORE
> + select VFIO_PCI_CORE
> help
> This provides migration support for MLX5 devices using the VFIO
> framework.
> diff --git a/drivers/vfio/pci/nvgpu/Kconfig b/drivers/vfio/pci/nvgpu/Kconfig
> new file mode 100644
> index 000000000000..066f764f7c5f
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgpu/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config NVGPU_VFIO_PCI
> + tristate "VFIO support for the GPU in the NVIDIA Grace Hopper Superchip"
> + depends on ARM64 || (COMPILE_TEST && 64BIT)
> + select VFIO_PCI_CORE
> + help
> + VFIO support for the GPU in the NVIDIA Grace Hopper Superchip is
> + required to assign the GPU device to a VM using KVM/qemu/etc.
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/vfio/pci/nvgpu/Makefile b/drivers/vfio/pci/nvgpu/Makefile
> new file mode 100644
> index 000000000000..00fd3a078218
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgpu/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_NVGPU_VFIO_PCI) += nvgpu-vfio-pci.o
> +nvgpu-vfio-pci-y := main.o
> diff --git a/drivers/vfio/pci/nvgpu/main.c b/drivers/vfio/pci/nvgpu/main.c
> new file mode 100644
> index 000000000000..ff68d0c5f865
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgpu/main.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/vfio_pci_core.h>
> +#include <linux/vfio.h>
> +
> +struct dev_mem_properties {
> + uint64_t hpa;
u64 is preferred
> + uint64_t mem_length;
> +};
> +
> +struct nvgpu_vfio_pci_core_device {
> + struct vfio_pci_core_device core_device;
> + struct dev_mem_properties mem_prop;
> +};
> +
> +static int nvgpu_vfio_pci_open_device(struct vfio_device *core_vdev)
> +{
> + struct vfio_pci_core_device *vdev =
> + container_of(core_vdev, struct vfio_pci_core_device, vdev);
> + int ret;
> +
> + ret = vfio_pci_core_enable(vdev);
> + if (ret)
> + return ret;
> +
> + vfio_pci_core_finish_enable(vdev);
> +
> + return 0;
> +}
> +
> +static int nvgpu_vfio_pci_mmap(struct vfio_device *core_vdev,
> + struct vm_area_struct *vma)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgpu_vfio_pci_core_device, core_device.vdev);
> +
> + unsigned long start_pfn;
> + unsigned int index;
> + u64 req_len, pgoff;
> + int ret = 0;
> +
> + index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
> + if (index != VFIO_PCI_BAR2_REGION_INDEX)
> + return vfio_pci_core_mmap(core_vdev, vma);
> +
> + /*
> + * Request to mmap the BAR. Map to the CPU accessible memory on the
> + * GPU using the memory information gathered from the system ACPI
> + * tables.
> + */
> + start_pfn = nvdev->mem_prop.hpa >> PAGE_SHIFT;
May be use PHYS_PFN()
> + req_len = vma->vm_end - vma->vm_start;
> + pgoff = vma->vm_pgoff &
> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
> + return -EINVAL;
you cound introduce accessor macros for nvdev->mem_prop.mem_length and
nvdev->mem_prop.hpa
> + /*
> + * Perform a PFN map to the memory. The device BAR is backed by the
> + * GPU memory now. Check that the mapping does not overflow out of
> + * the GPU memory size.
> + *
> + * The available GPU memory size may not be power-of-2 aligned. Given
> + * that the memory is exposed as a BAR, the mapping request is of the
> + * power-of-2 aligned size. Map only up to the size of the GPU memory.
> + * If the memory access is beyond the actual GPU memory size, it will
> + * be handled by the vfio_device_ops read/write.
> + *
> + * During device reset, the GPU is safely disconnected to the CPU
> + * and access to the BAR will be immediately returned preventing
> + * machine check.
> + */
> + ret = remap_pfn_range(vma, vma->vm_start, start_pfn + pgoff,
> + min(req_len, nvdev->mem_prop.mem_length - pgoff),
> + vma->vm_page_prot);
> + if (ret)
> + return ret;
> +
> + vma->vm_pgoff = start_pfn + pgoff;
> +
> + return 0;
> +}
> +
> +static long nvgpu_vfio_pci_ioctl(struct vfio_device *core_vdev,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgpu_vfio_pci_core_device, core_device.vdev);
> +
> + unsigned long minsz = offsetofend(struct vfio_region_info, offset);
> + struct vfio_region_info info;
> +
> + if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + if (info.index == VFIO_PCI_BAR2_REGION_INDEX) {
> + /*
> + * Request to determine the BAR region information. Send the
> + * GPU memory information.
> + */
> + uint32_t size;
> + struct vfio_region_info_cap_sparse_mmap *sparse;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> +
> + size = sizeof(struct vfio_region_info_cap_sparse_mmap) +
> + (sizeof(struct vfio_region_sparse_mmap_area));
> +
> + /*
> + * Setup for sparse mapping for the device memory. Only the
> + * available device memory on the hardware is shown as a
> + * mappable region.
> + */
> + sparse = kmalloc(size, GFP_KERNEL);
kzalloc may be ?
you could use struct_size(...,1) for the flexible array allocation and
avoid size.
> + if (sparse == NULL)
"if (!sparse)" is more common
> + return -ENOMEM;
> +
> + sparse->nr_areas = 1;
> + sparse->areas[0].offset = 0;
> + sparse->areas[0].size = nvdev->mem_prop.mem_length;
> + sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> + sparse->header.version = 1;
> +
> + vfio_info_add_capability(&caps, &sparse->header, size);
This routine can fail.
> + info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> + /*
> + * The available GPU memory size may not be power-of-2 aligned.
> + * Given that the memory is exposed as a BAR and may not be
> + * aligned, roundup to the next power-of-2.
> + */
> + info.size = is_power_of_2(nvdev->mem_prop.mem_length) ?
> + nvdev->mem_prop.mem_length :
> + roundup_pow_of_two(nvdev->mem_prop.mem_length);
may be avoid the is_power_of_2() test. It doesn't seem necessary.
> + info.flags = VFIO_REGION_INFO_FLAG_READ |
> + VFIO_REGION_INFO_FLAG_WRITE |
> + VFIO_REGION_INFO_FLAG_MMAP;
> +
> + if (caps.size) {
> + info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
> + } else {
> + vfio_info_cap_shift(&caps, sizeof(info));
> + if (copy_to_user((void __user *)arg +
> + sizeof(info), caps.buf,
> + caps.size)) {
> + kfree(caps.buf);
kfree(sparse);
> + return -EFAULT;
> + }
> + info.cap_offset = sizeof(info);
> + }
> + kfree(caps.buf);
> + }
kfree(sparse);
> + return copy_to_user((void __user *)arg, &info, minsz) ?
> + -EFAULT : 0;
> + }
> + }
> +
> + return vfio_pci_core_ioctl(core_vdev, cmd, arg);
> +}
> +
> +static ssize_t nvgpu_vfio_pci_read(struct vfio_device *core_vdev,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. A read to the BAR2 region implies an
> + * access outside the available device memory on the hardware.
> + */
> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + return -EINVAL;
> +
> + return vfio_pci_core_read(core_vdev, buf, count, ppos);
> +
> +}
> +
> +static ssize_t nvgpu_vfio_pci_write(struct vfio_device *core_vdev,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. A write to the BAR2 region implies an
> + * access outside the available device memory on the hardware.
> + */
> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + return -EINVAL;
> +
> + return vfio_pci_core_write(core_vdev, buf, count, ppos);
> +}
> +
> +static const struct vfio_device_ops nvgpu_vfio_pci_ops = {
> + .name = "nvgpu-vfio-pci",
> + .init = vfio_pci_core_init_dev,
> + .release = vfio_pci_core_release_dev,
> + .open_device = nvgpu_vfio_pci_open_device,
> + .close_device = vfio_pci_core_close_device,
> + .ioctl = nvgpu_vfio_pci_ioctl,
> + .read = nvgpu_vfio_pci_read,
> + .write = nvgpu_vfio_pci_write,
> + .mmap = nvgpu_vfio_pci_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,
> +};
> +
> +static struct nvgpu_vfio_pci_core_device *nvgpu_drvdata(struct pci_dev *pdev)
> +{
> + struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
> +
> + return container_of(core_device, struct nvgpu_vfio_pci_core_device,
> + core_device);
> +}
> +
> +static int
> +nvgpu_vfio_pci_fetch_memory_property(struct pci_dev *pdev,
> + struct nvgpu_vfio_pci_core_device *nvdev)
> +{
> + int ret;
> +
> + /*
> + * The memory information is present in the system ACPI tables as DSD
> + * properties nvidia,gpu-mem-base-pa and nvidia,gpu-mem-size.
> + */
> + ret = device_property_read_u64(&(pdev->dev), "nvidia,gpu-mem-base-pa",
please remove the extra parentheses.
> + &(nvdev->mem_prop.hpa));
> + if (ret)
> + return ret;
> +
> + ret = device_property_read_u64(&(pdev->dev), "nvidia,gpu-mem-size",
same here.
> + &(nvdev->mem_prop.mem_length));
> + return ret;
> +}
> +
> +static int nvgpu_vfio_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev;
> + int ret;
> +
> + nvdev = vfio_alloc_device(nvgpu_vfio_pci_core_device, core_device.vdev,
> + &pdev->dev, &nvgpu_vfio_pci_ops);
> + if (IS_ERR(nvdev))
> + return PTR_ERR(nvdev);
> +
> + dev_set_drvdata(&pdev->dev, nvdev);
> +
> + ret = nvgpu_vfio_pci_fetch_memory_property(pdev, nvdev);
> + if (ret)
> + goto out_put_vdev;
I would query these DT properties before allocating the device.
> +
> + ret = vfio_pci_core_register_device(&nvdev->core_device);
> + if (ret)
> + goto out_put_vdev;
> +
> + return ret;
> +
> +out_put_vdev:
> + vfio_put_device(&nvdev->core_device.vdev);
> + return ret;
> +}
> +
> +static void nvgpu_vfio_pci_remove(struct pci_dev *pdev)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev = nvgpu_drvdata(pdev);
> + struct vfio_pci_core_device *vdev = &nvdev->core_device;
> +
> + vfio_pci_core_unregister_device(vdev);
> + vfio_put_device(&vdev->vdev);
> +}
> +
> +static const struct pci_device_id nvgpu_vfio_pci_table[] = {
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2342) },
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2343) },
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2345) },
It would be good to add definitions for the PCI IDs.
Thanks
C.
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, nvgpu_vfio_pci_table);
> +
> +static struct pci_driver nvgpu_vfio_pci_driver = {
> + .name = KBUILD_MODNAME,
> + .id_table = nvgpu_vfio_pci_table,
> + .probe = nvgpu_vfio_pci_probe,
> + .remove = nvgpu_vfio_pci_remove,
> + .err_handler = &vfio_pci_core_err_handlers,
> + .driver_managed_dma = true,
> +};
> +
> +module_pci_driver(nvgpu_vfio_pci_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Ankit Agrawal <[email protected]>");
> +MODULE_AUTHOR("Aniket Agashe <[email protected]>");
> +MODULE_DESCRIPTION(
> + "VFIO NVGPU PF - User Level driver for NVIDIA devices with CPU coherently accessible device memory");
On Fri, Jun 23, 2023 at 08:27:17AM +0200, Cédric Le Goater wrote:
> > + req_len = vma->vm_end - vma->vm_start;
> > + pgoff = vma->vm_pgoff &
> > + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> > + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
> > + return -EINVAL;
>
> you cound introduce accessor macros for nvdev->mem_prop.mem_length and
> nvdev->mem_prop.hpa
Accessors are not the usual style..
> > +static const struct pci_device_id nvgpu_vfio_pci_table[] = {
> > + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2342) },
> > + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2343) },
> > + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2345) },
>
> It would be good to add definitions for the PCI IDs.
The common form these days is to have a comment with the marketing
name, we stopped putting constants for every device a while ago.
Jason
On Fri, Jun 23, 2023 at 03:04:17PM +0200, Cédric Le Goater wrote:
> On 6/23/23 13:47, Jason Gunthorpe wrote:
> > On Fri, Jun 23, 2023 at 08:27:17AM +0200, Cédric Le Goater wrote:
> > > > + req_len = vma->vm_end - vma->vm_start;
> > > > + pgoff = vma->vm_pgoff &
> > > > + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> > > > + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
> > > > + return -EINVAL;
> > >
> > > you cound introduce accessor macros for nvdev->mem_prop.mem_length and
> > > nvdev->mem_prop.hpa
> >
> > Accessors are not the usual style..
>
> I meant something like
>
> #define nvgpu_mem_size(nvdev) (nvdev)->mem_prop.mem_length
> #define nvgpu_mem_pa_base(nvdev) (nvdev)->mem_prop.hpa
>
> This is minor.
Yeah, this is what I thought you ment, I'm against this kind of
obfuscation in the kernel without a very strong purpose, and if you do
something like this it should be a static inline function.
Jason
On 6/23/23 13:47, Jason Gunthorpe wrote:
> On Fri, Jun 23, 2023 at 08:27:17AM +0200, Cédric Le Goater wrote:
>>> + req_len = vma->vm_end - vma->vm_start;
>>> + pgoff = vma->vm_pgoff &
>>> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
>>> + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
>>> + return -EINVAL;
>>
>> you cound introduce accessor macros for nvdev->mem_prop.mem_length and
>> nvdev->mem_prop.hpa
>
> Accessors are not the usual style..
I meant something like
#define nvgpu_mem_size(nvdev) (nvdev)->mem_prop.mem_length
#define nvgpu_mem_pa_base(nvdev) (nvdev)->mem_prop.hpa
This is minor.
>
>>> +static const struct pci_device_id nvgpu_vfio_pci_table[] = {
>>> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2342) },
>>> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2343) },
>>> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2345) },
>>
>> It would be good to add definitions for the PCI IDs.
>
> The common form these days is to have a comment with the marketing
> name, we stopped putting constants for every device a while ago.
OK then.
Thanks,
C.
On Wed, Jun 21, 2023 at 08:07:20PM -0700, [email protected] wrote:
> + if (caps.size) {
> + info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
Shouldn't this be an error if we can't fit the caps into the response?
Silently discarding the caps seems wrong..
> +static ssize_t nvgpu_vfio_pci_read(struct vfio_device *core_vdev,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. A read to the BAR2 region implies an
> + * access outside the available device memory on the hardware.
> + */
> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + return -EINVAL;
What does the qemu do in this case? Crash the VM?
Jason
On 6/23/23 15:07, Jason Gunthorpe wrote:
> On Fri, Jun 23, 2023 at 03:04:17PM +0200, Cédric Le Goater wrote:
>> On 6/23/23 13:47, Jason Gunthorpe wrote:
>>> On Fri, Jun 23, 2023 at 08:27:17AM +0200, Cédric Le Goater wrote:
>>>>> + req_len = vma->vm_end - vma->vm_start;
>>>>> + pgoff = vma->vm_pgoff &
>>>>> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
>>>>> + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
>>>>> + return -EINVAL;
>>>>
>>>> you cound introduce accessor macros for nvdev->mem_prop.mem_length and
>>>> nvdev->mem_prop.hpa
>>>
>>> Accessors are not the usual style..
>>
>> I meant something like
>>
>> #define nvgpu_mem_size(nvdev) (nvdev)->mem_prop.mem_length
>> #define nvgpu_mem_pa_base(nvdev) (nvdev)->mem_prop.hpa
>>
>> This is minor.
>
> Yeah, this is what I thought you ment, I'm against this kind of
> obfuscation in the kernel without a very strong purpose, and if you do
> something like this it should be a static inline function.
OK. We could drop the intermediate struct attribute instead then.
Thanks,
C.
On Fri, Jun 23, 2023 at 05:07:37PM +0200, Cédric Le Goater wrote:
> On 6/23/23 15:07, Jason Gunthorpe wrote:
> > On Fri, Jun 23, 2023 at 03:04:17PM +0200, Cédric Le Goater wrote:
> > > On 6/23/23 13:47, Jason Gunthorpe wrote:
> > > > On Fri, Jun 23, 2023 at 08:27:17AM +0200, Cédric Le Goater wrote:
> > > > > > + req_len = vma->vm_end - vma->vm_start;
> > > > > > + pgoff = vma->vm_pgoff &
> > > > > > + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> > > > > > + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
> > > > > > + return -EINVAL;
> > > > >
> > > > > you cound introduce accessor macros for nvdev->mem_prop.mem_length and
> > > > > nvdev->mem_prop.hpa
> > > >
> > > > Accessors are not the usual style..
> > >
> > > I meant something like
> > >
> > > #define nvgpu_mem_size(nvdev) (nvdev)->mem_prop.mem_length
> > > #define nvgpu_mem_pa_base(nvdev) (nvdev)->mem_prop.hpa
> > >
> > > This is minor.
> >
> > Yeah, this is what I thought you ment, I'm against this kind of
> > obfuscation in the kernel without a very strong purpose, and if you do
> > something like this it should be a static inline function.
>
> OK. We could drop the intermediate struct attribute instead then.
Yah, if the struct isn't needed for some other reason
Jason
On Fri, 23 Jun 2023 10:26:05 -0300
Jason Gunthorpe <[email protected]> wrote:
> On Wed, Jun 21, 2023 at 08:07:20PM -0700, [email protected] wrote:
> > + if (caps.size) {
> > + info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
> > + if (info.argsz < sizeof(info) + caps.size) {
> > + info.argsz = sizeof(info) + caps.size;
> > + info.cap_offset = 0;
>
> Shouldn't this be an error if we can't fit the caps into the response?
> Silently discarding the caps seems wrong..
It's required for backwards compatibility. If a userspace doesn't
support the info ioctl capabilities chain, it gets the basic
information successfully, while an enlightened userspace makes use of
the flags to know that a capability chain is available but unreported
due to an insufficient buffer size, with the required size being
provided in the return structure.
> > +static ssize_t nvgpu_vfio_pci_read(struct vfio_device *core_vdev,
> > + char __user *buf, size_t count, loff_t *ppos)
> > +{
> > + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> > +
> > + /*
> > + * Only the device memory present on the hardware is mapped, which may
> > + * not be power-of-2 aligned. A read to the BAR2 region implies an
> > + * access outside the available device memory on the hardware.
> > + */
> > + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> > + return -EINVAL;
>
> What does the qemu do in this case? Crash the VM?
Yes, I don't think return -errno matches what we discussed for
returning -1 on read and dropping writes outside of the device memory.
Also see comments in my review that read/write should handle the
coherent memory area as well, the device should work with x-no-mmap=on.
Thanks,
Alex
On Wed, 21 Jun 2023 20:07:20 -0700
<[email protected]> wrote:
> From: Ankit Agrawal <[email protected]>
>
> NVIDIA's upcoming Grace Hopper Superchip provides a PCI-like device
> for the on-chip GPU that is the logical OS representation of the
> internal proprietary cache coherent interconnect.
>
> This representation has a number of limitations compared to a real PCI
> device, in particular, it does not model the coherent GPU memory
> aperture as a PCI config space BAR, and PCI doesn't know anything
> about cacheable memory types.
>
> Provide a VFIO PCI variant driver that adapts the unique PCI
> representation into a more standard PCI representation facing
> userspace. The GPU memory aperture is obtained from ACPI using
> device_property_read_u64(), according to the FW specification,
> and exported to userspace as a separate VFIO_REGION. Since the device
> implements only one 64-bit BAR (BAR0), the GPU memory aperture is mapped
> to the next available PCI BAR (BAR2). Qemu will then naturally generate a
> PCI device in the VM with two 64-bit BARs (where the cacheable aperture
> reported in BAR2).
>
> Since this memory region is actually cache coherent with the CPU, the
> VFIO variant driver will mmap it into VMA using a cacheable mapping. The
> mapping is done using remap_pfn_range().
>
> PCI BAR are aligned to the power-of-2, but the actual memory on the
> device may not. The physical address from the last device PFN up to the
> next power-of-2 aligned PA thus is handled by the vfio-pci read/write
> device ops which returns an error.
>
> This goes along with a qemu series to provides the necessary
> implementation of the Grace Hopper Superchip firmware specification so
> that the guest operating system can see the correct ACPI modeling for
> the coherent GPU device.
> https://www.mail-archive.com/[email protected]/msg967557.html
>
> This patch is split from a patch series being pursued separately:
> https://lore.kernel.org/lkml/[email protected]/
>
> Applied and tested over v6.4-rc6.
>
> Signed-off-by: Ankit Agrawal <[email protected]>
> ---
> v3 -> v4
> - Mapping the available device memory using sparse mmap. The region outside
> the device memory is handled by read/write ops.
> - Removed the fault handler added in v3.
>
> v2 -> v3
> - Added fault handler to map the region outside the physical GPU memory
> up to the next power-of-2 to a dummy PFN.
> - Changed to select instead of "depends on" VFIO_PCI_CORE for all the
> vfio-pci variant driver.
> - Code cleanup based on feedback comments.
> - Code implemented and tested against v6.4-rc4.
>
> v1 -> v2
> - Updated the wording of reference to BAR offset and replaced with
> index.
> - The GPU memory is exposed at the fixed BAR2_REGION_INDEX.
> - Code cleanup based on feedback comments.
>
> MAINTAINERS | 6 +
> drivers/vfio/pci/Kconfig | 2 +
> drivers/vfio/pci/Makefile | 2 +
> drivers/vfio/pci/hisilicon/Kconfig | 2 +-
> drivers/vfio/pci/mlx5/Kconfig | 2 +-
> drivers/vfio/pci/nvgpu/Kconfig | 10 +
> drivers/vfio/pci/nvgpu/Makefile | 3 +
> drivers/vfio/pci/nvgpu/main.c | 311 +++++++++++++++++++++++++++++
> 8 files changed, 336 insertions(+), 2 deletions(-)
> create mode 100644 drivers/vfio/pci/nvgpu/Kconfig
> create mode 100644 drivers/vfio/pci/nvgpu/Makefile
> create mode 100644 drivers/vfio/pci/nvgpu/main.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f794002a192e..f3b3115fdfed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22169,6 +22169,12 @@ L: [email protected]
> S: Maintained
> F: drivers/vfio/platform/
>
> +VFIO NVIDIA PCI DRIVER
> +M: Ankit Agrawal <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/vfio/pci/nvgpu/
> +
> VGA_SWITCHEROO
> R: Lukas Wunner <[email protected]>
> S: Maintained
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..ade18b0ffb7b 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -59,4 +59,6 @@ source "drivers/vfio/pci/mlx5/Kconfig"
>
> source "drivers/vfio/pci/hisilicon/Kconfig"
>
> +source "drivers/vfio/pci/nvgpu/Kconfig"
> +
> endif
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 24c524224da5..0c93d452d0da 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5/
>
> obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/
> +
> +obj-$(CONFIG_NVGPU_VFIO_PCI) += nvgpu/
> diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
> index 5daa0f45d2f9..38e90e05d68a 100644
> --- a/drivers/vfio/pci/hisilicon/Kconfig
> +++ b/drivers/vfio/pci/hisilicon/Kconfig
> @@ -2,12 +2,12 @@
> config HISI_ACC_VFIO_PCI
> tristate "VFIO PCI support for HiSilicon ACC devices"
> depends on ARM64 || (COMPILE_TEST && 64BIT)
> - depends on VFIO_PCI_CORE
> depends on PCI_MSI
> depends on CRYPTO_DEV_HISI_QM
> depends on CRYPTO_DEV_HISI_HPRE
> depends on CRYPTO_DEV_HISI_SEC2
> depends on CRYPTO_DEV_HISI_ZIP
> + select VFIO_PCI_CORE
As noted in other reviews, rebase to vfio next branch of linux-next.
> help
> This provides generic PCI support for HiSilicon ACC devices
> using the VFIO framework.
> diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
> index 29ba9c504a75..7088edc4fb28 100644
> --- a/drivers/vfio/pci/mlx5/Kconfig
> +++ b/drivers/vfio/pci/mlx5/Kconfig
> @@ -2,7 +2,7 @@
> config MLX5_VFIO_PCI
> tristate "VFIO support for MLX5 PCI devices"
> depends on MLX5_CORE
> - depends on VFIO_PCI_CORE
> + select VFIO_PCI_CORE
> help
> This provides migration support for MLX5 devices using the VFIO
> framework.
> diff --git a/drivers/vfio/pci/nvgpu/Kconfig b/drivers/vfio/pci/nvgpu/Kconfig
> new file mode 100644
> index 000000000000..066f764f7c5f
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgpu/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config NVGPU_VFIO_PCI
> + tristate "VFIO support for the GPU in the NVIDIA Grace Hopper Superchip"
> + depends on ARM64 || (COMPILE_TEST && 64BIT)
> + select VFIO_PCI_CORE
> + help
> + VFIO support for the GPU in the NVIDIA Grace Hopper Superchip is
> + required to assign the GPU device to a VM using KVM/qemu/etc.
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/vfio/pci/nvgpu/Makefile b/drivers/vfio/pci/nvgpu/Makefile
> new file mode 100644
> index 000000000000..00fd3a078218
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgpu/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_NVGPU_VFIO_PCI) += nvgpu-vfio-pci.o
> +nvgpu-vfio-pci-y := main.o
> diff --git a/drivers/vfio/pci/nvgpu/main.c b/drivers/vfio/pci/nvgpu/main.c
> new file mode 100644
> index 000000000000..ff68d0c5f865
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgpu/main.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/vfio_pci_core.h>
> +#include <linux/vfio.h>
> +
> +struct dev_mem_properties {
> + uint64_t hpa;
> + uint64_t mem_length;
> +};
> +
> +struct nvgpu_vfio_pci_core_device {
> + struct vfio_pci_core_device core_device;
> + struct dev_mem_properties mem_prop;
> +};
> +
> +static int nvgpu_vfio_pci_open_device(struct vfio_device *core_vdev)
> +{
> + struct vfio_pci_core_device *vdev =
> + container_of(core_vdev, struct vfio_pci_core_device, vdev);
> + int ret;
> +
> + ret = vfio_pci_core_enable(vdev);
> + if (ret)
> + return ret;
> +
> + vfio_pci_core_finish_enable(vdev);
> +
> + return 0;
> +}
> +
> +static int nvgpu_vfio_pci_mmap(struct vfio_device *core_vdev,
> + struct vm_area_struct *vma)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgpu_vfio_pci_core_device, core_device.vdev);
> +
> + unsigned long start_pfn;
> + unsigned int index;
> + u64 req_len, pgoff;
> + int ret = 0;
> +
> + index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
> + if (index != VFIO_PCI_BAR2_REGION_INDEX)
> + return vfio_pci_core_mmap(core_vdev, vma);
> +
> + /*
> + * Request to mmap the BAR. Map to the CPU accessible memory on the
> + * GPU using the memory information gathered from the system ACPI
> + * tables.
> + */
> + start_pfn = nvdev->mem_prop.hpa >> PAGE_SHIFT;
> + req_len = vma->vm_end - vma->vm_start;
> + pgoff = vma->vm_pgoff &
> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> + if (pgoff >= (nvdev->mem_prop.mem_length >> PAGE_SHIFT))
> + return -EINVAL;
> +
> + /*
> + * Perform a PFN map to the memory. The device BAR is backed by the
> + * GPU memory now. Check that the mapping does not overflow out of
> + * the GPU memory size.
> + *
> + * The available GPU memory size may not be power-of-2 aligned. Given
> + * that the memory is exposed as a BAR, the mapping request is of the
> + * power-of-2 aligned size. Map only up to the size of the GPU memory.
> + * If the memory access is beyond the actual GPU memory size, it will
> + * be handled by the vfio_device_ops read/write.
> + *
> + * During device reset, the GPU is safely disconnected to the CPU
> + * and access to the BAR will be immediately returned preventing
> + * machine check.
> + */
> + ret = remap_pfn_range(vma, vma->vm_start, start_pfn + pgoff,
> + min(req_len, nvdev->mem_prop.mem_length - pgoff),
> + vma->vm_page_prot);
This should error rather than adjusting the mapping length if it
exceeds the map'able range.
> + if (ret)
> + return ret;
> +
> + vma->vm_pgoff = start_pfn + pgoff;
> +
> + return 0;
> +}
> +
> +static long nvgpu_vfio_pci_ioctl(struct vfio_device *core_vdev,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgpu_vfio_pci_core_device, core_device.vdev);
> +
> + unsigned long minsz = offsetofend(struct vfio_region_info, offset);
> + struct vfio_region_info info;
> +
> + if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + if (info.index == VFIO_PCI_BAR2_REGION_INDEX) {
> + /*
> + * Request to determine the BAR region information. Send the
> + * GPU memory information.
> + */
> + uint32_t size;
> + struct vfio_region_info_cap_sparse_mmap *sparse;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> +
> + size = sizeof(struct vfio_region_info_cap_sparse_mmap) +
> + (sizeof(struct vfio_region_sparse_mmap_area));
> +
> + /*
> + * Setup for sparse mapping for the device memory. Only the
> + * available device memory on the hardware is shown as a
> + * mappable region.
> + */
> + sparse = kmalloc(size, GFP_KERNEL);
kzalloc()
> + if (sparse == NULL)
> + return -ENOMEM;
> +
> + sparse->nr_areas = 1;
> + sparse->areas[0].offset = 0;
> + sparse->areas[0].size = nvdev->mem_prop.mem_length;
> + sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> + sparse->header.version = 1;
> +
> + vfio_info_add_capability(&caps, &sparse->header, size);
> +
> + info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> + /*
> + * The available GPU memory size may not be power-of-2 aligned.
> + * Given that the memory is exposed as a BAR and may not be
> + * aligned, roundup to the next power-of-2.
> + */
> + info.size = is_power_of_2(nvdev->mem_prop.mem_length) ?
> + nvdev->mem_prop.mem_length :
> + roundup_pow_of_two(nvdev->mem_prop.mem_length);
The terinary seems unnecessary, roundup_pow_of_two() should be a nop if
already a power of 2.
> + info.flags = VFIO_REGION_INFO_FLAG_READ |
> + VFIO_REGION_INFO_FLAG_WRITE |
> + VFIO_REGION_INFO_FLAG_MMAP;
> +
> + if (caps.size) {
> + info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
> + } else {
> + vfio_info_cap_shift(&caps, sizeof(info));
> + if (copy_to_user((void __user *)arg +
> + sizeof(info), caps.buf,
> + caps.size)) {
> + kfree(caps.buf);
> + return -EFAULT;
> + }
> + info.cap_offset = sizeof(info);
> + }
> + kfree(caps.buf);
> + }
> +
> + return copy_to_user((void __user *)arg, &info, minsz) ?
> + -EFAULT : 0;
'sparse' is leaked.
> + }
> + }
> +
> + return vfio_pci_core_ioctl(core_vdev, cmd, arg);
> +}
> +
> +static ssize_t nvgpu_vfio_pci_read(struct vfio_device *core_vdev,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. A read to the BAR2 region implies an
> + * access outside the available device memory on the hardware.
> + */
> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + return -EINVAL;
Shouldn't this return -1 value for the data and drop writes in the
function below? Returning -errno is a different thing which could
cause error logging in the VMM if not crash the guest.
Also, mmap'd regions are not required to be mmap'd, read & write to the
coherent area should work through an ioremap.
I had also asked in the previous review whether "nvgpu" is already
overused. I see a python tool named nvgpu, an OpenXLA tool, various
nvgpu things related to Tegra, an nvgpu dialect for MLIR, etc. There
are over 5,000 hits on google for "nvgpu", only a few of which
reference development of this module. Is there a more unique name we
can use? Thanks,
Alex
> +
> + return vfio_pci_core_read(core_vdev, buf, count, ppos);
> +
> +}
> +
> +static ssize_t nvgpu_vfio_pci_write(struct vfio_device *core_vdev,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. A write to the BAR2 region implies an
> + * access outside the available device memory on the hardware.
> + */
> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + return -EINVAL;
> +
> + return vfio_pci_core_write(core_vdev, buf, count, ppos);
> +}
> +
> +static const struct vfio_device_ops nvgpu_vfio_pci_ops = {
> + .name = "nvgpu-vfio-pci",
> + .init = vfio_pci_core_init_dev,
> + .release = vfio_pci_core_release_dev,
> + .open_device = nvgpu_vfio_pci_open_device,
> + .close_device = vfio_pci_core_close_device,
> + .ioctl = nvgpu_vfio_pci_ioctl,
> + .read = nvgpu_vfio_pci_read,
> + .write = nvgpu_vfio_pci_write,
> + .mmap = nvgpu_vfio_pci_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,
> +};
> +
> +static struct nvgpu_vfio_pci_core_device *nvgpu_drvdata(struct pci_dev *pdev)
> +{
> + struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
> +
> + return container_of(core_device, struct nvgpu_vfio_pci_core_device,
> + core_device);
> +}
> +
> +static int
> +nvgpu_vfio_pci_fetch_memory_property(struct pci_dev *pdev,
> + struct nvgpu_vfio_pci_core_device *nvdev)
> +{
> + int ret;
> +
> + /*
> + * The memory information is present in the system ACPI tables as DSD
> + * properties nvidia,gpu-mem-base-pa and nvidia,gpu-mem-size.
> + */
> + ret = device_property_read_u64(&(pdev->dev), "nvidia,gpu-mem-base-pa",
> + &(nvdev->mem_prop.hpa));
> + if (ret)
> + return ret;
> +
> + ret = device_property_read_u64(&(pdev->dev), "nvidia,gpu-mem-size",
> + &(nvdev->mem_prop.mem_length));
> + return ret;
> +}
> +
> +static int nvgpu_vfio_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev;
> + int ret;
> +
> + nvdev = vfio_alloc_device(nvgpu_vfio_pci_core_device, core_device.vdev,
> + &pdev->dev, &nvgpu_vfio_pci_ops);
> + if (IS_ERR(nvdev))
> + return PTR_ERR(nvdev);
> +
> + dev_set_drvdata(&pdev->dev, nvdev);
> +
> + ret = nvgpu_vfio_pci_fetch_memory_property(pdev, nvdev);
> + if (ret)
> + goto out_put_vdev;
> +
> + ret = vfio_pci_core_register_device(&nvdev->core_device);
> + if (ret)
> + goto out_put_vdev;
> +
> + return ret;
> +
> +out_put_vdev:
> + vfio_put_device(&nvdev->core_device.vdev);
> + return ret;
> +}
> +
> +static void nvgpu_vfio_pci_remove(struct pci_dev *pdev)
> +{
> + struct nvgpu_vfio_pci_core_device *nvdev = nvgpu_drvdata(pdev);
> + struct vfio_pci_core_device *vdev = &nvdev->core_device;
> +
> + vfio_pci_core_unregister_device(vdev);
> + vfio_put_device(&vdev->vdev);
> +}
> +
> +static const struct pci_device_id nvgpu_vfio_pci_table[] = {
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2342) },
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2343) },
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2345) },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, nvgpu_vfio_pci_table);
> +
> +static struct pci_driver nvgpu_vfio_pci_driver = {
> + .name = KBUILD_MODNAME,
> + .id_table = nvgpu_vfio_pci_table,
> + .probe = nvgpu_vfio_pci_probe,
> + .remove = nvgpu_vfio_pci_remove,
> + .err_handler = &vfio_pci_core_err_handlers,
> + .driver_managed_dma = true,
> +};
> +
> +module_pci_driver(nvgpu_vfio_pci_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Ankit Agrawal <[email protected]>");
> +MODULE_AUTHOR("Aniket Agashe <[email protected]>");
> +MODULE_DESCRIPTION(
> + "VFIO NVGPU PF - User Level driver for NVIDIA devices with CPU coherently accessible device memory");
On Mon, Jun 26, 2023 at 10:06:29AM -0600, Alex Williamson wrote:
> On Fri, 23 Jun 2023 10:26:05 -0300
> Jason Gunthorpe <[email protected]> wrote:
>
> > On Wed, Jun 21, 2023 at 08:07:20PM -0700, [email protected] wrote:
> > > + if (caps.size) {
> > > + info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
> > > + if (info.argsz < sizeof(info) + caps.size) {
> > > + info.argsz = sizeof(info) + caps.size;
> > > + info.cap_offset = 0;
> >
> > Shouldn't this be an error if we can't fit the caps into the response?
> > Silently discarding the caps seems wrong..
>
> It's required for backwards compatibility. If a userspace doesn't
> support the info ioctl capabilities chain, it gets the basic
> information successfully, while an enlightened userspace makes use of
> the flags to know that a capability chain is available but unreported
> due to an insufficient buffer size, with the required size being
> provided in the return structure.
Hum, maybe this part should be pushed into the cap helpers..
Jason
>>> +static ssize_t nvgpu_vfio_pci_read(struct vfio_device *core_vdev,
>>> + char __user *buf, size_t count, loff_t *ppos) {
>>> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
>>> +
>>> + /*
>>> + * Only the device memory present on the hardware is mapped, which may
>>> + * not be power-of-2 aligned. A read to the BAR2 region implies an
>>> + * access outside the available device memory on the hardware.
>>> + */
>>> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
>>> + return -EINVAL;
>>>
>> What does the qemu do in this case? Crash the VM?
>
> Yes, I don't think return -errno matches what we discussed for returning -1 on read and dropping writes outside of the device memory.
I tried to replicate what is done by vfio-pci core module in vfio_pci_bar_rw()
where -EINVAL is returned in case of e.g. the access offset is outside the
BAR range.
Such an error comes to Qemu in vfio_region_read/vfio_region_write() which
handles the error by returning data as -1 on read and dropping on write.
But it seems that we don't need such approach here. I will make the change
so that vfio-pci variant driver's read/write explicitly reads -1 and drops writes
instead of returning -EINVAL.
> I had also asked in the previous review whether "nvgpu" is already overused. I
> see a python tool named nvgpu, an OpenXLA tool, various nvgpu things related
> to Tegra, an nvgpu dialect for MLIR, etc. There are over 5,000 hits on google for
> "nvgpu", only a few of which reference development of this module. Is there a
> more unique name we can use? Thanks,
Sorry, had missed this comment. Are you suggesting changing the module name
or just reduce the number of times we use the nvgpu keyword in all the functions
of the module? I don't see any in-tree or vfio-pci module with a similar *nvgpu*
name, and the clash appears to be with items outside of the kernel tree. Given
that, should we still change the module name as nvgpu-vfio-pci sounds a relevant
name here? Thanks.
On Wed, 5 Jul 2023 18:37:42 +0000
Ankit Agrawal <[email protected]> wrote:
> > I had also asked in the previous review whether "nvgpu" is already overused. I
> > see a python tool named nvgpu, an OpenXLA tool, various nvgpu things related
> > to Tegra, an nvgpu dialect for MLIR, etc. There are over 5,000 hits on google for
> > "nvgpu", only a few of which reference development of this module. Is there a
> > more unique name we can use? Thanks,
>
> Sorry, had missed this comment. Are you suggesting changing the module name
> or just reduce the number of times we use the nvgpu keyword in all the functions
> of the module? I don't see any in-tree or vfio-pci module with a similar *nvgpu*
> name, and the clash appears to be with items outside of the kernel tree. Given
> that, should we still change the module name as nvgpu-vfio-pci sounds a relevant
> name here? Thanks.
I'm referring to the module name, which in turn would be reflected in
various function names. The fact that there's no in-tree *nvgpu*
driver seems irrelevant when a web search for the term shows a variety
of tools and drivers, I believe there's even an out-of-tree NVIDIA
sponsored nvgpu driver for Android, correct? How does this relate to
that? I don't think it does, so why generate confusion?
I don't know your future plans for this driver, but it's currently
limited to exposing essentially a single feature on a very, very small
product subset, while "nvgpu" seems to project something much more
generic.
If we're going to see more of devices exposing coherent memory with
CXL, does that mean this driver might be short lived and perhaps won't
see further expansion in functionality? If so maybe it should be named
more specifically for the product it supports. I see some NVIDIA pages
referring to the GH200 superchip, maybe "GH", ex. "nvgh", "nvgh-gpu"?
Reading through the datasheet, I'm also reminded of issues we had with
the POWER implementation relative to isolation, since this coherent
memory is enabled via NVLink-C2C, which is opaque to Linux. The
datasheet claims "[f]ourth-generation NVLink allows accessing peer
memory with direct loads, sotres, and atomic operations...", are those
direct accesses reflected in the PCI topology, ie. the PCIe ACS exposed
isolation, or is the peer here limited to the CPU? Thanks,
Alex