On Mon, 15 Nov 2021 19:06:38 +0530
<abhsahu@xxxxxxxxxx> wrote:
> From: Abhishek Sahu <abhsahu@xxxxxxxxxx>
>
> Currently, there is very limited power management support
> available in upstream vfio-pci driver. If there is no user of vfio-pci
> device, then the PCI device will be moved into D3Hot state by writing
> directly into PCI PM registers. This D3Hot state help in saving some
> amount of power but we can achieve zero power consumption if we go
> into D3cold state. The D3cold state cannot be possible with Native PCI
> PM. It requires interaction with platform firmware which is
> system-specific. To go into low power states (including D3cold), the
> runtime PM framework can be used which internally interacts with PCI
> and platform firmware and puts the device into the lowest possible
> D-States.
>
> This patch registers vfio-pci driver with the runtime PM framework.
>
> 1. The PCI core framework takes care of most of the runtime PM
> related things. For enabling the runtime PM, the PCI driver needs to
> decrement the usage count and needs to register the runtime
> suspend/resume routines. For vfio-pci based driver, these routines can
> be stubbed since the vfio-pci driver is not doing the PCI device
> initialization. All the config state saving, and PCI power management
> related things will be done by PCI core framework itself inside its
> runtime suspend/resume callbacks.
>
> 2. To prevent frequent runtime, suspend/resume, it uses autosuspend
> support with a default delay of 1 second.
>
> 3. Inside pci_reset_bus(), all the devices in bus/slot will be moved
> out of D0 state. This state change to D0 can happen directly without
> going through the runtime PM framework. So if runtime PM is enabled,
> then pm_runtime_resume() makes the runtime state active. Since the PCI
> device power state is already D0, so it should return early when it
> tries to change the state with pci_set_power_state(). Then
> pm_request_autosuspend() can be used which will internally check for
> device usage count and will move the device again into low power
> state.
>
> 4. Inside vfio_pci_core_disable(), the device usage count always needs
> to decremented which was incremented vfio_pci_core_enable() with
> pm_runtime_get_sync().
>
> 5. Since the runtime PM framework will provide the same functionality,
> so directly writing into PCI PM config register can be replaced with
> use of runtime PM routines. Also, the use of runtime PM can help us in
> more power saving.
>
> In the systems which do not support D3Cold,
>
> With the existing implementation:
>
> // PCI device
> # cat /sys/bus/pci/devices/0000\:01\:00.0/power_state
> D3hot
> // upstream bridge
> # cat /sys/bus/pci/devices/0000\:00\:01.0/power_state
> D0
>
> With runtime PM:
>
> // PCI device
> # cat /sys/bus/pci/devices/0000\:01\:00.0/power_state
> D3hot
> // upstream bridge
> # cat /sys/bus/pci/devices/0000\:00\:01.0/power_state
> D3hot
>
> So, with runtime PM, the upstream bridge or root port will also go
> into lower power state which is not possible with existing
> implementation.
>
> In the systems which support D3Cold,
>
> // PCI device
> # cat /sys/bus/pci/devices/0000\:01\:00.0/power_state
> D3hot
> // upstream bridge
> # cat /sys/bus/pci/devices/0000\:00\:01.0/power_state
> D0
>
> With runtime PM:
>
> // PCI device
> # cat /sys/bus/pci/devices/0000\:01\:00.0/power_state
> D3cold
> // upstream bridge
> # cat /sys/bus/pci/devices/0000\:00\:01.0/power_state
> D3cold
>
> So, with runtime PM, both the PCI device and upstream bridge will
> go into D3cold state.
>
> 6. If 'disable_idle_d3' module parameter is set, then also the runtime
> PM will be enabled, but in this case, the usage count should not be
> decremented.
>
> 7. vfio_pci_dev_set_try_reset() return value is unused now, so this
> function return type can be changed to void.
>
> Signed-off-by: Abhishek Sahu <abhsahu@xxxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci.c | 3 +
> drivers/vfio/pci/vfio_pci_core.c | 104 +++++++++++++++++++++++--------
> include/linux/vfio_pci_core.h | 4 ++
> 3 files changed, 84 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index a5ce92beb655..c8695baf3b54 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -193,6 +193,9 @@ static struct pci_driver vfio_pci_driver = {
> .remove = vfio_pci_remove,
> .sriov_configure = vfio_pci_sriov_configure,
> .err_handler = &vfio_pci_core_err_handlers,
> +#if defined(CONFIG_PM)
> + .driver.pm = &vfio_pci_core_pm_ops,
> +#endif
> };
>
> static void __init vfio_pci_fill_ids(void)
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index f948e6cd2993..511a52e92b32 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -152,7 +152,7 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
> }
>
> struct vfio_pci_group_info;
> -static bool vfio_pci_dev_set_try_reset(struct vfio_device_set *dev_set);
> +static void vfio_pci_dev_set_try_reset(struct vfio_device_set *dev_set);
> static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
> struct vfio_pci_group_info *groups);
>
> @@ -245,7 +245,8 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> u16 cmd;
> u8 msix_pos;
>
> - vfio_pci_set_power_state(vdev, PCI_D0);
> + if (!disable_idle_d3)
> + pm_runtime_get_sync(&pdev->dev);
I'm not a pm_runtime expert, but why are we not using
pm_runtime_resume_and_get() here and aborting the function on error?
I hope some folks more familiar with pm_runtime can also review usage
across this series.
>
> /* Don't allow our initial saved state to include busmaster */
> pci_clear_master(pdev);
> @@ -405,8 +406,17 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
> out:
> pci_disable_device(pdev);
>
> - if (!vfio_pci_dev_set_try_reset(vdev->vdev.dev_set) && !disable_idle_d3)
> - vfio_pci_set_power_state(vdev, PCI_D3hot);
> + vfio_pci_dev_set_try_reset(vdev->vdev.dev_set);
> +
> + /*
> + * The device usage count always needs to decremented which was
> + * incremented in vfio_pci_core_enable() with
> + * pm_runtime_get_sync().
> + */
*to be
Maybe:
/*
* Put the pm-runtime usage counter acquired during enable; mark
* last use time to delay autosuspend.
*/
> + if (!disable_idle_d3) {
> + pm_runtime_mark_last_busy(&pdev->dev);
> + pm_runtime_put_autosuspend(&pdev->dev);
> + }
> }
> EXPORT_SYMBOL_GPL(vfio_pci_core_disable);
>
> @@ -1847,19 +1857,23 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>
> vfio_pci_probe_power_state(vdev);
>
> - if (!disable_idle_d3) {
> - /*
> - * pci-core sets the device power state to an unknown value at
> - * bootup and after being removed from a driver. The only
> - * transition it allows from this unknown state is to D0, which
> - * typically happens when a driver calls pci_enable_device().
> - * We're not ready to enable the device yet, but we do want to
> - * be able to get to D3. Therefore first do a D0 transition
> - * before going to D3.
> - */
> - vfio_pci_set_power_state(vdev, PCI_D0);
> - vfio_pci_set_power_state(vdev, PCI_D3hot);
> - }
> + /*
> + * pci-core sets the device power state to an unknown value at
> + * bootup and after being removed from a driver. The only
> + * transition it allows from this unknown state is to D0, which
> + * typically happens when a driver calls pci_enable_device().
> + * We're not ready to enable the device yet, but we do want to
> + * be able to get to D3. Therefore first do a D0 transition
> + * before enabling runtime PM.
> + */
> + pci_set_power_state(pdev, PCI_D0);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
Let's #define this 1000 at the top of the file with some rationale how
we arrived at this heuristic (ie. avoid magic numbers in code). Thanks,
Alex
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_mark_last_busy(&pdev->dev);
> + pm_runtime_allow(&pdev->dev);
> +
> + if (!disable_idle_d3)
> + pm_runtime_put_autosuspend(&pdev->dev);
>
> ret = vfio_register_group_dev(&vdev->vdev);
> if (ret)
> @@ -1868,7 +1882,10 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>
> out_power:
> if (!disable_idle_d3)
> - vfio_pci_set_power_state(vdev, PCI_D0);
> + pm_runtime_get_noresume(&pdev->dev);
> +
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> + pm_runtime_forbid(&pdev->dev);
> out_vf:
> vfio_pci_vf_uninit(vdev);
> return ret;
> @@ -1887,7 +1904,10 @@ void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)
> vfio_pci_vga_uninit(vdev);
>
> if (!disable_idle_d3)
> - vfio_pci_set_power_state(vdev, PCI_D0);
> + pm_runtime_get_noresume(&pdev->dev);
> +
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> + pm_runtime_forbid(&pdev->dev);
> }
> EXPORT_SYMBOL_GPL(vfio_pci_core_unregister_device);
>
> @@ -2093,33 +2113,63 @@ static bool vfio_pci_dev_set_needs_reset(struct vfio_device_set *dev_set)
> * - At least one of the affected devices is marked dirty via
> * needs_reset (such as by lack of FLR support)
> * Then attempt to perform that bus or slot reset.
> - * Returns true if the dev_set was reset.
> */
> -static bool vfio_pci_dev_set_try_reset(struct vfio_device_set *dev_set)
> +static void vfio_pci_dev_set_try_reset(struct vfio_device_set *dev_set)
> {
> struct vfio_pci_core_device *cur;
> struct pci_dev *pdev;
> int ret;
>
> if (!vfio_pci_dev_set_needs_reset(dev_set))
> - return false;
> + return;
>
> pdev = vfio_pci_dev_set_resettable(dev_set);
> if (!pdev)
> - return false;
> + return;
>
> ret = pci_reset_bus(pdev);
> if (ret)
> - return false;
> + return;
>
> list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) {
> cur->needs_reset = false;
> - if (!disable_idle_d3)
> - vfio_pci_set_power_state(cur, PCI_D3hot);
> + if (!disable_idle_d3) {
> + /*
> + * Inside pci_reset_bus(), all the devices in bus/slot
> + * will be moved out of D0 state. This state change to
> + * D0 can happen directly without going through the
> + * runtime PM framework. pm_runtime_resume() will
> + * help make the runtime state as active and then
> + * pm_request_autosuspend() can be used which will
> + * internally check for device usage count and will
> + * move the device again into the low power state.
> + */
> + pm_runtime_resume(&pdev->dev);
> + pm_runtime_mark_last_busy(&pdev->dev);
> + pm_request_autosuspend(&pdev->dev);
> + }
> }
> - return true;
> }
>
> +#ifdef CONFIG_PM
> +static int vfio_pci_core_runtime_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int vfio_pci_core_runtime_resume(struct device *dev)
> +{
> + return 0;
> +}
> +
> +const struct dev_pm_ops vfio_pci_core_pm_ops = {
> + SET_RUNTIME_PM_OPS(vfio_pci_core_runtime_suspend,
> + vfio_pci_core_runtime_resume,
> + NULL)
> +};
> +EXPORT_SYMBOL_GPL(vfio_pci_core_pm_ops);
> +#endif
> +
> void vfio_pci_core_set_params(bool is_nointxmask, bool is_disable_vga,
> bool is_disable_idle_d3)
> {
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index ef9a44b6cf5d..aafe09c9fa64 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -231,6 +231,10 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev);
> void vfio_pci_core_disable(struct vfio_pci_core_device *vdev);
> void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev);
>
> +#ifdef CONFIG_PM
> +extern const struct dev_pm_ops vfio_pci_core_pm_ops;
> +#endif
> +
> static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
> {
> return (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
