Re: [PATCH v14 1/1] vfio/nvgrace-gpu: Add vfio pci variant module for grace hopper

From: Alex Williamson
Date: Tue Dec 12 2023 - 17:27:51 EST


On Wed, 13 Dec 2023 00:16:13 +0530
<ankita@xxxxxxxxxx> wrote:

> From: Ankit Agrawal <ankita@xxxxxxxxxx>
>
> 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 chip-to-chip cache coherent interconnect.
>
> The device is peculiar compared to a real PCI device in that whilst
> there is a real 64b PCI BAR1 (comprising region 2 & region 3) on the
> device, it is not used to access device memory once the faster
> chip-to-chip interconnect is initialized (occurs at the time of host
> system boot). The device memory is accessed instead using the chip-to-chip
> interconnect that is exposed as a contiguous physically addressable
> region on the host. This device memory aperture can be obtained from host
> ACPI table using device_property_read_u64(), according to the FW
> specification. Since the device memory is cache coherent with the CPU,
> it can be mmap into the user VMA with a cacheable mapping using
> remap_pfn_range() and used like a regular RAM. The device memory
> is not added to the host kernel, but mapped directly as this reduces
> memory wastage due to struct pages.
>
> There is also a requirement of a reserved 1G uncached region (termed as
> resmem) to support the Multi-Instance GPU (MIG) feature [1]. Based on [2],
> the requisite properties (uncached, unaligned access) can be achieved
> through a VM mapping (S1) of NORMAL_NC and host (S2) mapping with
> MemAttr[2:0]=0b101. To provide a different non-cached property to the
> reserved 1G region, it needs to be carved out from the device memory and
> mapped as a separate region in Qemu VMA with pgprot_writecombine().
> pgprot_writecombine() sets the Qemu VMA page properties (pgprot) as
> NORMAL_NC.
>
> Provide a VFIO PCI variant driver that adapts the unique device memory
> representation into a more standard PCI representation facing userspace.
>
> The variant driver exposes these two regions - the non-cached reserved
> (resmem) and the cached rest of the device memory (termed as usemem) as
> separate VFIO 64b BAR regions. Since the device implements 64-bit BAR0,
> the VFIO PCI variant driver maps the cached device memory aperture to
> the next available PCI BAR (i.e. comprising of region 2 and 3). The
> uncached carved out region is assigned BAR region 4 and 5. Qemu will
> then naturally generate a PCI device in the VM with the cacheable aperture
> reported as BAR2 region, the uncached as BAR4. The variant driver also
> provides emulation for these fake BARs' PCI config space offset registers.
>
> The memory layout on the host looks like the following:
> devmem (memlength)
> |--------------------------------------------------|
> |-------------cached------------------------|--NC--|
> | |
> usemem.phys/memphys resmem.phys
>
> PCI BARs need to be aligned to the power-of-2, but the actual memory on the
> device may not. A read or write access to the physical address from the
> last device PFN up to the next power-of-2 aligned physical address
> results in reading ~0 and dropped writes. Note that the GPU device
> driver [6] is capable of knowing the exact device memory size through
> separate means. The device memory size is primarily kept in the system
> ACPI tables for use by the VFIO PCI variant module.
>
> Note that the usemem memory is added by the VM Nvidia device driver [5]
> to the VM kernel as memblocks. Hence make the usable memory size memblock
> aligned.
>
> Currently there is no provision in KVM for a S2 mapping with
> MemAttr[2:0]=0b101, but there is an ongoing effort to provide the same [3].
> As previously mentioned, resmem is mapped pgprot_writecombine(), that
> sets the Qemu VMA page properties (pgprot) as NORMAL_NC. Using the
> proposed changes in [4] and [3], KVM marks the region with
> MemAttr[2:0]=0b101 in S2.
>
> This goes along with a qemu series [6] 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. Verified with the CUDA workload in the VM.
>
> [1] https://www.nvidia.com/en-in/technologies/multi-instance-gpu/
> [2] section D8.5.5 of https://developer.arm.com/documentation/ddi0487/latest/
> [3] https://lore.kernel.org/all/20231205033015.10044-1-ankita@xxxxxxxxxx/
> [4] https://lore.kernel.org/all/20230907181459.18145-2-ankita@xxxxxxxxxx/
> [5] https://github.com/NVIDIA/open-gpu-kernel-modules
> [6] https://lore.kernel.org/all/20231203060245.31593-1-ankita@xxxxxxxxxx/
>
> Applied over next-20231211.
> ---
> Link for variant driver v13:
> https://lore.kernel.org/all/20231114081611.30550-1-ankita@xxxxxxxxxx/
> Link for new carve-out region v1:
> https://lore.kernel.org/all/20231115080751.4558-1-ankita@xxxxxxxxxx/
>
> v13 -> v14
> - Merged the changes for second BAR implementation for MIG support
> on the device driver.
> https://lore.kernel.org/all/20231115080751.4558-1-ankita@xxxxxxxxxx/
> - Added the missing implementation of sub-word access to fake BARs'
> PCI config access. Implemented access algorithm suggested by
> Alex Williamson in the comments (Thanks!)
> - Added support to BAR accesses on the reserved memory with
> Qemu device param x-no-mmap=on.
> - Handled endian-ness in the PCI config space access.
> - Git commit message change
>
> v12 -> v13
> - Added emulation for the PCI config space BAR offset register for
> the fake BAR.
> - commit message updated with more details on the BAR offset emulation.
>
> v11 -> v12
> - More details in commit message on device memory size
>
> v10 -> v11
> - Removed sysfs attribute to expose the CPU coherent memory feature
> - Addressed review comments
>
> v9 -> v10
> - Add new sysfs attribute to expose the CPU coherent memory feature.
>
> v8 -> v9
> - Minor code adjustment suggested in v8.
>
> v7 -> v8
> - Various field names updated.
> - Added a new function to handle VFIO_DEVICE_GET_REGION_INFO ioctl.
> - Locking protection for memremap to bar region and other changes
> recommended in v7.
> - Added code to fail if the devmem size advertized is 0 in system DSDT.
>
> v6 -> v7
> - Handled out-of-bound and overflow conditions at various places to validate
> input offset and length.
> - Added code to return EINVAL for offset beyond region size.
>
> v5 -> v6
> - Added the code to handle BAR2 read/write using memremap to the device
> memory.
>
> v4 -> v5
> - Changed the module name from nvgpu-vfio-pci to nvgrace-gpu-vfio-pci.
> - Fixed memory leak and added suggested boundary checks on device memory
> mapping.
> - Added code to read all Fs and ignored write on region outside of the
> physical memory.
> - Other miscellaneous cleanup suggestions.
>
> 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.
>
> Signed-off-by: Ankit Agrawal <ankita@xxxxxxxxxx>
> Signed-off-by: Aniket Agashe <aniketa@xxxxxxxxxx>
> Tested-by: Ankit Agrawal <ankita@xxxxxxxxxx>
> ---
> MAINTAINERS | 6 +
> drivers/vfio/pci/Kconfig | 2 +
> drivers/vfio/pci/Makefile | 2 +
> drivers/vfio/pci/nvgrace-gpu/Kconfig | 10 +
> drivers/vfio/pci/nvgrace-gpu/Makefile | 3 +
> drivers/vfio/pci/nvgrace-gpu/main.c | 947 ++++++++++++++++++++++++++
> 6 files changed, 970 insertions(+)
> create mode 100644 drivers/vfio/pci/nvgrace-gpu/Kconfig
> create mode 100644 drivers/vfio/pci/nvgrace-gpu/Makefile
> create mode 100644 drivers/vfio/pci/nvgrace-gpu/main.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 98f7dd0499f1..6f8f3a6daa43 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22877,6 +22877,12 @@ L: kvm@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/vfio/platform/
>
> +VFIO NVIDIA GRACE GPU DRIVER
> +M: Ankit Agrawal <ankita@xxxxxxxxxx>
> +L: kvm@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: drivers/vfio/pci/nvgrace-gpu/
> +
> VGA_SWITCHEROO
> R: Lukas Wunner <lukas@xxxxxxxxx>
> S: Maintained
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index 8125e5f37832..2456210e85f1 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -65,4 +65,6 @@ source "drivers/vfio/pci/hisilicon/Kconfig"
>
> source "drivers/vfio/pci/pds/Kconfig"
>
> +source "drivers/vfio/pci/nvgrace-gpu/Kconfig"
> +
> endmenu
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 45167be462d8..1352c65e568a 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -13,3 +13,5 @@ obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5/
> obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/
>
> obj-$(CONFIG_PDS_VFIO_PCI) += pds/
> +
> +obj-$(CONFIG_NVGRACE_GPU_VFIO_PCI) += nvgrace-gpu/
> diff --git a/drivers/vfio/pci/nvgrace-gpu/Kconfig b/drivers/vfio/pci/nvgrace-gpu/Kconfig
> new file mode 100644
> index 000000000000..936e88d8d41d
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgrace-gpu/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config NVGRACE_GPU_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 using KVM/qemu/etc.
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/vfio/pci/nvgrace-gpu/Makefile b/drivers/vfio/pci/nvgrace-gpu/Makefile
> new file mode 100644
> index 000000000000..3ca8c187897a
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgrace-gpu/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_NVGRACE_GPU_VFIO_PCI) += nvgrace-gpu-vfio-pci.o
> +nvgrace-gpu-vfio-pci-y := main.o
> diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c
> new file mode 100644
> index 000000000000..b86897da0ab0
> --- /dev/null
> +++ b/drivers/vfio/pci/nvgrace-gpu/main.c
> @@ -0,0 +1,947 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/vfio_pci_core.h>
> +#include <linux/vfio.h>
> +
> +/* Memory size expected as non cached and reserved by the VM driver */
> +#define RESMEM_SIZE 0x40000000
> +#define MEMBLK_SIZE 0x20000000
> +
> +/*
> + * The device memory usable to the workloads running in the VM is cached
> + * and showcased as a 64b device BAR to the VM (represented as usemem).
> + * Moreover, the VM GPU device driver needs a non-cacheable region to
> + * support the MIG feature. This region is also exposed as a 64b BAR and
> + * represented as resmem.
> + *
> + * Each of these regions' state is saves as struct mem_region.
> + */
> +struct mem_region {
> + phys_addr_t memphys; /* Base physical address of the region */
> + size_t memlength; /* Region size */
> + union {
> + u32 u32_reg[2];
> + u64 u64_reg;
> + } bar_reg; /* Emulated BAR offset registers */
> + union {
> + void *memaddr;
> + void __iomem *ioaddr;
> + } bar_remap; /* Base virtual address of the region */
> +};
> +
> +struct nvgrace_gpu_vfio_pci_core_device {
> + struct vfio_pci_core_device core_device;
> + /* Cached and usable memory for the VM. */
> + struct mem_region usemem;
> + /* Non cached memory carved out from the end of device memory */
> + struct mem_region resmem;
> + struct mutex remap_lock;
> +};
> +
> +static bool nvgrace_gpu_vfio_pci_is_fake_bar(int index)
> +{
> + if (index == VFIO_PCI_BAR2_REGION_INDEX ||
> + index == VFIO_PCI_BAR4_REGION_INDEX)
> + return true;
> +
> + return false;
> +}

