[libvirt] [PATCH 7/8] Postpone reprobing till all the devices in iommu group are unbound from vfio
Andrea Bolognani
abologna at redhat.com
Fri Oct 30 15:53:35 UTC 2015
I know you're already working on a v2 of this, just a
couple of quick remarks below.
On Fri, 2015-10-30 at 05:01 +0530, Shivaprasad G Bhat wrote:
> Author: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
>
> The host will crash if a device is bound to host driver when the device
> belonging to same iommu group is in use by any of the guests. So,
> do the host driver probe only after all the devices in the iommu group
> have unbound from the vfio.
>
> The patch fixes
> https://bugzilla.redhat.com/show_bug.cgi?id=1272300
>
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> ---
> src/util/virpci.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 6c24a81..425c1a7 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1128,6 +1128,23 @@ static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char
> return result;
> }
>
> +static int virPCIDeviceBoundToVFIODriver(virPCIDeviceAddressPtr devAddr, void *opaque ATTRIBUTE_UNUSED)
Return type on a separate line, please.
> +{
> + int ret = 0;
> + virPCIDevicePtr pci = NULL;
> +
> + if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus,
> + devAddr->slot, devAddr->function)))
> + goto cleanup;
> +
> + if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci"))
> + ret = -1;
As mentioned in the comment for Patch 1, I think this is
fairly obscure: if I had not read throught the whole
series, I'd assume this checks whether the device is
configured to use vfio-pci as stub driver, not whether
it is currently bound to it, and it would not be
immediately clear to me how this check fits in a function
called virPCIDeviceBoundToVFIODriver().
I think it would be much cleaner to query the driver
explicitly using virPCIDeviceGetDriverPathAndName() and
remove that call from virPCIDeviceNew().
> +
> + cleanup:
> + virPCIDeviceFree(pci);
> + return ret;
> +}
> +
> static int
> virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
> virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED,
> @@ -1177,11 +1194,34 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
> dev->remove_slot = false;
>
> reprobe:
> - if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
> - goto cleanup;
> - /* Steal the dev from list inactiveDevs */
> - if (inactiveDevs)
> - virPCIDeviceListDel(inactiveDevs, dev);
> + if (STREQ_NULLABLE(dev->stubDriver, "vfio-pci")) {
> + size_t i = 0;
> + virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
> + dev->slot, dev->function };
> + if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, virPCIDeviceBoundToVFIODriver, NULL) < 0) {
> + result = 0;
> + goto cleanup;
> + }
> +
> + while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) {
> + virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i);
> + if (dev->iommuGroup == pcidev->iommuGroup) {
> + if (virPCIDeviceReprobeHostDriver(pcidev, driver, drvdir) < 0)
> + goto cleanup;
> + /* Steal the dev from list inactiveDevs */
> + virPCIDeviceListDel(inactiveDevs, pcidev);
> + continue;
> + }
> + i++;
> + }
> + } else {
> + if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
> + goto cleanup;
> + /* Steal the dev from list inactiveDevs */
> +
> + if (inactiveDevs)
> + virPCIDeviceListDel(inactiveDevs, dev);
Either put the empty line before the comment or just
get rid of it.
> + }
>
> result = 0;
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
More information about the libvir-list
mailing list