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

[Ankit Agrawal]

>> 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.