return (index == VFIO_PCI_BAR2_REGION_INDEX) ||
(index == VFIO_PCI_BAR4_REGION_INDEX);


> +
> +static void nvgrace_gpu_init_fake_bar_emu_regs(struct vfio_device *core_vdev)
> +{
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device,
> + core_device.vdev);
> +
> + nvdev->resmem.bar_reg.u64_reg = 0;
> + nvdev->usemem.bar_reg.u64_reg = 0;
> +}
> +
> +/* Choose the structure corresponding to the BAR with index. */
> +static int
> +nvgrace_gpu_vfio_pci_get_mem_region(int index,
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + struct mem_region *region)
> +{
> + if (index == VFIO_PCI_BAR4_REGION_INDEX)
> + *region = nvdev->usemem;
> + else if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + *region = nvdev->resmem;
> + else
> + return -EINVAL;
> +
> + return 0;

This seems redundant to the is_fake_bar function. For example:

struct mem_region *
nvgrace_gpu_vfio_pci_get_mem_region(int index,
struct nvgrace_gpu_vfio_pci_core_device *nvdev)
{
if (index == VFIO_PCI_BAR4_REGION_INDEX)
return nvdev->usemem;
if (index == VFIO_PCI_BAR2_REGION_INDEX)
return nvdev->resmem;
return NULL;
}

Could be used to replace both functions. It's strange that the
original is copying the struct rather than providing a pointer to the
correct struct anyway, seems like that's going to lead to subtle issues.

> +}
> +
> +static int nvgrace_gpu_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);
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device,
> + core_device.vdev);
> + int ret;
> +
> + ret = vfio_pci_core_enable(vdev);
> + if (ret)
> + return ret;
> +
> + vfio_pci_core_finish_enable(vdev);
> +
> + nvgrace_gpu_init_fake_bar_emu_regs(core_vdev);
> +
> + mutex_init(&nvdev->remap_lock);
> +
> + return 0;
> +}
> +
> +static void nvgrace_gpu_vfio_pci_close_device(struct vfio_device *core_vdev)
> +{
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device,
> + core_device.vdev);
> +
> + /* Unmap the mapping to the device memory cached region */
> + if (nvdev->usemem.bar_remap.memaddr) {
> + memunmap(nvdev->usemem.bar_remap.memaddr);
> + nvdev->usemem.bar_remap.memaddr = NULL;
> + }
> +
> + /* Unmap the mapping to the device memory non-cached region */
> + if (nvdev->resmem.bar_remap.ioaddr) {
> + iounmap(nvdev->resmem.bar_remap.ioaddr);
> + nvdev->resmem.bar_remap.ioaddr = NULL;
> + }
> +
> + mutex_destroy(&nvdev->remap_lock);
> +
> + vfio_pci_core_close_device(core_vdev);
> +}
> +
> +static int nvgrace_gpu_vfio_pci_mmap(struct vfio_device *core_vdev,
> + struct vm_area_struct *vma)
> +{
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device, core_device.vdev);
> +
> + unsigned long start_pfn;
> + unsigned int index;
> + u64 req_len, pgoff, end;
> + int ret = 0;
> + struct mem_region memregion;
> +
> + index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
> +
> + if (!nvgrace_gpu_vfio_pci_is_fake_bar(index))
> + return vfio_pci_core_mmap(core_vdev, vma);
> +
> + ret = nvgrace_gpu_vfio_pci_get_mem_region(index, nvdev, &memregion);
> + if (ret)
> + return ret;
> +
> + /*
> + * Request to mmap the BAR. Map to the CPU accessible memory on the
> + * GPU using the memory information gathered from the system ACPI
> + * tables.
> + */
> + pgoff = vma->vm_pgoff &
> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> +
> + if (check_sub_overflow(vma->vm_end, vma->vm_start, &req_len) ||
> + check_add_overflow(PHYS_PFN(memregion.memphys), pgoff, &start_pfn) ||
> + check_add_overflow(PFN_PHYS(pgoff), req_len, &end))
> + return -EOVERFLOW;
> +
> + /*
> + * Check that the mapping request does not go beyond available device
> + * memory size
> + */
> + if (end > memregion.memlength)
> + return -EINVAL;
> +
> + /*
> + * The carved out region of the device memory needs the NORMAL_NC
> + * property. Communicate as such to the hypervisor.
> + */
> + if (index == VFIO_PCI_BAR2_REGION_INDEX)
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);

