linux-kernel - Re: [PATCH v17 3/3] vfio/nvgrace-gpu: Add vfio pci variant module for grace hopper

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 8 Feb 2024 06:47:54 +0000
From: Ankit Agrawal <ankita@...dia.com>
To: Alex Williamson <alex.williamson@...hat.com>
CC: Jason Gunthorpe <jgg@...dia.com>, Yishai Hadas <yishaih@...dia.com>,
	"mst@...hat.com" <mst@...hat.com>, "shameerali.kolothum.thodi@...wei.com"
	<shameerali.kolothum.thodi@...wei.com>, "kevin.tian@...el.com"
	<kevin.tian@...el.com>, "clg@...hat.com" <clg@...hat.com>,
	"oleksandr@...alenko.name" <oleksandr@...alenko.name>,
	"satyanarayana.k.v.p@...el.com" <satyanarayana.k.v.p@...el.com>,
	"eric.auger@...hat.com" <eric.auger@...hat.com>, "brett.creeley@....com"
	<brett.creeley@....com>, "horms@...nel.org" <horms@...nel.org>, Rahul
 Rameshbabu <rrameshbabu@...dia.com>, 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 v17 3/3] vfio/nvgrace-gpu: Add vfio pci variant module for
 grace hopper

>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8999497011a2..529ec8966f58 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23103,6 +23103,12 @@ L:   kvm@...r.kernel.org
>>  S:   Maintained
>>  F:   drivers/vfio/platform/
>>
>> +VFIO NVIDIA GRACE GPU DRIVER
>> +M:   Ankit Agrawal <ankita@...dia.com>
>> +L:   kvm@...r.kernel.org
>> +S:   Supported
>> +F:   drivers/vfio/pci/nvgrace-gpu/
>> +
>
> Entries should be alphabetical.  This will end up colliding with [1] so
> I'll plan to fix it either way.

I will make the change to put it at the right place.


> Otherwise just a couple optional comments from me below.  I see Zhi also
> has a few good comments.  I'd suggest soliciting a review from the other
> variant driver reviewers for this version and maybe we can make v18 the
> final version.  Thanks,
>
> Alex
> 
> [1]https://lore.kernel.org/all/20240205235427.2103714-1-alex.williamson@redhat.com/

Great!


>> +static ssize_t
>> +nvgrace_gpu_write_config_emu(struct vfio_device *core_vdev,
>> +                          const char __user *buf, size_t count, loff_t *ppos)
>> +{
>> +     struct nvgrace_gpu_vfio_pci_core_device *nvdev =
>> +             container_of(core_vdev, struct nvgrace_gpu_vfio_pci_core_device,
>> +                          core_device.vdev);
>> +     u64 pos = *ppos & VFIO_PCI_OFFSET_MASK;
>> +     size_t register_offset;
>> +     loff_t copy_offset;
>> +     size_t copy_count;
>> +     struct mem_region *memregion = NULL;
>
> Nit, consistency and reverse Christmas tree variable declaration would
> suggest pushing this up in the list, but it's not strictly required.

Ack, I'll make the change.


>> +     if (index == USEMEM_REGION_INDEX && !memregion->memaddr) {
>> +             memregion->memaddr = memremap(memregion->memphys,
>> +                                           memregion->memlength,
>> +                                           MEMREMAP_WB);
>> +             if (!memregion->memaddr)
>> +                     ret = -ENOMEM;
>> +     } else if (index == RESMEM_REGION_INDEX && !memregion->ioaddr) {
>> +             memregion->ioaddr = ioremap_wc(memregion->memphys,
>> +                                            memregion->memlength);
>> +             if (!memregion->ioaddr)
>> +                     ret = -ENOMEM;
>> +     }
>
> As .memaddr and .ioaddr are a union we can consolidate the NULL test.

Ok, will do that.