Re: [PATCH v3 8/8] vfio/pci: Add the support for PCI D3cold state

[Abhishek Sahu <abhsahu@xxxxxxxxxx>]

On 5/30/2022 5:55 PM, Jason Gunthorpe wrote:
> On Mon, May 30, 2022 at 04:45:59PM +0530, Abhishek Sahu wrote:
> 
>>  1. In real use case, config or any other ioctl should not come along
>>     with VFIO_DEVICE_FEATURE_POWER_MANAGEMENT ioctl request.
>>  
>>  2. Maintain some 'access_count' which will be incremented when we
>>     do any config space access or ioctl.
> 
> Please don't open code locks - if you need a lock then write a proper
> lock. You can use the 'try' variants to bail out in cases where that
> is appropriate.
> 
> Jason

 Thanks Jason for providing your inputs.

 In that case, should I introduce new rw_semaphore (For example
 power_lock) and move ‘platform_pm_engaged’ under ‘power_lock’ ?
 
 I was mainly concerned about locking rules w.r.t. existing
 ‘memory_lock’ and the code present in
 vfio_pci_zap_and_down_write_memory_lock() which is internally taking
 ‘mmap_lock’ and ‘vma_lock’. But from the initial analysis, it seems
 this should not cause any issue since we should not need ‘power_lock’
 in the mmap fault handler or any read/write functions. We can
 maintain following locking order
 
   power_lock => memory_lock
 
 1. At the beginning of config space access or ioctl, we can take the
    lock
 
     down_read(&vdev->power_lock);
     if (vdev->platform_pm_engaged) {
         up_read(&vdev->power_lock);
         return -EIO;
     }
 
    And before returning from config or ioctl, we can release the lock.
 
 2.  Now ‘platform_pm_engaged’ is not protected with memory_lock and we
     need to support the case where VFIO_DEVICE_FEATURE_POWER_MANAGEMENT
     can be called without putting the device into D3hot explicitly.
     So, I need to introduce a second variable which tracks the memory
     disablement (like power_state_d3 in this patch) and will be
     protected with 'memory_lock'. It will be set for both the cases,
     where users change the power state to D3hot by config
     write or user makes this ioctl. Inside vfio_pci_core_feature_pm(), now
     the code will become
    
         down_write(&vdev->power_lock);
         ...
         switch (vfio_pm.low_power_state) {
         case VFIO_DEVICE_LOW_POWER_STATE_ENTER:
                 ...
                         vfio_pci_zap_and_down_write_memory_lock(vdev);
                         vdev->power_state_d3 = true;
                         up_write(&vdev->memory_lock);

         ...
         up_write(&vdev->power_lock);
 
 3.  Inside __vfio_pci_memory_enabled(), we can check
     vdev->power_state_d3 instead of current_state.
 
 4.  For ioctl access, as mentioned previously I need to add two
     callbacks functions (one for start and one for end) in the struct
     vfio_device_ops and call the same at start and end of ioctl from
     vfio_device_fops_unl_ioctl().

 Thanks,
 Abhishek