Consider:

#define USEMEM_REGION_INDEX VFIO_PCI_BAR4_REGION_INDEX
#define RESMEM_REGION_INDEX VFIO_PCI_BAR2_REGION_INDEX

And use them throughout. I think this is already confused with the
commit log above:

The variant driver exposes these two regions - the non-cached
reserved (resmem) and the cached rest of the device memory
(termed as usemem) as separate VFIO 64b BAR regions. Since the
device implements 64-bit BAR0, the VFIO PCI variant driver maps
the cached device memory aperture to the next available PCI BAR
(i.e. comprising of region 2 and 3). The uncached carved out
region is assigned BAR region 4 and 5. Qemu will then naturally
generate a PCI device in the VM with the cacheable aperture
reported as BAR2 region, the uncached as BAR4.

That says BAR2 = usemem and BAR4 = resmem. get_mem_region says BAR4 =
usemem and BAR2 = resmem. Pick one.
> +
> + /*
> + * Perform a PFN map to the memory and back the device BAR by the
> + * GPU memory.
> + *
> + * The available GPU memory size may not be power-of-2 aligned. Map up
> + * to the size of the device 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,
> + req_len, vma->vm_page_prot);
> + if (ret)
> + return ret;
> +
> + vma->vm_pgoff = start_pfn;
> +
> + return 0;
> +}
> +
> +static long
> +nvgrace_gpu_vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
> + unsigned long arg)
> +{
> + unsigned long minsz = offsetofend(struct vfio_region_info, offset);
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device, core_device.vdev);
> + struct vfio_region_info_cap_sparse_mmap *sparse;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> + struct vfio_region_info info;
> + struct mem_region memregion;
> + uint32_t size;
> + int ret;
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + if (nvgrace_gpu_vfio_pci_is_fake_bar(info.index)) {

Probably makes sense to flip the test polarity, nest the core ioctl
call within this branch and do all the work in the main flow.
Otherwise the below probably needs a separate function.

> + ret = nvgrace_gpu_vfio_pci_get_mem_region(info.index, nvdev, &memregion);
> + if (ret)
> + return ret;
> + /*
> + * Request to determine the BAR region information. Send the
> + * GPU memory information.
> + */
> + size = struct_size(sparse, areas, 1);
> +
> + /*
> + * Setup for sparse mapping for the device memory. Only the
> + * available device memory on the hardware is shown as a
> + * mappable region.
> + */
> + sparse = kzalloc(size, GFP_KERNEL);
> + if (!sparse)
> + return -ENOMEM;
> +
> + sparse->nr_areas = 1;
> + sparse->areas[0].offset = 0;
> + sparse->areas[0].size = memregion.memlength;
> + sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> + sparse->header.version = 1;
> +
> + ret = vfio_info_add_capability(&caps, &sparse->header, size);
> + kfree(sparse);
> + if (ret)
> + return ret;
> +
> + info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> + /*
> + * The region memory size may not be power-of-2 aligned.
> + * Given that the memory as a BAR and may not be
> + * aligned, roundup to the next power-of-2.
> + */
> + info.size = roundup_pow_of_two(memregion.memlength);
> + 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, VFIO_DEVICE_GET_REGION_INFO, arg);
> +}
> +
> +static long nvgrace_gpu_vfio_pci_ioctl(struct vfio_device *core_vdev,
> + unsigned int cmd, unsigned long arg)
> +{
> + if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> + return nvgrace_gpu_vfio_pci_ioctl_get_region_info(core_vdev, arg);
> +
> + if (cmd == VFIO_DEVICE_RESET)
> + nvgrace_gpu_init_fake_bar_emu_regs(core_vdev);
> +
> + return vfio_pci_core_ioctl(core_vdev, cmd, arg);

A switch might be useful to make this fallthrough explicit.

switch (cmd) {
case VFIO_DEVICE_GET_REGION_INFO:
return nvgrace_gpu_vfio_pci_ioctl_get_region_info(core_vdev, arg);
case VFIO_DEVICE_RESET:
nvgrace_gpu_init_fake_bar_emu_regs(core_vdev);
fallthrough;
default:
return vfio_pci_core_ioctl(core_vdev, cmd, arg);
}

I think you also need to consider VFIO_DEVICE_IOEVENTFD. It doesn't
need to be supported for BAR2&4 but it doesn't make any sense to let it
pass through to the core code since it would enable writes to the
physical BARs.

> +}
> +
> +/*
> + * Check whether the given range of offset is within the expected
> + * boundary.
> + */
> +static bool
> +range_within_boundary(loff_t boundary_start, loff_t boundary_end,
> + loff_t range_start, loff_t range_end)
> +{
> + if (boundary_start <= range_start &&
> + boundary_end >= range_end) {
> + return true;
> + }
> + return false;
> +}
> +
> +/*
> + * Both the usable (usemem) and the reserved (resmem) device memory region
> + * are exposed as a 64b fake BARs in the VM. These fake BARs must respond
> + * to the accesses on their respective PCI config space offsets.
> + *
> + * resmem BAR owns PCI_BASE_ADDRESS_2 & PCI_BASE_ADDRESS_3.
> + * usemem BAR owns PCI_BASE_ADDRESS_4 & PCI_BASE_ADDRESS_5.
> + */
> +
> +/*
> + * Return the pointer to the desired location within the appropriate
> + * emulated register.
> + *
> + * First find the emulated register based on the accessed PCI config
> + * offset. Then traverse to the appropriate location within the
> + * register (applicable for sub-word size access).
> + */
> +static u8 *
> +register_ref(struct nvgrace_gpu_vfio_pci_core_device *nvdev, u64 start, u64 end)
> +{
> + u8 *reg_ref = NULL;
> +
> + if (range_within_boundary(PCI_BASE_ADDRESS_5,
> + PCI_CARDBUS_CIS, start, end)) {
> + reg_ref = (u8 *) &(nvdev->usemem.bar_reg.u32_reg[1]);
> + reg_ref += start - PCI_BASE_ADDRESS_5;
> + } else if (range_within_boundary(PCI_BASE_ADDRESS_4,
> + PCI_BASE_ADDRESS_5, start, end)) {
> + reg_ref = (u8 *) &(nvdev->usemem.bar_reg.u32_reg[0]);
> + reg_ref += start - PCI_BASE_ADDRESS_4;
> + } else if (range_within_boundary(PCI_BASE_ADDRESS_3,
> + PCI_BASE_ADDRESS_4, start, end)) {
> + reg_ref = (u8 *) &(nvdev->resmem.bar_reg.u32_reg[1]);
> + reg_ref += start - PCI_BASE_ADDRESS_3;
> + } else if (range_within_boundary(PCI_BASE_ADDRESS_2,
> + PCI_BASE_ADDRESS_3, start, end)) {
> + reg_ref = (u8 *) &(nvdev->resmem.bar_reg.u32_reg[0]);
> + reg_ref += start - PCI_BASE_ADDRESS_2;
> + }
> +
> + return reg_ref;
> +}
> +
> +static ssize_t
> +nvgrace_gpu_read_config_emu(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + u64 pos = *ppos & VFIO_PCI_OFFSET_MASK;
> + __le32 val32;
> + u8 *reg_ref;
> + u32 tmp_val;
> + size_t bar_size;
> +
> + if (!IS_ALIGNED(pos, count))
> + return -EINVAL;
> +
> + if (range_within_boundary(PCI_BASE_ADDRESS_2,
> + PCI_BASE_ADDRESS_4, pos, pos + count)) {
> + bar_size = roundup_pow_of_two(nvdev->resmem.memlength);
> + nvdev->resmem.bar_reg.u64_reg &= ~(bar_size - 1);
> + nvdev->resmem.bar_reg.u64_reg |= PCI_BASE_ADDRESS_MEM_TYPE_64 |
> + PCI_BASE_ADDRESS_MEM_PREFETCH;
> + } else {
> + bar_size = roundup_pow_of_two(nvdev->usemem.memlength);
> + nvdev->usemem.bar_reg.u64_reg &= ~(bar_size - 1);
> + nvdev->usemem.bar_reg.u64_reg |= PCI_BASE_ADDRESS_MEM_TYPE_64 |
> + PCI_BASE_ADDRESS_MEM_PREFETCH;
> + }
> +
> + reg_ref = register_ref(nvdev, pos, pos + count);
> + if (!reg_ref)
> + return -EINVAL;
> +
> + switch (count) {
> + case 1:
> + tmp_val = *reg_ref;
> + break;
> + case 2:
> + tmp_val = *((u16 *) reg_ref);
> + break;
> + case 4:
> + tmp_val = *((u32 *) reg_ref);
> + break;
> + }
> +
> + val32 = cpu_to_le32(tmp_val);
> +
> + if (copy_to_user(buf, &val32, count))
> + return -EFAULT;
> +
> + *ppos += count;
> +
> + return count;
> +}
> +
> +static ssize_t
> +nvgrace_gpu_write_config_emu(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u64 pos = *ppos & VFIO_PCI_OFFSET_MASK;
> + u8 *reg_ref;
> + __le32 val;
> + u32 tmp_val;
> +
> + if (!IS_ALIGNED(pos, count))
> + return -EINVAL;
> +
> + if (copy_from_user(&val, buf, count))
> + return -EFAULT;
> +
> + reg_ref = register_ref(nvdev, pos, pos + count);
> + if (!reg_ref)
> + return -EINVAL;
> +
> + tmp_val = le32_to_cpu(val);
> +
> + switch (count) {
> + case 1:
> + *reg_ref = tmp_val;
> + break;
> + case 2:
> + *((u16 *) reg_ref) = tmp_val;
> + break;
> + case 4:
> + *((u32 *) reg_ref) = tmp_val;
> + break;
> + }
> +
> + *ppos += count;
> + return count;
> +}

Please just use the code Yishai proposed for the virtio-vfio-pci driver:

https://lore.kernel.org/all/20231207102820.74820-10-yishaih@xxxxxxxxxx

It's more generic, I believe it works correctly, and we don't need
multiple implementations.

> +
> +/*
> + * Ad hoc map the device memory in the module kernel VA space. Primarily needed
> + * to support Qemu's device x-no-mmap=on option.
> + *
> + * The usemem region is cacheable memory and hence is memremaped.
> + * The resmem region is non-cached and is mapped using ioremap_wc (NORMAL_NC).
> + */
> +static int
> +nvgrace_gpu_map_device_mem(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + int index)
> +{
> + mutex_lock(&nvdev->remap_lock);
> + if (index == VFIO_PCI_BAR4_REGION_INDEX &&
> + !nvdev->usemem.bar_remap.memaddr) {
> + nvdev->usemem.bar_remap.memaddr
> + = memremap(nvdev->usemem.memphys, nvdev->usemem.memlength, MEMREMAP_WB);
> + if (!nvdev->usemem.bar_remap.memaddr) {
> + mutex_unlock(&nvdev->remap_lock);
> + return -ENOMEM;
> + }

Or just declare int ret = 0; and set ret = -ENOMEM; here and use the
common exit path.

> + } else if (index == VFIO_PCI_BAR2_REGION_INDEX &&
> + !nvdev->resmem.bar_remap.ioaddr) {
> + nvdev->resmem.bar_remap.ioaddr
> + = ioremap_wc(nvdev->resmem.memphys, nvdev->resmem.memlength);
> + if (!nvdev->resmem.bar_remap.ioaddr) {
> + mutex_unlock(&nvdev->remap_lock);
> + return -ENOMEM;
> + }
> + }
> + mutex_unlock(&nvdev->remap_lock);
> +
> + return 0;
> +}
> +
> +static ssize_t
> +nvgrace_gpu_read_mmio(struct mem_region *region,
> + char __user *buf, size_t count,
> + loff_t offset)
> +{
> + unsigned int read = 0;
> +
> + while (count) {
> + size_t filled;
> +
> + if (count >= 4 && !(offset % 4)) {
> + u32 val;
> +
> + val = ioread32(region->bar_remap.ioaddr + offset);
> + if (copy_to_user(buf, &val, 4))
> + goto read_mmio_exit;
> +
> + filled = 4;
> + } else if (count >= 2 && !(offset % 2)) {
> + u16 val;
> +
> + val = ioread16(region->bar_remap.ioaddr + offset);
> + if (copy_to_user(buf, &val, 2))
> + goto read_mmio_exit;
> +
> + filled = 2;
> + } else {
> + u8 val;
> +
> + val = ioread8(region->bar_remap.ioaddr + offset);
> + if (copy_to_user(buf, &val, 1))
> + goto read_mmio_exit;
> +
> + filled = 1;
> + }
> +
> + count -= filled;
> + read += filled;
> + offset += filled;
> + buf += filled;
> + }
> + return read;
> +
> +read_mmio_exit:
> + return -EFAULT;
> +}
> +
> +/*
> + * Read the data from the device memory (mapped either through ioremap
> + * or memremap) into the user buffer.
> + */
> +static int
> +nvgrace_gpu_map_and_read(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + void __user *buf, size_t mem_count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
> + int ret = 0;
> +
> + /*
> + * Handle read on the BAR regions. Map to the target device memory
> + * physical address and copy to the request read buffer.
> + */
> + ret = nvgrace_gpu_map_device_mem(nvdev, index);
> + if (ret)
> + goto read_exit;
> +
> + if (index == VFIO_PCI_BAR4_REGION_INDEX) {
> + if (copy_to_user(buf, (u8 *)nvdev->usemem.bar_remap.memaddr + offset, mem_count))
> + ret = -EFAULT;
> + } else
> + return nvgrace_gpu_read_mmio(&(nvdev->resmem), buf, mem_count, offset);

If one branch gets braces, they all get braces

Documentation/process/coding-style.rst 3) Placing Braces and Spaces

