vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest

PATCH In review Bandan Das 2 participants 6 messages Jan 30, 2014

Subsystems (3)

KVM PCI VFIO driver

lore

Share & tools

lore

Bandan Das

author

2014-01-30 17:24 UTC #0 lore raw

PATCH 1/1 vfio: Set PCI_DEV_FLAGS_ASSIGNED when assigning device to guest

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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/vfio/pci/vfio_pci.c +2 -0

index 7ba0424..7cc7ed6 100644
@@ -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);
 }

Signature · 1 line

1.8.3.1

Alex Williamson

2014-01-30 17:33 UTC #1 lore raw

Quoted 4 lines · Bandan Das

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

Quoted 24 lines

---
 drivers/vfio/pci/vfio_pci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/vfio/pci/vfio_pci.c +2 -0

index 7ba0424..7cc7ed6 100644
@@ -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);
 }

Alex Williamson

2014-01-30 17:36 UTC #2 lore raw

[cc +linux-pci]

Quoted 41 lines · Alex Williamson

Quoted 4 lines · Bandan Das
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
Quoted 24 lines
---
 drivers/vfio/pci/vfio_pci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vfio/pci/vfio_pci.c +2 -0
index 7ba0424..7cc7ed6 100644
@@ -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);
 }
 

Bandan Das

author

2014-01-30 18:06 UTC #3 lore raw

Quoted 15 lines · Alex Williamson

[cc +linux-pci]
Quoted 12 lines · Alex Williamson
Quoted 4 lines · Bandan Das
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

Quoted 35 lines

Quoted 18 lines
Alex
Quoted 16 lines
---
 drivers/vfio/pci/vfio_pci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vfio/pci/vfio_pci.c +1 -0
index 7ba0424..7cc7ed6 100644
@@ -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;
 }
 
Quoted 10 lines
drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 156
Quoted 8 lines
drivers/vfio/pci/vfio_pci.c +1 -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Williamson

2014-01-30 18:38 UTC #4 lore raw

Quoted 18 lines · Bandan Das

Quoted 15 lines · Alex Williamson
[cc +linux-pci]
Quoted 12 lines · Alex Williamson
Quoted 4 lines · Bandan Das
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,

Quoted 2 lines

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?

Quoted 5 lines

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

Quoted 37 lines

Quoted 29 lines
Quoted 16 lines
Quoted 16 lines
---
 drivers/vfio/pci/vfio_pci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vfio/pci/vfio_pci.c +1 -0
index 7ba0424..7cc7ed6 100644
@@ -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;
 }
 
Quoted 10 lines
drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 156
Quoted 8 lines
drivers/vfio/pci/vfio_pci.c +1 -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);
 }
 
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bandan Das

author

2014-01-30 20:10 UTC #5 lore raw

Quoted 19 lines · Alex Williamson

Quoted 18 lines · Bandan Das
Quoted 15 lines · Alex Williamson
[cc +linux-pci]
Quoted 12 lines · Alex Williamson
Quoted 4 lines · Bandan Das
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,

Quoted 7 lines

Quoted 2 lines
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.

Quoted 11 lines

Quoted 5 lines
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 ?

Quoted 39 lines

Alex
Quoted 37 lines
Quoted 29 lines
Quoted 16 lines
Quoted 16 lines
---
 drivers/vfio/pci/vfio_pci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vfio/pci/vfio_pci.c +1 -0
index 7ba0424..7cc7ed6 100644
@@ -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;
 }
 
Quoted 10 lines
drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 156
Quoted 8 lines
drivers/vfio/pci/vfio_pci.c +1 -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);
 }
 
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html