Message-ID: <ad80eb14-18a1-8895-ecfb-32687a4ba021@nvidia.com>
Date:   Fri, 8 Jul 2022 15:09:22 +0530
From:   Abhishek Sahu <abhsahu@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Cornelia Huck <cohuck@...hat.com>,
        Yishai Hadas <yishaih@...dia.com>,
        Jason Gunthorpe <jgg@...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 v4 2/6] vfio: Add a new device feature for the power
 management

On 7/6/2022 9:09 PM, Alex Williamson wrote:
> On Fri, 1 Jul 2022 16:38:10 +0530
> Abhishek Sahu <abhsahu@...dia.com> wrote:
> 
>> This patch adds the new feature VFIO_DEVICE_FEATURE_POWER_MANAGEMENT
>> for the power management in the header file. The implementation for the
>> same will be added in the subsequent patches.
>>
>> With the standard registers, all power states cannot be achieved. The
>> platform-based power management needs to be involved to go into the
>> lowest power state. For all the platform-based power management, this
>> device feature can be used.
>>
>> This device feature uses flags to specify the different operations. In
>> the future, if any more power management functionality is needed then
>> a new flag can be added to it. It supports both GET and SET operations.
>>
>> Signed-off-by: Abhishek Sahu <abhsahu@...dia.com>
>> ---
>>  include/uapi/linux/vfio.h | 55 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 733a1cddde30..7e00de5c21ea 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -986,6 +986,61 @@ enum vfio_device_mig_state {
>>  	VFIO_DEVICE_STATE_RUNNING_P2P = 5,
>>  };
>>  
>> +/*
>> + * Perform power management-related operations for the VFIO device.
>> + *
>> + * The low power feature uses platform-based power management to move the
>> + * device into the low power state.  This low power state is device-specific.
>> + *
>> + * This device feature uses flags to specify the different operations.
>> + * It supports both the GET and SET operations.
>> + *
>> + * - VFIO_PM_LOW_POWER_ENTER flag moves the VFIO device into the low power
>> + *   state with platform-based power management.  This low power state will be
>> + *   internal to the VFIO driver and the user will not come to know which power
>> + *   state is chosen.  Once the user has moved the VFIO device into the low
>> + *   power state, then the user should not do any device access without moving
>> + *   the device out of the low power state.
> 
> Except we're wrapping device accesses to make this possible.  This
> should probably describe how any discrete access will wake the device
> but ongoing access through mmaps will generate user faults.
> 

 Sure. I will add that details also.

>> + *
>> + * - VFIO_PM_LOW_POWER_EXIT flag moves the VFIO device out of the low power
>> + *    state.  This flag should only be set if the user has previously put the
>> + *    device into low power state with the VFIO_PM_LOW_POWER_ENTER flag.
> 
> Indenting.
> 
 
 I will fix this.

>> + *
>> + * - VFIO_PM_LOW_POWER_ENTER and VFIO_PM_LOW_POWER_EXIT are mutually exclusive.
>> + *
>> + * - VFIO_PM_LOW_POWER_REENTERY_DISABLE flag is only valid with
>> + *   VFIO_PM_LOW_POWER_ENTER.  If there is any access for the VFIO device on
>> + *   the host side, then the device will be moved out of the low power state
>> + *   without the user's guest driver involvement.  Some devices require the
>> + *   user's guest driver involvement for each low-power entry.  If this flag is
>> + *   set, then the re-entry to the low power state will be disabled, and the
>> + *   host kernel will not move the device again into the low power state.
>> + *   The VFIO driver internally maintains a list of devices for which low
>> + *   power re-entry is disabled by default and for those devices, the
>> + *   re-entry will be disabled even if the user has not set this flag
>> + *   explicitly.
> 
> Wrong polarity.  The kernel should not maintain the policy.  By default
> every wakeup, whether from host kernel accesses or via user accesses
> that do a pm-get should signal a wakeup to userspace.  Userspace needs
> to opt-out of that wakeup to let the kernel automatically re-enter low
> power and userspace needs to maintain the policy for which devices it
> wants that to occur.
> 
 
 Okay. So that means, in the kernel side, we don’t have to maintain
 the list which currently contains NVIDIA device ID. Also, in our
 updated approach, this opt-out of that wake-up means that user
 has not provided eventfd in the feature SET ioctl. Correct ?
 
>> + *
>> + * For the IOCTL call with VFIO_DEVICE_FEATURE_GET:
>> + *
>> + * - VFIO_PM_LOW_POWER_ENTER will be set if the user has put the device into
>> + *   the low power state, otherwise, VFIO_PM_LOW_POWER_EXIT will be set.
>> + *
>> + * - If the device is in a normal power state currently, then
>> + *   VFIO_PM_LOW_POWER_REENTERY_DISABLE will be set for the devices where low
>> + *   power re-entry is disabled by default.  If the device is in the low power
>> + *   state currently, then VFIO_PM_LOW_POWER_REENTERY_DISABLE will be set
>> + *   according to the current transition.
> 
> Very confusing semantics.
> 
> What if the feature SET ioctl took an eventfd and that eventfd was one
> time use.  Calling the ioctl would setup the eventfd to notify the user
> on wakeup and call pm-put.  Any access to the device via host, ioctl,
> or region would be wrapped in pm-get/put and the pm-resume handler
> would perform the matching pm-get to balance the feature SET and signal
> the eventfd. 

 This seems a better option. It will help in making the ioctl simpler
 and we don’t have to add a separate index for PME which I added in
 patch 6. 

> If the user opts-out by not providing a wakeup eventfd,
> then the pm-resume handler does not perform a pm-get. Possibly we
> could even allow mmap access if a wake-up eventfd is provided.

 Sorry. I am not clear on this mmap part. We currently invalidates
 mapping before going into runtime-suspend. Now, if use tries do
 mmap then do we need some extra handling in the fault handler ?
 Need your help in understanding this part.

> The
> feature GET ioctl would be used to exit low power behavior and would be
> a no-op if the wakeup eventfd had already been signaled.  Thanks,
>
 
 I will use the GET ioctl for low power exit instead of returning the
 current status.
 
 Regards,
 Abhishek

> Alex
> 
>> + */
>> +struct vfio_device_feature_power_management {
>> +	__u32	flags;
>> +#define VFIO_PM_LOW_POWER_ENTER			(1 << 0)
>> +#define VFIO_PM_LOW_POWER_EXIT			(1 << 1)
>> +#define VFIO_PM_LOW_POWER_REENTERY_DISABLE	(1 << 2)
>> +	__u32	reserved;
>> +};
>> +
>> +#define VFIO_DEVICE_FEATURE_POWER_MANAGEMENT	3
>> +
>>  /* -------- API for Type1 VFIO IOMMU -------- */
>>  
>>  /**
> 

Powered by blists - more mailing lists