Re: [PATCH v1 1/1] vfio/nvgrace-gpu: Add Blackwell-Next GPU readiness check via CXL DVSEC

[Ankit Agrawal]

Thanks for the suggestion Alex, comments inline.

>>  
>> +#define CXL_DEVICE_DVSEC_ID          0
>> +#define CXL_DVSEC_RANGE_1_LOW                0x1C
>> +#define CXL_DVSEC_MEMORY_VALID               BIT(0)
>> +#define CXL_DVSEC_MEMORY_ACTIVE              BIT(1)
>
> Looks like pci_regs.h has all these:
>
> #define PCI_DVSEC_CXL_DEVICE                            0
> #define  PCI_DVSEC_CXL_RANGE_SIZE_LOW(i)                (0x1C + (i * 0x10))
> #define   PCI_DVSEC_CXL_MEM_INFO_VALID                  _BITUL(0)
> #define   PCI_DVSEC_CXL_MEM_ACTIVE                      _BITUL(1)
>
> We should search and replace to use the common macros and drop our
> local versions.

Ack, will fix that. Also add the missing MEMORY_ACTIVE_TIMEOUT in the
pci_regs.h from Table 8-13. DVSEC CXL Range 1 Size Low (Offset 1Ch)
This will replace CXL_DVSEC_MEMORY_ACTIVE_TIMEOUT in the patch.

>>  
>> +/*
>> + * Decode the 3-bit Memory_Active_Timeout field from CXL DVSEC Range 1 Low
>
> Is this range 1 or range 0 bits?  The pci_regs.h macro suggests 0.
> Same for the commit log.

This has been a cause of confusion for me. The CXL spec 4.0 has the following
8.1.3.8.2 DVSEC CXL Range 1 Size Low (Offset 1Ch). So that's why I used range 1.

The pci_regs.h does not have any clarification. It does point to the same section.
/* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
#define PCI_DVSEC_CXL_DEVICE                            0
#define  PCI_DVSEC_CXL_CAP                              0xA
#define   PCI_DVSEC_CXL_MEM_CAPABLE                     _BITUL(2)
#define   PCI_DVSEC_CXL_HDM_COUNT                       __GENMASK(5, 4)
#define  PCI_DVSEC_CXL_CTRL                             0xC
#define   PCI_DVSEC_CXL_MEM_ENABLE                      _BITUL(2)
#define  PCI_DVSEC_CXL_RANGE_SIZE_HIGH(i)               (0x18 + (i * 0x10))
#define  PCI_DVSEC_CXL_RANGE_SIZE_LOW(i)                (0x1C + (i * 0x10))

>>        if (!__vfio_pci_memory_enabled(vdev))
>>                return -EIO;
>>  
>> -     ret = nvgrace_gpu_wait_device_ready(vdev->barmap[0]);
>> +     ret = nvdev->wait_device_ready(nvdev, vdev->barmap[0]);
>
> A bit of a nit, but it's a little redundant to have both cxl_dvsec and
> .wait_device_ready on the object.  It also presents the problem that
> each variant has the same prototype, but requires a different
> parameter.  Wouldn't a wrapper be a bit cleaner instead?
>
> static inline int nvgrace_gpu_wait_device_ready(struct nvgrace_gpu_pci_core_device *nvdev,
>                                                 void __iomem *io)
> {
>         return nvdev->cxl_dvsec ?
>                 nvgrace_gpu_wait_device_ready_bw_next(nvdev) :
>                 nvgrace_gpu_wait_device_ready_legacy(io);
> }

Sounds better, will make the change.

>
> Thanks,
> Alex