* [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest @ 2014-01-30 17:24 Bandan Das 2014-01-30 17:33 ` Alex Williamson 0 siblings, 1 reply; 6+ messages in thread From: Bandan Das @ 2014-01-30 17:24 UTC (permalink / raw) To: kvm; +Cc: Alex Williamson Some drivers such as ixgbe rely on pci_vfs_assigned() to prevent disabling sr-iov when vfs are still assigned during hotplug event or module removal. Set and unset PCI_DEV_FLAGS_ASSIGNED appropriately Signed-off-by: Bandan Das <bsd@redhat.com> --- drivers/vfio/pci/vfio_pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 7ba0424..7cc7ed6 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -90,6 +90,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) vdev->has_vga = true; #endif + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; return 0; } @@ -149,6 +150,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) __func__, dev_name(&pdev->dev), ret); } + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; pci_restore_state(pdev); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest 2014-01-30 17:24 [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest Bandan Das @ 2014-01-30 17:33 ` Alex Williamson 2014-01-30 17:36 ` Alex Williamson 0 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2014-01-30 17:33 UTC (permalink / raw) To: Bandan Das; +Cc: kvm On Thu, 2014-01-30 at 22:54 +0530, Bandan Das wrote: > Some drivers such as ixgbe rely on pci_vfs_assigned() to prevent > disabling sr-iov when vfs are still assigned during hotplug > event or module removal. Set and unset PCI_DEV_FLAGS_ASSIGNED > appropriately This flag has always felt like a band-aide for KVM device assignment because a device could be used without an actual driver attached to the device. vfio-pci is an actual driver, so why should it matter whether the device is assigned or in use by ixgbevf or in use by pci-vfio? It seems like sr-iov shouldn't be disabled so long as either a driver or this (needs to be deprecated) flag are set. Thanks, Alex > --- > drivers/vfio/pci/vfio_pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 7ba0424..7cc7ed6 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -90,6 +90,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > vdev->has_vga = true; > #endif > > + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; > return 0; > } > > @@ -149,6 +150,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) > __func__, dev_name(&pdev->dev), ret); > } > > + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; > pci_restore_state(pdev); > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest 2014-01-30 17:33 ` Alex Williamson @ 2014-01-30 17:36 ` Alex Williamson 2014-01-30 18:06 ` Bandan Das 0 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2014-01-30 17:36 UTC (permalink / raw) To: Bandan Das; +Cc: kvm, linux-pci [cc +linux-pci] On Thu, 2014-01-30 at 10:33 -0700, Alex Williamson wrote: > On Thu, 2014-01-30 at 22:54 +0530, Bandan Das wrote: > > Some drivers such as ixgbe rely on pci_vfs_assigned() to prevent > > disabling sr-iov when vfs are still assigned during hotplug > > event or module removal. Set and unset PCI_DEV_FLAGS_ASSIGNED > > appropriately > > This flag has always felt like a band-aide for KVM device assignment > because a device could be used without an actual driver attached to the > device. vfio-pci is an actual driver, so why should it matter whether > the device is assigned or in use by ixgbevf or in use by pci-vfio? It > seems like sr-iov shouldn't be disabled so long as either a driver or > this (needs to be deprecated) flag are set. Thanks, > > Alex > > > --- > > drivers/vfio/pci/vfio_pci.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > > index 7ba0424..7cc7ed6 100644 > > --- a/drivers/vfio/pci/vfio_pci.c > > +++ b/drivers/vfio/pci/vfio_pci.c > > @@ -90,6 +90,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > > vdev->has_vga = true; > > #endif > > > > + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; > > return 0; > > } > > > > @@ -149,6 +150,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) > > __func__, dev_name(&pdev->dev), ret); > > } > > > > + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; > > pci_restore_state(pdev); > > } > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest 2014-01-30 17:36 ` Alex Williamson @ 2014-01-30 18:06 ` Bandan Das 2014-01-30 18:38 ` Alex Williamson 0 siblings, 1 reply; 6+ messages in thread From: Bandan Das @ 2014-01-30 18:06 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, linux-pci Alex Williamson <alex.williamson@redhat.com> writes: > [cc +linux-pci] > > On Thu, 2014-01-30 at 10:33 -0700, Alex Williamson wrote: >> On Thu, 2014-01-30 at 22:54 +0530, Bandan Das wrote: >> > Some drivers such as ixgbe rely on pci_vfs_assigned() to prevent >> > disabling sr-iov when vfs are still assigned during hotplug >> > event or module removal. Set and unset PCI_DEV_FLAGS_ASSIGNED >> > appropriately >> >> This flag has always felt like a band-aide for KVM device assignment >> because a device could be used without an actual driver attached to the >> device. vfio-pci is an actual driver, so why should it matter whether >> the device is assigned or in use by ixgbevf or in use by pci-vfio? It >> seems like sr-iov shouldn't be disabled so long as either a driver or >> this (needs to be deprecated) flag are set. Thanks, With cases such as vfio-pci, not having this flag set will disable sriov while a guest is using it resulting in a hung guest. I am unfamiliar if similar scenarios are applicable to other test cases. If the flag needs to be set, what's a better place then ? Or how does the PF driver decide not to disable sr-iov, should it be based on - as long as the device is bound to a driver ? Bandan >> Alex >> >> > --- >> > drivers/vfio/pci/vfio_pci.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> > index 7ba0424..7cc7ed6 100644 >> > --- a/drivers/vfio/pci/vfio_pci.c >> > +++ b/drivers/vfio/pci/vfio_pci.c >> > @@ -90,6 +90,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) >> > vdev->has_vga = true; >> > #endif >> > >> > + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; >> > return 0; >> > } >> > >> > @@ -149,6 +150,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) >> > __func__, dev_name(&pdev->dev), ret); >> > } >> > >> > + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; >> > pci_restore_state(pdev); >> > } >> > >> >> > > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" 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] 6+ messages in thread
* Re: [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest 2014-01-30 18:06 ` Bandan Das @ 2014-01-30 18:38 ` Alex Williamson 2014-01-30 20:10 ` Bandan Das 0 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2014-01-30 18:38 UTC (permalink / raw) To: Bandan Das; +Cc: kvm, linux-pci On Thu, 2014-01-30 at 23:36 +0530, Bandan Das wrote: > Alex Williamson <alex.williamson@redhat.com> writes: > > > [cc +linux-pci] > > > > On Thu, 2014-01-30 at 10:33 -0700, Alex Williamson wrote: > >> On Thu, 2014-01-30 at 22:54 +0530, Bandan Das wrote: > >> > Some drivers such as ixgbe rely on pci_vfs_assigned() to prevent > >> > disabling sr-iov when vfs are still assigned during hotplug > >> > event or module removal. Set and unset PCI_DEV_FLAGS_ASSIGNED > >> > appropriately > >> > >> This flag has always felt like a band-aide for KVM device assignment > >> because a device could be used without an actual driver attached to the > >> device. vfio-pci is an actual driver, so why should it matter whether > >> the device is assigned or in use by ixgbevf or in use by pci-vfio? It > >> seems like sr-iov shouldn't be disabled so long as either a driver or > >> this (needs to be deprecated) flag are set. Thanks, > > With cases such as vfio-pci, not having this flag set will disable sriov > while a guest is using it resulting in a hung guest. That sounds like a bug, why would a PF disable sr-iov while there's still a driver attached to the VF? Is the PF assuming the driver can reconnect to the VF later or continue w/o a VF? How can it make such an assumption? > I am unfamiliar if > similar scenarios are applicable to other test cases. If the flag needs > to be set, what's a better place then ? Or how does the PF driver decide not > to disable sr-iov, should it be based on - as long as the device is bound to > a driver ? It seems like a better approach would be for drivers that are capable of having the VF removed from under them to set a flag, which would allow the PF driver to be told rather than assume it can disable sr-iov. Then it would be a feature flag rather than some made-up association with device assignment. Thanks, Alex > >> > --- > >> > drivers/vfio/pci/vfio_pci.c | 2 ++ > >> > 1 file changed, 2 insertions(+) > >> > > >> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > >> > index 7ba0424..7cc7ed6 100644 > >> > --- a/drivers/vfio/pci/vfio_pci.c > >> > +++ b/drivers/vfio/pci/vfio_pci.c > >> > @@ -90,6 +90,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > >> > vdev->has_vga = true; > >> > #endif > >> > > >> > + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; > >> > return 0; > >> > } > >> > > >> > @@ -149,6 +150,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) > >> > __func__, dev_name(&pdev->dev), ret); > >> > } > >> > > >> > + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; > >> > pci_restore_state(pdev); > >> > } > >> > > >> > >> > > > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" 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] 6+ messages in thread
* Re: [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest 2014-01-30 18:38 ` Alex Williamson @ 2014-01-30 20:10 ` Bandan Das 0 siblings, 0 replies; 6+ messages in thread From: Bandan Das @ 2014-01-30 20:10 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, linux-pci Alex Williamson <alex.williamson@redhat.com> writes: > On Thu, 2014-01-30 at 23:36 +0530, Bandan Das wrote: >> Alex Williamson <alex.williamson@redhat.com> writes: >> >> > [cc +linux-pci] >> > >> > On Thu, 2014-01-30 at 10:33 -0700, Alex Williamson wrote: >> >> On Thu, 2014-01-30 at 22:54 +0530, Bandan Das wrote: >> >> > Some drivers such as ixgbe rely on pci_vfs_assigned() to prevent >> >> > disabling sr-iov when vfs are still assigned during hotplug >> >> > event or module removal. Set and unset PCI_DEV_FLAGS_ASSIGNED >> >> > appropriately >> >> >> >> This flag has always felt like a band-aide for KVM device assignment >> >> because a device could be used without an actual driver attached to the >> >> device. vfio-pci is an actual driver, so why should it matter whether >> >> the device is assigned or in use by ixgbevf or in use by pci-vfio? It >> >> seems like sr-iov shouldn't be disabled so long as either a driver or >> >> this (needs to be deprecated) flag are set. Thanks, >> >> With cases such as vfio-pci, not having this flag set will disable sriov >> while a guest is using it resulting in a hung guest. > > That sounds like a bug, why would a PF disable sr-iov while there's > still a driver attached to the VF? Is the PF assuming the driver can > reconnect to the VF later or continue w/o a VF? How can it make such an > assumption? It seems (atleast in the ixgbe driver), the decision is based on pci_vfs_assigned which checks the PCI_DEV_FLAGS_ASSIGNED bit i.e whether or not the device is assigned. int ixgbe_disable_sriov(struct ixgbe_adapter *adapter) { ... #ifdef CONFIG_PCI_IOV /* * If our VFs are assigned we cannot shut down SR-IOV * without causing issues, so just leave the hardware * available but disabled */ if (pci_vfs_assigned(adapter->pdev)) { e_dev_warn("Unloading driver while VFs are assigned - VFs will not be deallocated\n"); return -EPERM; } /* disable iov and allow time for transactions to clear */ pci_disable_sriov(adapter->pdev); #endif The only other place I found this bit being used is i40e and xen pci stub. >> I am unfamiliar if >> similar scenarios are applicable to other test cases. If the flag needs >> to be set, what's a better place then ? Or how does the PF driver decide not >> to disable sr-iov, should it be based on - as long as the device is bound to >> a driver ? > > It seems like a better approach would be for drivers that are capable of > having the VF removed from under them to set a flag, which would allow > the PF driver to be told rather than assume it can disable sr-iov. Then > it would be a feature flag rather than some made-up association with > device assignment. Thanks, Sorry but I am a little unclear here. Isn't the PCI_DEV_FLAGS_ASSIGNED bit (maybe renamed) be usable for this case too ? Just setting/clearing that bit during the probe is what is required for the above to work... or am I missing something ? > Alex > >> >> > --- >> >> > drivers/vfio/pci/vfio_pci.c | 2 ++ >> >> > 1 file changed, 2 insertions(+) >> >> > >> >> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> >> > index 7ba0424..7cc7ed6 100644 >> >> > --- a/drivers/vfio/pci/vfio_pci.c >> >> > +++ b/drivers/vfio/pci/vfio_pci.c >> >> > @@ -90,6 +90,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) >> >> > vdev->has_vga = true; >> >> > #endif >> >> > >> >> > + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; >> >> > return 0; >> >> > } >> >> > >> >> > @@ -149,6 +150,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) >> >> > __func__, dev_name(&pdev->dev), ret); >> >> > } >> >> > >> >> > + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; >> >> > pci_restore_state(pdev); >> >> > } >> >> > >> >> >> >> >> > >> > >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe kvm" 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] 6+ messages in thread
end of thread, other threads:[~2014-01-30 20:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-30 17:24 [PATCH] vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest Bandan Das 2014-01-30 17:33 ` Alex Williamson 2014-01-30 17:36 ` Alex Williamson 2014-01-30 18:06 ` Bandan Das 2014-01-30 18:38 ` Alex Williamson 2014-01-30 20:10 ` Bandan Das
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox