Re: [libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

2019-08-16 Thread Daniel Henrique Barboza 

Reviewed-by: Daniel Henrique Barboza 

On 8/14/19 8:57 AM, Michal Privoznik wrote:

The virHostdevPreparePCIDevices() function works in several
steps. In the very first one, it checks if devices we want to
detach from the host are not taken already by some other domain.
However, this piece of code returns different results depending
on the stub driver used (which is not wrong per se, but keep on
reading). If the stub driver is KVM then
virHostdevIsPCINodeDeviceUsed() is called which basically checks
if a PCI device from the detach list is not used by any domain
(including the one we are preparing the device for). If that is
the case, an error is reported ("device in use") and -1 is
returned.

However, that is not what happens if the stub driver is VFIO. If
the stub driver is VFIO, then we iterate over all PCI devices
from the same IOMMU group and check if they are taken by some
other domain (because a PCI device, well IOMMU group, can't be
shared between two or more qemu processes). But we fail to check,
if the device we are trying to detach from the host is not
already taken by a domain. That is, calling
virHostdevPreparePCIDevices() over a hostdev device twice
succeeds the first time and fails too late in the second run
(fortunately, virHostdevResetAllPCIDevices() will throw an error,
but this is already too late because the PCI device in question
was moved to the list of inactive PCI devices and now it appears
in both lists).

Signed-off-by: Michal Privoznik 
---
  src/util/virhostdev.c | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index cb69582c21..6861b8a84e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -53,7 +53,7 @@ struct virHostdevIsPCINodeDeviceUsedData {
  virHostdevManagerPtr mgr;
  const char *driverName;
  const char *domainName;
-const bool usesVFIO;
+bool usesVFIO;
  };
  
  /* This module makes heavy use of bookkeeping lists contained inside a

@@ -707,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
  virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
  bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
  bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
VIR_PCI_STUB_DRIVER_VFIO);
-struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, usesVFIO};
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, false};
  int hdrType = -1;
  
  if (virPCIGetHeaderType(pci, ) < 0)

@@ -728,18 +728,19 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
  }
  
  /* The device is in use by other active domain if

- * the dev is in list activePCIHostdevs. VFIO devices
- * belonging to same iommu group can't be shared
- * across guests.
- */
+ * the dev is in list activePCIHostdevs. */
  devAddr = virPCIDeviceGetAddress(pci);
+if (virHostdevIsPCINodeDeviceUsed(devAddr, ))
+goto cleanup;
+
+/* VFIO devices belonging to same IOMMU group can't be
+ * shared across guests. Check if that's the case. */
  if (usesVFIO) {
+data.usesVFIO = true;
  if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
   
virHostdevIsPCINodeDeviceUsed,
   ) < 0)
  goto cleanup;
-} else if (virHostdevIsPCINodeDeviceUsed(devAddr, )) {
-goto cleanup;
  }
  }
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

2019-08-16 Thread Daniel Henrique Barboza 




On 8/16/19 5:59 AM, Michal Privoznik wrote:

On 8/14/19 6:14 PM, Daniel Henrique Barboza wrote:

Michal, I believe the problem you're trying to fix in this patch
is somehow the same I'm trying to fix in this patch here:


https://www.redhat.com/archives/libvir-list/2019-June/msg01270.html

Do you mind taking a look? If that's the case, I can drop that patch
from the series that implements Multifunction PCI hotplug.


Oh, I haven't taken a look on them yet. Yes, this patch fixes the same 
problem. If you're okay with it, I'd rather go with my approach since 
it's simpler.


I don't mind. Let's go with yours. I'll remove that patch from the series as
soon as this gets pushed (I would need to resubmit that whole series
anyway).


Thanks,


DHB



Michal


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

2019-08-16 Thread Michal Privoznik 

On 8/14/19 6:14 PM, Daniel Henrique Barboza wrote:

Michal, I believe the problem you're trying to fix in this patch
is somehow the same I'm trying to fix in this patch here:


https://www.redhat.com/archives/libvir-list/2019-June/msg01270.html

Do you mind taking a look? If that's the case, I can drop that patch
from the series that implements Multifunction PCI hotplug.


Oh, I haven't taken a look on them yet. Yes, this patch fixes the same 
problem. If you're okay with it, I'd rather go with my approach since 
it's simpler.


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

2019-08-14 Thread Daniel Henrique Barboza 

Michal, I believe the problem you're trying to fix in this patch
is somehow the same I'm trying to fix in this patch here:


https://www.redhat.com/archives/libvir-list/2019-June/msg01270.html

Do you mind taking a look? If that's the case, I can drop that patch
from the series that implements Multifunction PCI hotplug.


Thanks,

DHB


On 8/14/19 8:57 AM, Michal Privoznik wrote:

The virHostdevPreparePCIDevices() function works in several
steps. In the very first one, it checks if devices we want to
detach from the host are not taken already by some other domain.
However, this piece of code returns different results depending
on the stub driver used (which is not wrong per se, but keep on
reading). If the stub driver is KVM then
virHostdevIsPCINodeDeviceUsed() is called which basically checks
if a PCI device from the detach list is not used by any domain
(including the one we are preparing the device for). If that is
the case, an error is reported ("device in use") and -1 is
returned.

