Date:   Wed, 27 Jul 2022 11:37:02 +0530
From:   Abhishek Sahu <abhsahu@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Alex Williamson <alex.williamson@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Yishai Hadas <yishaih@...dia.com>,
        Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>,
        Kevin Tian <kevin.tian@...el.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Max Gurtovoy <mgurtovoy@...dia.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v5 1/5] vfio: Add the device features for the low power
 entry and exit

On 7/26/2022 10:53 PM, Jason Gunthorpe wrote:
> On Tue, Jul 26, 2022 at 06:17:18PM +0530, Abhishek Sahu wrote:
>>  Thanks Alex for your thorough review of uAPI.
>>  I have incorporated all the suggestions.
>>  Following is the updated uAPI.
>>  
>>  /*
>>   * Upon VFIO_DEVICE_FEATURE_SET, allow the device to be moved into a low power
>>   * state with the platform-based power management.  Device use of lower power
>>   * states depends on factors managed by the runtime power management core,
>>   * including system level support and coordinating support among dependent
>>   * devices.  Enabling device low power entry does not guarantee lower power
>>   * usage by the device, nor is a mechanism provided through this feature to
>>   * know the current power state of the device.  If any device access happens
>>   * (either from the host or through the vfio uAPI) when the device is in the
>>   * low power state, then the host will move the device out of the low power
>>   * state as necessary prior to the access.  Once the access is completed, the
>>   * device may re-enter the low power state.  For single shot low power support
>>   * with wake-up notification, see
>>   * VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY_WITH_WAKEUP below.  Access to mmap'd
>>   * device regions is disabled on LOW_POWER_ENTRY and may only be resumed after
>>   * calling LOW_POWER_EXIT.
>>   */
>>  #define VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY 3
>>  
>>  /*
>>   * This device feature has the same behavior as
>>   * VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY with the exception that the user
>>   * provides an eventfd for wake-up notification.
> 
> It feels like this should be one entry point instead of two.
> 
> A flag "automatic re-sleep" and an optional eventfd (-1 means not
> provided) seems to capture both of these behaviors in a bit clearer
> and extendable way.
> 
> Jason

 We discussed about that in the earlier version of the patch series.
 Since we have different exit related handling, so to avoid confusion
 we proceeded with 2 separate variants for the low power entry. Also,
 we don't need any parameter for the first case.

 But, I can do the changes to make a single entry point, if we conclude
 for that. 

 From my side, I have explored how the uAPI looks like if
 we go with this approach.

 /*
  * Upon VFIO_DEVICE_FEATURE_SET, allow the device to be moved into a low power
  * state with the platform-based power management.  Device use of lower power
  * states depends on factors managed by the runtime power management core,
  * including system level support and coordinating support among dependent
  * devices.  Enabling device low power entry does not guarantee lower power
  * usage by the device, nor is a mechanism provided through this feature to
  * know the current power state of the device.  If any device access happens
  * (either from the host or through the vfio uAPI) when the device is in the
  * low power state, then the host will move the device out of the low power
  * state as necessary prior to the access.  Once the access is completed, the
  * device re-entry to a low power state will be controlled through
  * VFIO_DEVICE_LOW_POWER_REENTERY_DISABLE flag.
  *
  * If LOW_POWER_REENTERY_DISABLE flag is not set, the device may re-enter the
  * low power state.  Access to mmap'd device regions is disabled on
  * LOW_POWER_ENTRY and may only be resumed after calling LOW_POWER_EXIT.
  *
  * If LOW_POWER_REENTERY_DISABLE flag is set, then user needs to provide an
  * eventfd for wake-up notification.  When the device moves out of the low
  * power state for the wake-up, the host will not allow the device to re-enter
  * a low power state without a subsequent user call to LOW_POWER_ENTRY.
  * Access to mmap'd device regions is disabled on LOW_POWER_ENTRY and may only
  * be resumed after the low power exit.  The low power exit can happen either
  * through LOW_POWER_EXIT or through any other access (where the wake-up
  * notification has been generated).  The access to mmap'd device regions will
  * not trigger low power exit.
  *
  * The notification through the provided eventfd will be generated only when
  * the device has entered and is resumed from a low power state after
  * calling this device feature IOCTL.  A device that has not entered low power
  * state, as managed through the runtime power management core, will not
  * generate a notification through the provided eventfd on access.  Calling the
  * LOW_POWER_EXIT feature is optional in the case where notification has been
  * signaled on the provided eventfd that a resume from low power has occurred.
  *
  * The wakeup_eventfd needs to be valid only if LOW_POWER_REENTERY_DISABLE
  * flag is set, otherwise, it will be ignored.
  */
 #define VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY 3
 
 struct vfio_device_low_power_entry_with_wakeup {
 	__u32 flags;
 #define VFIO_DEVICE_LOW_POWER_REENTERY_DISABLE	(1 << 0)
 	__s32 wakeup_eventfd;
 };
 
 /*
  * Upon VFIO_DEVICE_FEATURE_SET, disallow use of device low power states as
  * previously enabled via VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY device feature.
  * This device feature IOCTL may itself generate a wakeup eventfd notification
  * if the device had previously entered a low power state with
  * VFIO_DEVICE_LOW_POWER_REENTERY_DISABLE flag set.
  */
 #define VFIO_DEVICE_FEATURE_LOW_POWER_EXIT 4

 Thanks,
 Abhishek

Powered by blists - more mailing lists