Thanks Alex, comments inline. > -----Original Message----- > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > Sent: Thursday, April 20, 2023 4:17 PM > To: Ankit Agrawal <ankita@xxxxxxxxxx> > Cc: Jason Gunthorpe <jgg@xxxxxxxxxx>; Aniket Agashe > <aniketa@xxxxxxxxxx>; Neo Jia <cjia@xxxxxxxxxx>; Kirti Wankhede > <kwankhede@xxxxxxxxxx>; Tarun Gupta (SW-GPU) <targupta@xxxxxxxxxx>; > Vikram Sethi <vsethi@xxxxxxxxxx>; Andy Currid <acurrid@xxxxxxxxxx>; > kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v1 1/1] vfio/nvgpu: Add vfio pci variant module for grace > hopper device > > External email: Use caution opening links or attachments > > > On Wed, 19 Apr 2023 15:28:36 -0700 > <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 propritary 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 the VFIO_REGION that covers the first PCI > > BAR. qemu will naturally generate a PCI device in the VM where the > > cacheable aperture is reported in BAR1. > > > > 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(). > > > > 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://github.com/qemu/qemu/compare/master...ankita-nv:qemu:dev- > ankit > > /cohmem-0330 > > > > This patch is split from a patch series being pursued separately: > > https://lore.kernel.org/lkml/20230405180134.16932-2- > ankita@xxxxxxxxxx/ > > > > Applied and tested over v6.3-rc4. > > > > Signed-off-by: Ankit Agrawal <ankita@xxxxxxxxxx> > > --- > > MAINTAINERS | 6 + > > drivers/vfio/pci/Kconfig | 2 + > > drivers/vfio/pci/Makefile | 2 + > > drivers/vfio/pci/nvgpu/Kconfig | 10 ++ > > drivers/vfio/pci/nvgpu/Makefile | 3 + > > drivers/vfio/pci/nvgpu/main.c | 255 > ++++++++++++++++++++++++++++++++ > > 6 files changed, 278 insertions(+) > > 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 > > 1dc8bd26b6cf..6b48756c30d3 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -21954,6 +21954,12 @@ L: kvm@xxxxxxxxxxxxxxx > > S: Maintained > > F: drivers/vfio/pci/mlx5/ > > > > +VFIO NVIDIA PCI DRIVER > > +M: Ankit Agrawal <ankita@xxxxxxxxxx> > > +L: kvm@xxxxxxxxxxxxxxx > > +S: Maintained > > +F: drivers/vfio/pci/nvgpu/ > > + > > VGA_SWITCHEROO > > R: Lukas Wunner <lukas@xxxxxxxxx> > > 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/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..9e08e8cf4606 > > --- /dev/null > > +++ b/drivers/vfio/pci/nvgpu/main.c > > @@ -0,0 +1,255 @@ > > +// 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> > > + > > +#define DUMMY_PFN \ > > + (((nvdev->mem_prop.hpa + nvdev->mem_prop.mem_length) >> > > +PAGE_SHIFT) - 1) > > + > > +struct dev_mem_properties { > > + uint64_t hpa; > > + uint64_t mem_length; > > + int bar1_start_offset; > > +}; > > + > > +struct nvgpu_vfio_pci_core_device { > > + struct vfio_pci_core_device core_device; > > + struct dev_mem_properties mem_prop; }; > > + > > +static int vfio_get_bar1_start_offset(struct vfio_pci_core_device > > +*vdev) { > > + u8 val = 0; > > + > > + pci_read_config_byte(vdev->pdev, 0x10, &val); > > + /* > > + * The BAR1 start offset in the PCI config space depends on the BAR0size. > > + * Check if the BAR0 is 64b and return the approproiate BAR1 offset. > > + */ > > + if (val & PCI_BASE_ADDRESS_MEM_TYPE_64) > > + return VFIO_PCI_BAR2_REGION_INDEX; > > + > > + return VFIO_PCI_BAR1_REGION_INDEX; } > > This is really confusing offsets vs indexes, it's clearly returning a region index, > not offset. Also we already have resources setup for BAR0, so rather than > working on the raw BAR value, how about: > > return pci_resource_flags(vdev->pdev, 0) & IORESOURCE_MEM_64 ? > VFIO_PCI_BAR2_REGION_INDEX : > VFIO_PCI_BAR1_REGION_INDEX; > > OTOH, why are we trying to pack the BARs, couldn't we always put it at BAR2? > Right, it should have said region index. And yes, I think we can put it at index 2. > > + > > +static int nvgpu_vfio_pci_open_device(struct vfio_device *core_vdev) > > +{ > > + struct nvgpu_vfio_pci_core_device *nvdev = container_of( > > + core_vdev, struct nvgpu_vfio_pci_core_device, core_device.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); > > + > > + nvdev->mem_prop.bar1_start_offset = > > + vfio_get_bar1_start_offset(vdev); > > + > > + return ret; > > +} > > + > > +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 != nvdev->mem_prop.bar1_start_offset) > > offset vs index... Ack, will name the variable appropriately at this and other places. > > > + return vfio_pci_core_mmap(core_vdev, vma); > > + > > + /* > > + * Request to mmap the BAR1. Map to the CPU accessible memory on > > + the > > But it might be BAR2... > > > + * 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 BAR1 is backed by the > > + * GPU memory now. Check that the mapping does not overflow out of > > + * the GPU memory size. > > + */ > > + ret = remap_pfn_range(vma, vma->vm_start, start_pfn + pgoff, > > + min(req_len, nvdev->mem_prop.mem_length - pgoff), > > + vma->vm_page_prot); > > What's the behavior of this "BAR" relative to things like > PCI_COMMAND_MEMORY or reset? If the user generates a secondary bus > reset on the parent bridge in one thread, while accessing the device in another > thread, isn't that susceptible to platform error handling just like any other PCI > device? This is why vfio-pci-core has all the zapping and faulting of mmaps to > real BARs. > > > + 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; > > + > > + switch (cmd) { > > + case 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 == nvdev->mem_prop.bar1_start_offset) { > > index vs offset... > > > + /* > > + * Request to determine the BAR1 region information. Send the > > + * GPU memory information. > > + */ > > + info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index); > > + info.size = nvdev->mem_prop.mem_length; > > + info.flags = VFIO_REGION_INFO_FLAG_READ | > > + VFIO_REGION_INFO_FLAG_WRITE | > > + VFIO_REGION_INFO_FLAG_MMAP; > > + return copy_to_user((void __user *)arg, &info, minsz) ? > > + -EFAULT : 0; > > + } > > + > > + if (info.index == nvdev->mem_prop.bar1_start_offset + 1) { > > + /* > > + * The BAR1 region is 64b. Ignore this access. > > + */ > > + info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index); > > + info.size = 0; > > + info.flags = 0; > > + return copy_to_user((void __user *)arg, &info, minsz) ? > > + -EFAULT : 0; > > + } > > Not sure why the core code doesn't handle BAR+1 > > > + > > + return vfio_pci_core_ioctl(core_vdev, cmd, arg); > > + > > + default: > > + return vfio_pci_core_ioctl(core_vdev, cmd, arg); > > + } > > This might work better as simply: > > if (cmd == VFIO_DEVICE_GET_REGION_INFO) { > /* virtual BAR returns... */ > } > > return vfio_pci_core_ioctl(...); > > It at least avoids the duplication. > Right, that looks better. > > +} > > + > > +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 = vfio_pci_core_read, > > + .write = vfio_pci_core_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 = 0; > > Unnecessary initialization. Thanks Ack, will fix in the next version. > > Alex > > > + > > + /* > > + * 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 <ankita@xxxxxxxxxx>"); > > +MODULE_AUTHOR("Aniket Agashe <aniketa@xxxxxxxxxx>"); > > +MODULE_DESCRIPTION( > > + "VFIO NVGPU PF - User Level driver for NVIDIA devices with CPU > coherently accessible device memory");