However, that is not what happens if the stub driver is VFIO. If
the stub driver is VFIO, then we iterate over all PCI devices
from the same IOMMU group and check if they are taken by some
other domain (because a PCI device, well IOMMU group, can't be
shared between two or more qemu processes). But we fail to check,
if the device we are trying to detach from the host is not
already taken by a domain. That is, calling
virHostdevPreparePCIDevices() over a hostdev device twice
succeeds the first time and fails too late in the second run
(fortunately, virHostdevResetAllPCIDevices() will throw an error,
but this is already too late because the PCI device in question
was moved to the list of inactive PCI devices and now it appears
in both lists).

Signed-off-by: Michal Privoznik 
---
  src/util/virhostdev.c | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index cb69582c21..6861b8a84e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -53,7 +53,7 @@ struct virHostdevIsPCINodeDeviceUsedData {
  virHostdevManagerPtr mgr;
  const char *driverName;
  const char *domainName;
-const bool usesVFIO;
+bool usesVFIO;
  };
  
  /* This module makes heavy use of bookkeeping lists contained inside a

@@ -707,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
  virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
  bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
  bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
VIR_PCI_STUB_DRIVER_VFIO);
-struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, usesVFIO};
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, false};
  int hdrType = -1;
  
  if (virPCIGetHeaderType(pci, ) < 0)

@@ -728,18 +728,19 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
  }
  
  /* The device is in use by other active domain if

- * the dev is in list activePCIHostdevs. VFIO devices
- * belonging to same iommu group can't be shared
- * across guests.
- */
+ * the dev is in list activePCIHostdevs. */
  devAddr = virPCIDeviceGetAddress(pci);
+if (virHostdevIsPCINodeDeviceUsed(devAddr, ))
+goto cleanup;
+
+/* VFIO devices belonging to same IOMMU group can't be
+ * shared across guests. Check if that's the case. */
  if (usesVFIO) {
+data.usesVFIO = true;
  if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
   
virHostdevIsPCINodeDeviceUsed,
   ) < 0)
  goto cleanup;
-} else if (virHostdevIsPCINodeDeviceUsed(devAddr, )) {
-goto cleanup;
  }
  }
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

2019-08-14 Thread Michal Privoznik 
The virHostdevPreparePCIDevices() function works in several
steps. In the very first one, it checks if devices we want to
detach from the host are not taken already by some other domain.
However, this piece of code returns different results depending
on the stub driver used (which is not wrong per se, but keep on
reading). If the stub driver is KVM then
virHostdevIsPCINodeDeviceUsed() is called which basically checks
if a PCI device from the detach list is not used by any domain
(including the one we are preparing the device for). If that is
the case, an error is reported ("device in use") and -1 is
returned.

However, that is not what happens if the stub driver is VFIO. If
the stub driver is VFIO, then we iterate over all PCI devices
from the same IOMMU group and check if they are taken by some
other domain (because a PCI device, well IOMMU group, can't be
shared between two or more qemu processes). But we fail to check,
if the device we are trying to detach from the host is not
already taken by a domain. That is, calling
virHostdevPreparePCIDevices() over a hostdev device twice
succeeds the first time and fails too late in the second run
(fortunately, virHostdevResetAllPCIDevices() will throw an error,
but this is already too late because the PCI device in question
was moved to the list of inactive PCI devices and now it appears
in both lists).

Signed-off-by: Michal Privoznik 
---
 src/util/virhostdev.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index cb69582c21..6861b8a84e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -53,7 +53,7 @@ struct virHostdevIsPCINodeDeviceUsedData {
 virHostdevManagerPtr mgr;
 const char *driverName;
 const char *domainName;
-const bool usesVFIO;
+bool usesVFIO;
 };
 
 /* This module makes heavy use of bookkeeping lists contained inside a
@@ -707,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
 virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
 bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
 bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
VIR_PCI_STUB_DRIVER_VFIO);
-struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, usesVFIO};
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, false};
 int hdrType = -1;
 
 if (virPCIGetHeaderType(pci, ) < 0)
@@ -728,18 +728,19 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
 }
 
 /* The device is in use by other active domain if
- * the dev is in list activePCIHostdevs. VFIO devices
- * belonging to same iommu group can't be shared
- * across guests.
- */
+ * the dev is in list activePCIHostdevs. */
 devAddr = virPCIDeviceGetAddress(pci);
+if (virHostdevIsPCINodeDeviceUsed(devAddr, ))
+goto cleanup;
+
+/* VFIO devices belonging to same IOMMU group can't be
+ * shared across guests. Check if that's the case. */
 if (usesVFIO) {
+data.usesVFIO = true;
 if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
  
virHostdevIsPCINodeDeviceUsed,
  ) < 0)
 goto cleanup;
-} else if (virHostdevIsPCINodeDeviceUsed(devAddr, )) {
-goto cleanup;
 }
 }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list