Re: [PATCH v4 3/3] vfio/nvgrace-gpu: Check the HBM training and C2C link status

[Alex Williamson <alex.williamson@redhat.com>]

On Sun, 19 Jan 2025 20:12:32 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 20 Jan 2025 02:24:14 +0000
> Ankit Agrawal <ankita@nvidia.com> wrote:
> 
> > >> +EXPORT_SYMBOL_GPL(vfio_pci_memory_lock_and_enable);
> > >>
> > >>  void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev, u16 cmd)
> > >>  {
> > >>       pci_write_config_word(vdev->pdev, PCI_COMMAND, cmd);
> > >>       up_write(&vdev->memory_lock);
> > >>  }
> > >> +EXPORT_SYMBOL_GPL(vfio_pci_memory_unlock_and_restore);
> > >>
> > >>  static unsigned long vma_to_pfn(struct vm_area_struct *vma)
> > >>  {    
> > >
> > > The access is happening before the device is exposed to the user, the
> > > above are for handling conditions while there may be races with user
> > > access, this is totally unnecessary.    
> > 
> > Right. What I could do to reuse the code is to take out the part
> > related to locking/unlocking as new functions and export that.
> > The current vfio_pci_memory_lock_and_enable() would take the lock
> > and call the new function. Same for vfio_pci_memory_unlock_and_restore().
> > The nvgrace module could also call that new function. Does that sound
> > reasonable?  
> 
> No, this is standard PCI driver stuff, everything you need is already
> there.  Probably pci_enable_device() and some variant of
> pci_request_regions().
> 
> > > Does this delay even need to happen in the probe function, or could it
> > > happen in the open_device callback?  That would still be before user
> > > access, but if we expect it to generally work, it would allow the
> > > training to happen in the background up until the user tries to open
> > > the device.  Thanks,
> > >
> > > Alex    
> > 
> > The thought process is that since it is purely bare metal coming to proper
> > state while boot, the nvgrace module should probably wait for the startup
> > to complete during probe() instead of delaying until open() time.  
> 
> If the driver is statically loaded, that might mean you're willing to
> stall boot for up to 30s.  In practice is this ever actually going to
> fail?  Thanks,

On second thought, I guess a vfio-pci variant driver can't
automatically bind to a device, whether statically built or not, so
maybe this isn't a concern.  I'm not sure if there are other concerns
with busy waiting for up to 30s at driver probe.  Thanks,

Alex