* [PATCH] vfio-pci: Use pci "try" reset interface
@ 2013-12-16 22:16 Alex Williamson
2014-01-14 23:48 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: Alex Williamson @ 2013-12-16 22:16 UTC (permalink / raw)
To: alex.williamson; +Cc: linux-pci, linux-kernel, kvm
PCI resets will attempt to take the device_lock for any device to be
reset. This is a problem if that lock is already held, for instance
in the device remove path. It's not sufficient to simply kill the
user process or skip the reset if called after .remove as a race could
result in the same deadlock. Instead, we handle all resets as "best
effort" using the PCI "try" reset interfaces. This prevents the user
from being able to induce a deadlock by triggering a reset.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
This depends on the proposed pci "try" function/slot/bus reset patch.
drivers/vfio/pci/vfio_pci.c | 29 +++++++++--------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 6ab71b9..576e34e 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -139,25 +139,14 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
pci_write_config_word(pdev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
/*
- * Careful, device_lock may already be held. This is the case if
- * a driver unbind is blocked. Try to get the locks ourselves to
- * prevent a deadlock.
+ * Try to reset the device. The success of this is dependent on
+ * being able to lock the device, which is not always possible.
*/
if (vdev->reset_works) {
- bool reset_done = false;
-
- if (pci_cfg_access_trylock(pdev)) {
- if (device_trylock(&pdev->dev)) {
- __pci_reset_function_locked(pdev);
- reset_done = true;
- device_unlock(&pdev->dev);
- }
- pci_cfg_access_unlock(pdev);
- }
-
- if (!reset_done)
- pr_warn("%s: Unable to acquire locks for reset of %s\n",
- __func__, dev_name(&pdev->dev));
+ int ret = pci_try_reset_function(pdev);
+ if (ret)
+ pr_debug("%s: reset of device %s = %d\n",
+ __func__, dev_name(&pdev->dev), ret);
}
pci_restore_state(pdev);
@@ -514,7 +503,7 @@ static long vfio_pci_ioctl(void *device_data,
} else if (cmd == VFIO_DEVICE_RESET) {
return vdev->reset_works ?
- pci_reset_function(vdev->pdev) : -EINVAL;
+ pci_try_reset_function(vdev->pdev) : -EINVAL;
} else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
struct vfio_pci_hot_reset_info hdr;
@@ -684,8 +673,8 @@ reset_info_exit:
&info, slot);
if (!ret)
/* User has access, do the reset */
- ret = slot ? pci_reset_slot(vdev->pdev->slot) :
- pci_reset_bus(vdev->pdev->bus);
+ ret = slot ? pci_try_reset_slot(vdev->pdev->slot) :
+ pci_try_reset_bus(vdev->pdev->bus);
hot_reset_release:
for (i--; i >= 0; i--)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] vfio-pci: Use pci "try" reset interface
2013-12-16 22:16 [PATCH] vfio-pci: Use pci "try" reset interface Alex Williamson
@ 2014-01-14 23:48 ` Bjorn Helgaas
2014-01-15 3:19 ` Alex Williamson
0 siblings, 1 reply; 3+ messages in thread
From: Bjorn Helgaas @ 2014-01-14 23:48 UTC (permalink / raw)
To: Alex Williamson
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
On Mon, Dec 16, 2013 at 3:16 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> PCI resets will attempt to take the device_lock for any device to be
> reset. This is a problem if that lock is already held, for instance
> in the device remove path. It's not sufficient to simply kill the
> user process or skip the reset if called after .remove as a race could
> result in the same deadlock. Instead, we handle all resets as "best
> effort" using the PCI "try" reset interfaces. This prevents the user
> from being able to induce a deadlock by triggering a reset.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> This depends on the proposed pci "try" function/slot/bus reset patch.
>
> drivers/vfio/pci/vfio_pci.c | 29 +++++++++--------------------
> 1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..576e34e 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -139,25 +139,14 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> pci_write_config_word(pdev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
>
> /*
> - * Careful, device_lock may already be held. This is the case if
> - * a driver unbind is blocked. Try to get the locks ourselves to
> - * prevent a deadlock.
> + * Try to reset the device. The success of this is dependent on
> + * being able to lock the device, which is not always possible.
> */
> if (vdev->reset_works) {
> - bool reset_done = false;
> -
> - if (pci_cfg_access_trylock(pdev)) {
> - if (device_trylock(&pdev->dev)) {
> - __pci_reset_function_locked(pdev);
> - reset_done = true;
> - device_unlock(&pdev->dev);
> - }
> - pci_cfg_access_unlock(pdev);
> - }
> -
> - if (!reset_done)
> - pr_warn("%s: Unable to acquire locks for reset of %s\n",
> - __func__, dev_name(&pdev->dev));
> + int ret = pci_try_reset_function(pdev);
> + if (ret)
> + pr_debug("%s: reset of device %s = %d\n",
> + __func__, dev_name(&pdev->dev), ret);
Why is this a pr_debug() instead of dev_dbg()? I see vfio_pci.c seems
to always use pr_*() instead of dev_*(), so you probably have a
reason.
The text of the message ("vfio_pci_disable: reset of device
0000:00:00.1 = -35") doesn't seem terribly clear; as a user, I'd have
to go look up the code to know whether I should be concerned about it.
Maybe it should be explicit that the device was not reset?
> }
>
> pci_restore_state(pdev);
> @@ -514,7 +503,7 @@ static long vfio_pci_ioctl(void *device_data,
>
> } else if (cmd == VFIO_DEVICE_RESET) {
> return vdev->reset_works ?
> - pci_reset_function(vdev->pdev) : -EINVAL;
> + pci_try_reset_function(vdev->pdev) : -EINVAL;
>
> } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
> struct vfio_pci_hot_reset_info hdr;
> @@ -684,8 +673,8 @@ reset_info_exit:
> &info, slot);
> if (!ret)
> /* User has access, do the reset */
> - ret = slot ? pci_reset_slot(vdev->pdev->slot) :
> - pci_reset_bus(vdev->pdev->bus);
> + ret = slot ? pci_try_reset_slot(vdev->pdev->slot) :
> + pci_try_reset_bus(vdev->pdev->bus);
>
> hot_reset_release:
> for (i--; i >= 0; i--)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] vfio-pci: Use pci "try" reset interface
2014-01-14 23:48 ` Bjorn Helgaas
@ 2014-01-15 3:19 ` Alex Williamson
0 siblings, 0 replies; 3+ messages in thread
From: Alex Williamson @ 2014-01-15 3:19 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
On Tue, 2014-01-14 at 16:48 -0700, Bjorn Helgaas wrote:
> On Mon, Dec 16, 2013 at 3:16 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > PCI resets will attempt to take the device_lock for any device to be
> > reset. This is a problem if that lock is already held, for instance
> > in the device remove path. It's not sufficient to simply kill the
> > user process or skip the reset if called after .remove as a race could
> > result in the same deadlock. Instead, we handle all resets as "best
> > effort" using the PCI "try" reset interfaces. This prevents the user
> > from being able to induce a deadlock by triggering a reset.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> > This depends on the proposed pci "try" function/slot/bus reset patch.
> >
> > drivers/vfio/pci/vfio_pci.c | 29 +++++++++--------------------
> > 1 file changed, 9 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 6ab71b9..576e34e 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -139,25 +139,14 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> > pci_write_config_word(pdev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
> >
> > /*
> > - * Careful, device_lock may already be held. This is the case if
> > - * a driver unbind is blocked. Try to get the locks ourselves to
> > - * prevent a deadlock.
> > + * Try to reset the device. The success of this is dependent on
> > + * being able to lock the device, which is not always possible.
> > */
> > if (vdev->reset_works) {
> > - bool reset_done = false;
> > -
> > - if (pci_cfg_access_trylock(pdev)) {
> > - if (device_trylock(&pdev->dev)) {
> > - __pci_reset_function_locked(pdev);
> > - reset_done = true;
> > - device_unlock(&pdev->dev);
> > - }
> > - pci_cfg_access_unlock(pdev);
> > - }
> > -
> > - if (!reset_done)
> > - pr_warn("%s: Unable to acquire locks for reset of %s\n",
> > - __func__, dev_name(&pdev->dev));
> > + int ret = pci_try_reset_function(pdev);
> > + if (ret)
> > + pr_debug("%s: reset of device %s = %d\n",
> > + __func__, dev_name(&pdev->dev), ret);
>
> Why is this a pr_debug() instead of dev_dbg()? I see vfio_pci.c seems
> to always use pr_*() instead of dev_*(), so you probably have a
> reason.
Inertia and lack of anyone suggesting it are probably the biggest
reasons. As you note, I've been using pr_foo throughout the file, so
I'd probably prefer consistency and fix them in a separate patch.
> The text of the message ("vfio_pci_disable: reset of device
> 0000:00:00.1 = -35") doesn't seem terribly clear; as a user, I'd have
> to go look up the code to know whether I should be concerned about it.
> Maybe it should be explicit that the device was not reset?
Sure, I'll tweak the message to make it more clear that it failed and
include the return value as extra info. Thanks,
Alex
> > }
> >
> > pci_restore_state(pdev);
> > @@ -514,7 +503,7 @@ static long vfio_pci_ioctl(void *device_data,
> >
> > } else if (cmd == VFIO_DEVICE_RESET) {
> > return vdev->reset_works ?
> > - pci_reset_function(vdev->pdev) : -EINVAL;
> > + pci_try_reset_function(vdev->pdev) : -EINVAL;
> >
> > } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
> > struct vfio_pci_hot_reset_info hdr;
> > @@ -684,8 +673,8 @@ reset_info_exit:
> > &info, slot);
> > if (!ret)
> > /* User has access, do the reset */
> > - ret = slot ? pci_reset_slot(vdev->pdev->slot) :
> > - pci_reset_bus(vdev->pdev->bus);
> > + ret = slot ? pci_try_reset_slot(vdev->pdev->slot) :
> > + pci_try_reset_bus(vdev->pdev->bus);
> >
> > hot_reset_release:
> > for (i--; i >= 0; i--)
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-01-15 3:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-16 22:16 [PATCH] vfio-pci: Use pci "try" reset interface Alex Williamson
2014-01-14 23:48 ` Bjorn Helgaas
2014-01-15 3:19 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).