| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<SA1PR12MB719956FA6354BD78709D6836B0512@SA1PR12MB7199.namprd12.prod.outlook.com>
Date: Mon, 19 Feb 2024 03:48:00 +0000
From: Ankit Agrawal <ankita@...dia.com>
To: Yishai Hadas <yishaih@...dia.com>, Jason Gunthorpe <jgg@...dia.com>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"shameerali.kolothum.thodi@...wei.com"
<shameerali.kolothum.thodi@...wei.com>, "kevin.tian@...el.com"
<kevin.tian@...el.com>, "mst@...hat.com" <mst@...hat.com>,
"eric.auger@...hat.com" <eric.auger@...hat.com>, "jgg@...pe.ca"
<jgg@...pe.ca>, "oleksandr@...alenko.name" <oleksandr@...alenko.name>,
"clg@...hat.com" <clg@...hat.com>, "satyanarayana.k.v.p@...el.com"
<satyanarayana.k.v.p@...el.com>, "brett.creeley@....com"
<brett.creeley@....com>, "horms@...nel.org" <horms@...nel.org>,
"shannon.nelson@....com" <shannon.nelson@....com>
CC: Aniket Agashe <aniketa@...dia.com>, Neo Jia <cjia@...dia.com>, Kirti
Wankhede <kwankhede@...dia.com>, "Tarun Gupta (SW-GPU)"
<targupta@...dia.com>, Vikram Sethi <vsethi@...dia.com>, Andy Currid
<acurrid@...dia.com>, Alistair Popple <apopple@...dia.com>, John Hubbard
<jhubbard@...dia.com>, Dan Williams <danw@...dia.com>, "Anuj Aggarwal
(SW-GPU)" <anuaggarwal@...dia.com>, Matt Ochs <mochs@...dia.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH v18 3/3] vfio/nvgrace-gpu: Add vfio pci variant module for
grace hopper
Thanks Kevin and Yishai for the reviews. Comments inline.
>> +static int nvgrace_gpu_mmap(struct vfio_device *core_vdev,
>> + struct vm_area_struct *vma)
>> +{
>> + struct nvgrace_gpu_pci_core_device *nvdev =
>> + container_of(core_vdev, struct nvgrace_gpu_pci_core_device,
>> + core_device.vdev);
>
> No need for a new line here.
Ack.
>> +static ssize_t
>> +nvgrace_gpu_read_mem(struct nvgrace_gpu_pci_core_device *nvdev,
>> + char __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;
>> + u8 val = 0xFF;
>> + int ret;
>> +
>> + memregion = nvgrace_gpu_memregion(index, nvdev);
>> + if (!memregion)
>
> Can that happen ? it was just tested by the caller.
Ok, I can remove it. Will put a comment instead that this has been checked.
>> + /*
>> + * 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));
>
> Did you condier a failure here ?
Yeah, that has to be checked here. Will make the change in the next post.
>> +/*
>> + * 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
>> + *
>> + * Writes extending beyond the reported size are truncated; writes starting
>> + * beyond the reported size generate -EINVAL.
>> + */
>> +static ssize_t
>> +nvgrace_gpu_write_mem(struct nvgrace_gpu_pci_core_device *nvdev,
>> + size_t count, loff_t *ppos, const char __user *buf)
>> +{
>> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
>> + u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
>> + struct mem_region *memregion;
>> + size_t mem_count;
>> + int ret = 0;
>> +
>> + memregion = nvgrace_gpu_memregion(index, nvdev);
>> + if (!memregion)
>
> Same as the above note in nvgrace_gpu_read_mem().
Ack.
>> +static const struct vfio_device_ops nvgrace_gpu_pci_ops = {
>> + .name = "nvgrace-gpu-vfio-pci",
>> + .init = vfio_pci_core_init_dev,
>> + .release = vfio_pci_core_release_dev,
>> + .open_device = nvgrace_gpu_open_device,
>> + .close_device = nvgrace_gpu_close_device,
>> + .ioctl = nvgrace_gpu_ioctl,
>> + .read = nvgrace_gpu_read,
>> + .write = nvgrace_gpu_write,
>> + .mmap = nvgrace_gpu_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 const struct vfio_device_ops nvgrace_gpu_pci_core_ops = {
>> + .name = "nvgrace-gpu-vfio-pci-core",
>> + .init = vfio_pci_core_init_dev,
>> + .release = vfio_pci_core_release_dev,
>> + .open_device = nvgrace_gpu_open_device,
>> + .close_device = vfio_pci_core_close_device,
>> + .ioctl = vfio_pci_core_ioctl,
>> + .device_feature = vfio_pci_core_ioctl_feature,
>
> This entry is missing above as part of nvgrace_gpu_pci_ops.
Yes. Will add.
>> + .read = vfio_pci_core_read,
>> + .write = vfio_pci_core_write,
>> + .mmap = vfio_pci_core_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_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_pci_core_device,
>> + core_device);
>> +}
>
> The above function is called only once.
> You could just inline its first line (i.e. struct vfio_pci_core_device
> *core_device = dev_get_drvdata(&pdev->dev); and drop it.
True, will fix.
>> +
>> + /*
>> + * The USEMEM part of the device memory has to be MEMBLK_SIZE
>> + * aligned. This is a hardwired ABI value between the GPU FW and
>> + * VFIO driver. The VM device driver is also aware of it and make
>> + * use of the value for its calculation to determine USEMEM size.
>> + */
>> + 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;
>> + }
>> +
>> + if (nvdev->usemem.memlength == 0) {
>> + ret = -EINVAL;
>> + goto done;
>> + }
>> +
>
> Couldn't that check be done earlier in this function ?
Yes, will move it.
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Ankit Agrawal <ankita@...dia.com>");
>> +MODULE_AUTHOR("Aniket Agashe <aniketa@...dia.com>");
>> +MODULE_DESCRIPTION("VFIO NVGRACE GPU PF - User Level driver for NVIDIA devices with CPU coherently accessible device memory");
>
> I'm not in the full details here, however, the construction of the
> variant driver looks OK, so:
>
> Reviewed-by: Yishai Hadas <yishaih@...dia.com>
Thanks.
Powered by blists - more mailing lists