> +
> +read_exit:
> + return ret;
> +}
> +
> +/*
> + * Read count bytes from the device memory at an offset. The actual device
> + * memory size (available) may not be a power-of-2. So the driver fakes
> + * the size to a power-of-2 (reported) when exposing to a user space driver.
> + *
> + * Read request beyond the actual device size is filled with ~0, while
> + * those beyond the actual reported size is skipped.
> + *
> + * A read from a negative or an offset greater than reported size, a negative
> + * count are considered error conditions and returned with an -EINVAL.
> + */
> +static ssize_t
> +nvgrace_gpu_read_mem(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + void __user *buf, size_t count, loff_t *ppos)
> +{
> + u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + struct mem_region memregion;
> + size_t mem_count, i, bar_size;
> + u8 val = 0xFF;
> + int ret;
> +
> + ret = nvgrace_gpu_vfio_pci_get_mem_region(index, nvdev, &memregion);
> + if (ret)
> + return ret;
> +
> + bar_size = roundup_pow_of_two(memregion.memlength);
> +
> + if (offset >= bar_size)
> + return -EINVAL;
> +
> + /* Clip short the read request beyond reported BAR size */
> + count = min(count, bar_size - (size_t)offset);
> +
> + /*
> + * Determine how many bytes to be actually read from the device memory.
> + * Read request beyond the actual device memory size is filled with ~0,
> + * while those beyond the actual reported size is skipped.
> + */
> + if (offset >= memregion.memlength)
> + mem_count = 0;
> + else
> + mem_count = min(count, memregion.memlength - (size_t)offset);
> +
> + ret = nvgrace_gpu_map_and_read(nvdev, buf, mem_count, ppos);
> + if (ret)
> + return ret;
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. A read to an offset beyond the device memory
> + * size is filled with ~0.
> + */
> + for (i = mem_count; i < count; i++)
> + put_user(val, (unsigned char __user *)(buf + i));
> +
> + *ppos += count;
> + return count;
> +}
> +
> +static ssize_t
> +nvgrace_gpu_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);
> + u64 pos = *ppos & VFIO_PCI_OFFSET_MASK;
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device,
> + core_device.vdev);
> +
> + if (nvgrace_gpu_vfio_pci_is_fake_bar(index))
> + return nvgrace_gpu_read_mem(nvdev, buf, count, ppos);
> +
> + if ((index == VFIO_PCI_CONFIG_REGION_INDEX) &&
> + (range_within_boundary(PCI_BASE_ADDRESS_2,
> + PCI_CARDBUS_CIS, pos, pos + count))) {
> + return nvgrace_gpu_read_config_emu(nvdev, buf, count, ppos);
> + }

