Re: [PATCH v5 1/1] vfio/nvgpu: Add vfio pci variant module for grace hopper

[Ankit Agrawal]

>> +static ssize_t nvgrace_gpu_vfio_pci_read(struct vfio_device *core_vdev,
>> +             char __user *buf, size_t count, loff_t *ppos)
>> +{
>> +     unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
>> +     struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
>> +             core_vdev, struct nvgrace_gpu_vfio_pci_core_device, core_device.vdev);
>> +     u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
>> +     u8 val = 0xFF;
>> +     size_t i;
>> +
>> +     /*
>> +      * Only the device memory present on the hardware is mapped, which may
>> +      * not be power-of-2 aligned. A read to the BAR2 region implies an
>> +      * access outside the available device memory on the hardware.
>> +      */
>
> This is not true, userspace has no requirement to only access BAR2 via
> mmap.  This should support reads from within the coherent memory area.

Just to confirm, the ask is to just update the comment to reflect the behavior,
right? (I missed to do that in this posting). Because we do redirect the call to
vfio_pci_core_read() here which will perform the read that is within the device
region. The read response to synthesize -1 is only for the range that is outside
the device memory region.

>> +     if ((index == VFIO_PCI_BAR2_REGION_INDEX) &&
>> +             (offset >= nvdev->mem_length)) {
>> +             for (i = 0; i < count; i++)
>> +                     if (copy_to_user(buf + i, &val, 1))
>> +                             return -EFAULT;
>> +             return count;
>> +     }
>> +
>> +     return vfio_pci_core_read(core_vdev, buf, count, ppos);
>> +
>> +}
>> +
>> +static ssize_t nvgrace_gpu_vfio_pci_write(struct vfio_device *core_vdev,
>> +             const char __user *buf, size_t count, loff_t *ppos)
>> +{
>> +     unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
>> +     struct nvgrace_gpu_vfio_pci_core_device *nvdev = container_of(
>> +             core_vdev, struct nvgrace_gpu_vfio_pci_core_device, core_device.vdev);
>> +     u64 offset = *ppos & VFIO_PCI_OFFSET_MASK;
>> +
>> +     /*
>> +      * Only the device memory present on the hardware is mapped, which may
>> +      * not be power-of-2 aligned. A write to the BAR2 region implies an
>> +      * access outside the available device memory on the hardware.
>> +      */
>
> Likewise this should support writes within the coherent memory area.
> Disabling mmap support in QEMU is useful for tracing device accesses.
> Thanks,
>
> Alex

Same comment as above.

> +     if ((index == VFIO_PCI_BAR2_REGION_INDEX) &&
> +             (offset >= nvdev->mem_length))
> +             return count;
> +
> +     return vfio_pci_core_write(core_vdev, buf, count, ppos);
> +}