Re: [PATCH v4 2/6] vfio: Add a new device feature for the power management

From: Alex Williamson <alex.williamson@redhat.com>
To: Abhishek Sahu <abhsahu@nvidia.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 2/6] vfio: Add a new device feature for the power management
Date: Wed, 6 Jul 2022 09:39:59 -0600	[thread overview]
Message-ID: <20220706093959.3bd2cbbb.alex.williamson@redhat.com> (raw)
In-Reply-To: <20220701110814.7310-3-abhsahu@nvidia.com>

On Fri, 1 Jul 2022 16:38:10 +0530
Abhishek Sahu <abhsahu@nvidia.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@nvidia.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.

> + *
> + * - 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.

> + *
> + * - 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.

> + *
> + * 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.  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.  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,

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 -------- */
>  
>  /**