Again, follow how Yishai did this, send all config reads to a function
that does a vfio_pci_core_read() then fill in the emulated fields based
on range_intersect_range().

Maybe Yishai & Co could review this as well, it seems overly
complicated. I expect the separate read and write functions could be
combined and we probably need to take into account COMMAND_MEM and
therefore memory_lock as well. It might make sense to export do_io_rw
from the core. Thanks,

Alex

> +
> + return vfio_pci_core_read(core_vdev, buf, count, ppos);
> +}
> +
> +static ssize_t
> +nvgrace_gpu_write_mmio(struct mem_region *region,
> + const char __user *buf, size_t count,
> + loff_t offset)
> +{
> + unsigned int write = 0;
> +
> + while (count) {
> + size_t filled;
> +
> + if (count >= 4 && !(offset % 4)) {
> + u32 val;
> +
> + if (copy_from_user(&val, buf, 4))
> + goto write_mmio_exit;
> + iowrite32(val, region->bar_remap.ioaddr + offset);
> +
> + filled = 4;
> + } else if (count >= 2 && !(offset % 2)) {
> + u16 val;
> +
> + if (copy_from_user(&val, buf, 2))
> + goto write_mmio_exit;
> + iowrite16(val, region->bar_remap.ioaddr + offset);
> +
> + filled = 2;
> + } else {
> + u8 val;
> +
> + if (copy_from_user(&val, buf, 1))
> + goto write_mmio_exit;
> + iowrite8(val, region->bar_remap.ioaddr + offset);
> +
> + filled = 1;
> + }
> +
> + count -= filled;
> + write += filled;
> + offset += filled;
> + buf += filled;
> + }
> +
> + return write;
> +
> +write_mmio_exit:
> + return -EFAULT;
> +}
> +
> +/*
> + * Write the data to the device memory (mapped either through ioremap
> + * or memremap) from the user buffer.
> + */
> +static int
> +nvgrace_gpu_map_and_write(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + const void __user *buf, size_t mem_count, loff_t *ppos)
> +{
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
> + int ret = 0;
> +
> + ret = nvgrace_gpu_map_device_mem(nvdev, index);
> + if (ret)
> + goto write_exit;
> +
> + if (index == VFIO_PCI_BAR4_REGION_INDEX) {
> + if (copy_from_user((u8 *)nvdev->usemem.bar_remap.memaddr + offset, buf, mem_count))
> + return -EFAULT;
> + } else
> + return nvgrace_gpu_write_mmio(&(nvdev->resmem), buf, mem_count, offset);
> +
> +write_exit:
> + return ret;
> +}
> +
> +/*
> + * Write count bytes to the device memory at a given offset. The actual device
> + * memory size (available) may not be a power-of-2. So the driver fakes the
> + * size to a power-of-2 (reported) when exposing to a user space driver.
> + *
> + * Write request beyond the actual device size are dropped, while those
> + * beyond the actual reported size are skipped entirely.
> + *
> + * A write to a negative or an offset greater than the reported size, a
> + * negative count are considered error conditions and returned with an -EINVAL.
> + */
> +static ssize_t
> +nvgrace_gpu_write_mem(struct nvgrace_gpu_vfio_pci_core_device *nvdev,
> + size_t count, loff_t *ppos, const void __user *buf)
> +{
> + u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + struct mem_region memregion;
> + size_t mem_count, bar_size;
> + int ret = 0;
> +
> + ret = nvgrace_gpu_vfio_pci_get_mem_region(index, nvdev, &memregion);
> + if (ret)
> + return ret;
> +
> + bar_size = roundup_pow_of_two(memregion.memlength);
> +
> + if (offset >= bar_size)
> + return -EINVAL;
> +
> + /* Clip short the write request beyond reported BAR size */
> + count = min(count, bar_size - (size_t)offset);
> +
> + /*
> + * Determine how many bytes to be actually written to the device memory.
> + * Do not write to the offset beyond available size.
> + */
> + if (offset >= memregion.memlength)
> + goto exitfn;
> +
> + /*
> + * Only the device memory present on the hardware is mapped, which may
> + * not be power-of-2 aligned. Drop access outside the available device
> + * memory on the hardware.
> + */
> + mem_count = min(count, memregion.memlength - (size_t)offset);
> +
> + ret = nvgrace_gpu_map_and_write(nvdev, buf, mem_count, ppos);
> + if (ret)
> + return ret;
> +
> +exitfn:
> + *ppos += count;
> + return count;
> +}
> +
> +static ssize_t
> +nvgrace_gpu_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);
> + u64 pos = *ppos & VFIO_PCI_OFFSET_MASK;
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
> + core_vdev, struct nvgrace_gpu_vfio_pci_core_device, core_device.vdev);
> +
> + if (nvgrace_gpu_vfio_pci_is_fake_bar(index))
> + return nvgrace_gpu_write_mem(nvdev, count, ppos, buf);
> +
> + if ((index == VFIO_PCI_CONFIG_REGION_INDEX) &&
> + (range_within_boundary(PCI_BASE_ADDRESS_2,
> + PCI_CARDBUS_CIS, pos, pos + count))) {
> + return nvgrace_gpu_write_config_emu(nvdev, buf, count, ppos);
> + }
> +
> + return vfio_pci_core_write(core_vdev, buf, count, ppos);
> +}
> +
> +static const struct vfio_device_ops nvgrace_gpu_vfio_pci_ops = {
> + .name = "nvgrace-gpu-vfio-pci",
> + .init = vfio_pci_core_init_dev,
> + .release = vfio_pci_core_release_dev,
> + .open_device = nvgrace_gpu_vfio_pci_open_device,
> + .close_device = nvgrace_gpu_vfio_pci_close_device,
> + .ioctl = nvgrace_gpu_vfio_pci_ioctl,
> + .read = nvgrace_gpu_vfio_pci_read,
> + .write = nvgrace_gpu_vfio_pci_write,
> + .mmap = nvgrace_gpu_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,
> + .detach_ioas = vfio_iommufd_physical_detach_ioas,
> +};
> +
> +static struct
> +nvgrace_gpu_vfio_pci_core_device *nvgrace_gpu_drvdata(struct pci_dev *pdev)
> +{
> + struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
> +
> + return container_of(core_device, struct nvgrace_gpu_vfio_pci_core_device,
> + core_device);
> +}
> +
> +static int
> +nvgrace_gpu_vfio_pci_fetch_memory_property(struct pci_dev *pdev,
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev)
> +{
> + int ret;
> + u64 memphys, memlength;
> +
> + /*
> + * 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",
> + &(memphys));
> + if (ret)
> + return ret;
> +
> + if (memphys > type_max(phys_addr_t))
> + return -EOVERFLOW;
> +
> + ret = device_property_read_u64(&pdev->dev, "nvidia,gpu-mem-size",
> + &(memlength));
> + if (ret)
> + return ret;
> +
> + if (memlength > type_max(size_t))
> + return -EOVERFLOW;
> +
> + /*
> + * If the C2C link is not up due to an error, the coherent device
> + * memory size is returned as 0. Fail in such case.
> + */
> + if (memlength == 0)
> + return -ENOMEM;
> +
> + /*
> + * The VM GPU device driver needs a non-cacheable region to support
> + * the MIG feature. Since the device memory is mapped as NORMAL cached,
> + * carve out a region from the end with a different NORMAL_NC
> + * property (called as reserved memory and represented as resmem). This
> + * region then is exposed as a 64b BAR (region 2 and 3) to the VM, while
> + * exposing the rest (termed as usable memory and represented using usemem)
> + * as cacheable 64b BAR (region 4 and 5).
> + *
> + * devmem (memlength)
> + * |-------------------------------------------------|
> + * | |
> + * usemem.phys/memphys resmem.phys
> + */
> + nvdev->usemem.memphys = memphys;
> +
> + /*
> + * The device memory exposed to the VM is added to the kernel by the
> + * VM driver module in chunks of memory block size. Only the usable
> + * memory (usemem) is added to the kernel for usage by the VM
> + * workloads. Make the usable memory size memblock aligned.
> + */
> + if (check_sub_overflow(memlength, RESMEM_SIZE,
> + &nvdev->usemem.memlength)) {
> + ret = -EOVERFLOW;
> + goto done;
> + }
> + nvdev->usemem.memlength = round_down(nvdev->usemem.memlength,
> + MEMBLK_SIZE);
> + if ((check_add_overflow(nvdev->usemem.memphys,
> + nvdev->usemem.memlength, &nvdev->resmem.memphys)) ||
> + (check_sub_overflow(memlength, nvdev->usemem.memlength,
> + &nvdev->resmem.memlength))) {
> + ret = -EOVERFLOW;
> + goto done;
> + }
> +
> +done:
> + return ret;
> +}
> +
> +static int nvgrace_gpu_vfio_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev;
> + int ret;
> +
> + nvdev = vfio_alloc_device(nvgrace_gpu_vfio_pci_core_device, core_device.vdev,
> + &pdev->dev, &nvgrace_gpu_vfio_pci_ops);
> + if (IS_ERR(nvdev))
> + return PTR_ERR(nvdev);
> +
> + dev_set_drvdata(&pdev->dev, nvdev);
> +
> + ret = nvgrace_gpu_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 nvgrace_gpu_vfio_pci_remove(struct pci_dev *pdev)
> +{
> + struct nvgrace_gpu_vfio_pci_core_device *nvdev = nvgrace_gpu_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 nvgrace_gpu_vfio_pci_table[] = {
> + /* GH200 120GB */
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2342) },
> + /* GH200 480GB */
> + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, 0x2345) },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, nvgrace_gpu_vfio_pci_table);
> +
> +static struct pci_driver nvgrace_gpu_vfio_pci_driver = {
> + .name = KBUILD_MODNAME,
> + .id_table = nvgrace_gpu_vfio_pci_table,
> + .probe = nvgrace_gpu_vfio_pci_probe,
> + .remove = nvgrace_gpu_vfio_pci_remove,
> + .err_handler = &vfio_pci_core_err_handlers,
> + .driver_managed_dma = true,
> +};
> +
> +module_pci_driver(nvgrace_gpu_vfio_pci_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Ankit Agrawal <ankita@xxxxxxxxxx>");
> +MODULE_AUTHOR("Aniket Agashe <aniketa@xxxxxxxxxx>");
> +MODULE_DESCRIPTION(
> + "VFIO NVGRACE GPU PF - User Level driver for NVIDIA devices with CPU coherently accessible device memory");