Re: [PATCH v4 3/6] vfio: Increment the runtime PM usage count during IOCTL call

From: Abhishek Sahu <abhsahu@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>,
	Yishai Hadas <yishaih@nvidia.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>,
	Kevin Tian <kevin.tian@intel.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v4 3/6] vfio: Increment the runtime PM usage count during IOCTL call
Date: Mon, 11 Jul 2022 15:20:42 +0530	[thread overview]
Message-ID: <5dc17961-584a-1ac5-5742-6f7709876eb4@nvidia.com> (raw)
In-Reply-To: <20220708104900.1780b8d7.alex.williamson@redhat.com>

On 7/8/2022 10:19 PM, Alex Williamson wrote:
> On Fri, 8 Jul 2022 15:13:16 +0530
> Abhishek Sahu <abhsahu@nvidia.com> wrote:
> 
>> On 7/6/2022 9:10 PM, Alex Williamson wrote:
>>> On Fri, 1 Jul 2022 16:38:11 +0530
>>> Abhishek Sahu <abhsahu@nvidia.com> wrote:
>>>   
>>>> The vfio-pci based driver will have runtime power management
>>>> support where the user can put the device into the low power state
>>>> and then PCI devices can go into the D3cold state. If the device is
>>>> in the low power state and the user issues any IOCTL, then the
>>>> device should be moved out of the low power state first. Once
>>>> the IOCTL is serviced, then it can go into the low power state again.
>>>> The runtime PM framework manages this with help of usage count.
>>>>
>>>> One option was to add the runtime PM related API's inside vfio-pci
>>>> driver but some IOCTL (like VFIO_DEVICE_FEATURE) can follow a
>>>> different path and more IOCTL can be added in the future. Also, the
>>>> runtime PM will be added for vfio-pci based drivers variant currently,
>>>> but the other VFIO based drivers can use the same in the
>>>> future. So, this patch adds the runtime calls runtime-related API in
>>>> the top-level IOCTL function itself.
>>>>
>>>> For the VFIO drivers which do not have runtime power management
>>>> support currently, the runtime PM API's won't be invoked. Only for
>>>> vfio-pci based drivers currently, the runtime PM API's will be invoked
>>>> to increment and decrement the usage count.  
>>>
>>> Variant drivers can easily opt-out of runtime pm support by performing
>>> a gratuitous pm-get in their device-open function.
>>>    
>>
>>  Do I need to add this line in the commit message?
> 
> Maybe I misinterpreted, but my initial read was that there was some
> sort of opt-in, which there is by providing pm-runtime support in the
> driver, which vfio-pci-core does for all vfio-pci variant drivers.  But
> there's also an opt-out, where a vfio-pci variant driver might not want
> to support pm-runtime support and could accomplish that by bumping the
> pm reference count on device-open such that the user cannot trigger a
> pm-suspend.
> 
>>>> Taking this usage count incremented while servicing IOCTL will make
>>>> sure that the user won't put the device into low power state when any
>>>> other IOCTL is being serviced in parallel. Let's consider the
>>>> following scenario:
>>>>
>>>>  1. Some other IOCTL is called.
>>>>  2. The user has opened another device instance and called the power
>>>>     management IOCTL for the low power entry.
>>>>  3. The power management IOCTL moves the device into the low power state.
>>>>  4. The other IOCTL finishes.
>>>>
>>>> If we don't keep the usage count incremented then the device
>>>> access will happen between step 3 and 4 while the device has already
>>>> gone into the low power state.
>>>>
>>>> The runtime PM API's should not be invoked for
>>>> VFIO_DEVICE_FEATURE_POWER_MANAGEMENT since this IOCTL itself performs
>>>> the runtime power management entry and exit for the VFIO device.  
>>>
>>> I think the one-shot interface I proposed in the previous patch avoids
>>> the need for special handling for these feature ioctls.  Thanks,
>>>   
>>
>>  Okay. So, for low power exit case (means feature GET ioctl in the
>>  updated case) also, we will trigger eventfd. Correct?
> 
> If all ioctls are wrapped in pm-get/put, then the pm feature exit ioctl
> would wakeup and signal the eventfd via the pm-get.  I'm not sure if
> it's worthwhile to try to surprise this eventfd.  Do you foresee any
> issues?  Thanks,
> 
> Alex
> 

 I think it should be fine. It requires some extra handling in the
 hypervisor or QEMU side where it needs to skip the virtual PME or
 similar notification handling for the cases when the guest has
 actually triggered the low power exit but that can be easily done.

 Regards,
 Abhishek