Received-SPF: pass (google.com: domain of linux-kernel+bounces-107675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 19 Mar 2024 08:50:11 -0600
From: Alex Williamson <alex.williamson@redhat.com>
To: Mikhail Malyshev <mike.malyshev@gmail.com>
Cc: jgg@ziepe.ca, yi.l.liu@intel.com, kevin.tian@intel.com,
 tglx@linutronix.de, reinette.chatre@intel.com, stefanha@redhat.com,
 abhsahu@nvidia.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/1] Reenable runtime PM for dynamically unbound devices
Message-ID: <20240319085011.2da113ae.alex.williamson@redhat.com>
In-Reply-To: <20240319120410.1477713-1-mike.malyshev@gmail.com>
References: <20240319120410.1477713-1-mike.malyshev@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 19 Mar 2024 12:04:09 +0000
Mikhail Malyshev <mike.malyshev@gmail.com> wrote:

> When trying to run a VM with PCI passthrough of intel-eth-pci ETH device
> QEMU fails with "Permission denied" error. This happens only if
> intel-eth-pci driver is dynamically unbound from the device using
> "echo -n $DEV > /sys/bus/pci/drivers/stmmac/unbind" command. If
> "vfio-pci.ids=..." is used to bind the device to vfio-pci driver and the
> device is never probed by intel-eth-pci driver the problem does not occur.
> 
> When intel-eth-pci driver is dynamically unbound from the device
> .remove()
>   intel_eth_pci_remove()
>     stmmac_dvr_remove()
>       pm_runtime_disable();

Why isn't the issue in intel-eth-pci?

For example stmmac_dvr_remove() does indeed call pm_runtime_disable()
unconditionally, but stmmac_dvr_probe() only conditionally calls
pm_runtime_enable() with logic like proposed here for vfio-pci.  Isn't
it this conditional enabling which causes an unbalanced disable depth
that's the core of the problem?

It doesn't seem like it should be the responsibility of the next driver
to correct the state from the previous driver.  You've indicated that
the device works with vfio-pci if there's no previous driver, so
clearly intel-eth-pci isn't leaving the device in the same runtime pm
state that it found it.  Thanks,

Alex

> Later when QEMU tries to get the device file descriptor by calling
> VFIO_GROUP_GET_DEVICE_FD ioctl pm_runtime_resume_and_get returns -EACCES.
> It happens because dev->power.disable_depth == 1 .
> 
> vfio_group_fops_unl_ioctl(VFIO_GROUP_GET_DEVICE_FD)
>   vfio_group_ioctl_get_device_fd()
>     vfio_device_open()
>       ret = device->ops->open_device()
>         vfio_pci_open_device()
>           vfio_pci_core_enable()
>               ret = pm_runtime_resume_and_get();
> 
> This behavior was introduced by
> commit 7ab5e10eda02 ("vfio/pci: Move the unused device into low power state with runtime PM")
> 
> This may be the case for any driver calling pm_runtime_disable() in its
> .remove() callback.
> 
> The case when a runtime PM may be disable for a device is not handled so we
> call pm_runtime_enable() in vfio_pci_core_register_device to re-enable it.
> 
> Mikhail Malyshev (1):
>   vfio/pci: Reenable runtime PM for dynamically unbound devices
> 
>  drivers/vfio/pci/vfio_pci_core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> --
> 2.34.1
>