Re: [PATCH v17 3/3] vfio/nvgrace-gpu: Add vfio pci variant module for grace hopper
From: Ankit Agrawal
Date: Thu Feb 08 2024 - 01:48:13 EST
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8999497011a2..529ec8966f58 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23103,6 +23103,12 @@ L: kvm@xxxxxxxxxxxxxxx
>> S: Maintained
>> F: drivers/vfio/platform/
>>
>> +VFIO NVIDIA GRACE GPU DRIVER
>> +M: Ankit Agrawal <ankita@xxxxxxxxxx>
>> +L: kvm@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxx/
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.