Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

The pci_driver_bind() and pci_driver_unbind() functions are "internal implementation", meaning other parts of the code should be able to call them and get the job done. Checking for actions (PCI_ACTION_BIND and PCI_ACTION_UNBIND) should be done in handlers (pci_driver_handle_bind() and pci_driver_handle_unbind()). Surprisingly, the other two actions (PCI_ACTION_NEW_ID and PCI_ACTION_REMOVE_ID) are checked already at this level.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index beb5e1490d..6865f992dc 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -551,8 +551,8 @@ pci_driver_bind(struct pciDriver *driver, int ret = -1; char *devpath = NULL, *driverpath = NULL;

- if (dev->driver || PCI_ACTION_BIND & driver->fail) { - /* Device already bound or failing driver requested */ + if (dev->driver) { + /* Device already bound */ errno = ENODEV; return ret; } @@ -598,8 +598,8 @@ pci_driver_unbind(struct pciDriver *driver, int ret = -1; char *devpath = NULL, *driverpath = NULL;

- if (dev->driver != driver || PCI_ACTION_UNBIND & driver->fail) { - /* Device not bound to the @driver or failing driver used */ + if (dev->driver != driver) { + /* Device not bound to the @driver */ errno = ENODEV; return ret; } @@ -669,8 +669,8 @@ pci_driver_handle_bind(const char *path) struct pciDevice *dev = pci_device_find_by_content(path); struct pciDriver *driver = pci_driver_find_by_path(path);

- if (!driver || !dev) { - /* This should never happen (TM) */ + if (!driver || !dev || PCI_ACTION_BIND & driver->fail) { + /* No driver, no device or failing driver requested */ errno = ENODEV; goto cleanup; } @@ -686,8 +686,8 @@ pci_driver_handle_unbind(const char *path) int ret = -1; struct pciDevice *dev = pci_device_find_by_content(path);

- if (!dev || !dev->driver) { - /* This should never happen (TM) */ + if (!dev || !dev->driver || PCI_ACTION_UNBIND & dev->driver->fail) { + /* No device, device not binded or failing driver requested */ errno = ENODEV; goto cleanup; }

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

This reverts commit b70c093ffa00cd87c8d39d3652b798f033a81faf.

In next commit the virpcimock is going to be extended and thus binding a PCI device to vfio-pci driver will finally succeed. Remove this test as it will no longer make sense.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 32 -------------------------------- 1 file changed, 32 deletions(-)

diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 961a7eff1a..9ecd1b7d27 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -256,36 +256,6 @@ testVirPCIDeviceDetachSingle(const void *opaque) return ret; }

-static int -testVirPCIDeviceDetachFail(const void *opaque) -{ - const struct testPCIDevData *data = opaque; - int ret = -1; - virPCIDevicePtr dev; - - dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); - if (!dev) - goto cleanup; - - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); - - if (virPCIDeviceDetach(dev, NULL, NULL) < 0) { - if (virTestGetVerbose() || virTestGetDebug()) - virDispatchError(NULL); - virResetLastError(); - ret = 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Attaching device %s to %s should have failed", - virPCIDeviceGetName(dev), - virPCIStubDriverTypeToString(VIR_PCI_STUB_DRIVER_VFIO)); - } - - cleanup: - virPCIDeviceFree(dev); - return ret; -} - static int testVirPCIDeviceReattachSingle(const void *opaque) { @@ -421,8 +391,6 @@ mymain(void) DO_TEST_PCI(testVirPCIDeviceIsAssignable, 5, 0x90, 1, 0); DO_TEST_PCI(testVirPCIDeviceIsAssignable, 1, 1, 0, 0);

- DO_TEST_PCI(testVirPCIDeviceDetachFail, 0, 0x0a, 1, 0); - /* Reattach a device already bound to non-stub a driver */ DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915"); DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 1, 0);

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

Newer kernels (v3.16-rc1~29^2~6^4) have 'driver_override' file which simplifies way of binding a PCI device to desired driver. Libvirt has support for this for some time too (v2.3.0-rc1~236), but not our virpcimock. So far we did not care because our code is designed to deal with this situation. Except for one. hypothetical case: binding a device to the vfio-pci driver can be successful only via driver_override. Any attempt to bind a PCI device to vfio-pci driver using old method (new_id + unbind + bind) will fail because of b803b29c1a5. While on vanilla kernel I'm able to use the old method successfully, it's failing on RHEL kernels (not sure why).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 52 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 6865f992dc..1c21e4e045 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -87,6 +87,11 @@ char *fakesysfspcidir; * Probe for a driver that handles the specified device. * Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function). * + * /sys/bus/pci/devices/<device>/driver_override + * Name of a driver that overrides preferred driver can be written + * here. The device will be attached to it on drivers_probe event. + * Writing an empty string (or "\n") clears the override. + * * As a little hack, we are not mocking write to these files, but close() * instead. The advantage is we don't need any self growing array to hold the * partial writes and construct them back. We can let all the writes finish, @@ -147,6 +152,7 @@ static struct pciDevice *pci_device_find_by_content(const char *path); static void pci_driver_new(const char *name, int fail, ...); static struct pciDriver *pci_driver_find_by_dev(struct pciDevice *dev); static struct pciDriver *pci_driver_find_by_path(const char *path); +static struct pciDriver *pci_driver_find_by_driver_override(struct pciDevice *dev); static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev); static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev); static int pci_driver_handle_change(int fd, const char *path); @@ -202,7 +208,8 @@ make_symlink(const char *path, static int pci_read_file(const char *path, char *buf, - size_t buf_size) + size_t buf_size, + bool truncate) { int ret = -1; int fd = -1; @@ -224,7 +231,8 @@ pci_read_file(const char *path, goto cleanup; }

- if (ftruncate(fd, 0) < 0) + if (truncate && + ftruncate(fd, 0) < 0) goto cleanup;

ret = 0; @@ -398,6 +406,8 @@ pci_device_new_from_stub(const struct pciDevice *data) ABORT("@tmp overflow"); make_file(devpath, "class", tmp, -1);

+ make_file(devpath, "driver_override", NULL, -1); + if (snprintf(tmp, sizeof(tmp), "%s/../../../kernel/iommu_groups/%d", devpath, dev->iommuGroup) < 0) { @@ -441,7 +451,7 @@ pci_device_find_by_content(const char *path) { char tmp[32];

- if (pci_read_file(path, tmp, sizeof(tmp)) < 0) + if (pci_read_file(path, tmp, sizeof(tmp), true) < 0) return NULL;

return pci_device_find_by_id(tmp); @@ -450,7 +460,10 @@ pci_device_find_by_content(const char *path) static int pci_device_autobind(struct pciDevice *dev) { - struct pciDriver *driver = pci_driver_find_by_dev(dev); + struct pciDriver *driver = pci_driver_find_by_driver_override(dev); + + if (!driver) + driver = pci_driver_find_by_dev(dev);

if (!driver) { /* No driver found. Nothing to do */ @@ -544,6 +557,31 @@ pci_driver_find_by_path(const char *path) return NULL; }

+static struct pciDriver * +pci_driver_find_by_driver_override(struct pciDevice *dev) +{ + VIR_AUTOFREE(char *) path = NULL; + char tmp[32]; + size_t i; + + if (virAsprintfQuiet(&path, + SYSFS_PCI_PREFIX "devices/%s/driver_override", + dev->id) < 0) + return NULL; + + if (pci_read_file(path, tmp, sizeof(tmp), false) < 0) + return NULL; + + for (i = 0; i < nPCIDrivers; i++) { + struct pciDriver *driver = pciDrivers[i]; + + if (STREQ(tmp, driver->name)) + return driver; + } + + return NULL; +} + static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev) @@ -657,6 +695,8 @@ pci_driver_handle_change(int fd ATTRIBUTE_UNUSED, const char *path) ret = pci_driver_handle_remove_id(path); else if (STREQ(file, "drivers_probe")) ret = pci_driver_handle_drivers_probe(path); + else if (STREQ(file, "driver_override")) + ret = 0; /* nada */ else ABORT("Not handled write to: %s", path); return ret; @@ -711,7 +751,7 @@ pci_driver_handle_new_id(const char *path) goto cleanup; }

- if (pci_read_file(path, buf, sizeof(buf)) < 0) + if (pci_read_file(path, buf, sizeof(buf), true) < 0) goto cleanup;

if (sscanf(buf, "%x %x", &vendor, &device) < 2) { @@ -766,7 +806,7 @@ pci_driver_handle_remove_id(const char *path) goto cleanup; }

- if (pci_read_file(path, buf, sizeof(buf)) < 0) + if (pci_read_file(path, buf, sizeof(buf), true) < 0) goto cleanup;

if (sscanf(buf, "%x %x", &vendor, &device) < 2) {

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

When creating a PCI device, the pciDevice structure contains @id member which holds device address (DDDD.BB:DD.F) and is type of 'char *'. But the structure is initialized from a const char and in fact we never modify or free the @id.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 1c21e4e045..853ac588e9 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -120,7 +120,7 @@ struct pciDriver { };

struct pciDevice { - char *id; + const char *id; int vendor; int device; int klass; @@ -878,7 +878,7 @@ init_env(void)

# define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \ do { \ - struct pciDevice dev = {.id = (char *)Id, .vendor = Vendor, \ + struct pciDevice dev = {.id = Id, .vendor = Vendor, \ .device = Device, __VA_ARGS__}; \ pci_device_new_from_stub(&dev); \ } while (0)

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

It saves us couple of lines.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 109 +++++++++++++++------------------------------ 1 file changed, 36 insertions(+), 73 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 853ac588e9..0950f3ba00 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -172,7 +172,7 @@ make_file(const char *path, ssize_t len) { int fd = -1; - char *filepath = NULL; + VIR_AUTOFREE(char *) filepath = NULL; if (value && len == -1) len = strlen(value);

@@ -186,7 +186,6 @@ make_file(const char *path, ABORT("Unable to write: %s", filepath);

VIR_FORCE_CLOSE(fd); - VIR_FREE(filepath); }

static void @@ -194,15 +193,13 @@ make_symlink(const char *path, const char *name, const char *target) { - char *filepath = NULL; + VIR_AUTOFREE(char *) filepath = NULL;

if (virAsprintfQuiet(&filepath, "%s/%s", path, name) < 0) ABORT_OOM();

if (symlink(target, filepath) < 0) ABORT("Unable to create symlink filepath -> target"); - - VIR_FREE(filepath); }

static int @@ -213,7 +210,7 @@ pci_read_file(const char *path, { int ret = -1; int fd = -1; - char *newpath; + VIR_AUTOFREE(char *) newpath = NULL;

if (virAsprintfQuiet(&newpath, "%s/%s", fakesysfspcidir, @@ -237,7 +234,6 @@ pci_read_file(const char *path,

ret = 0; cleanup: - VIR_FREE(newpath); real_close(fd); return ret; } @@ -335,10 +331,10 @@ static void pci_device_new_from_stub(const struct pciDevice *data) { struct pciDevice *dev; - char *devpath; - char *id; + VIR_AUTOFREE(char *) devpath = NULL; + VIR_AUTOFREE(char *) id = NULL; char *c; - char *configSrc; + VIR_AUTOFREE(char *) configSrc = NULL; char tmp[256]; struct stat sb; bool configSrcExists = false; @@ -363,7 +359,6 @@ pci_device_new_from_stub(const struct pciDevice *data) virAsprintfQuiet(&devpath, "%s/devices/%s", fakesysfspcidir, data->id) < 0) ABORT_OOM();

- VIR_FREE(id); memcpy(dev, data, sizeof(*dev));

if (virFileMakePath(devpath) < 0) @@ -380,14 +375,13 @@ pci_device_new_from_stub(const struct pciDevice *data) * file, and parallel VPATH builds must not stomp on the * original; besides, 'make distcheck' requires the original * to be read-only */ - char *buf; + VIR_AUTOFREE(char *) buf = NULL; ssize_t len;

if ((len = virFileReadAll(configSrc, 4096, &buf)) < 0) ABORT("Unable to read config file '%s'", configSrc);

make_file(devpath, "config", buf, len); - VIR_FREE(buf); } else { /* If there's no config data in the virpcitestdata dir, create a dummy * config file */ @@ -427,9 +421,6 @@ pci_device_new_from_stub(const struct pciDevice *data)

if (VIR_APPEND_ELEMENT_QUIET(pciDevices, nPCIDevices, dev) < 0) ABORT_OOM(); - - VIR_FREE(devpath); - VIR_FREE(configSrc); }

static struct pciDevice * @@ -483,7 +474,7 @@ pci_driver_new(const char *name, int fail, ...) struct pciDriver *driver; va_list args; int vendor, device; - char *driverpath; + VIR_AUTOFREE(char *) driverpath = NULL;

if (VIR_ALLOC_QUIET(driver) < 0 || VIR_STRDUP_QUIET(driver->name, name) < 0 || @@ -519,8 +510,6 @@ pci_driver_new(const char *name, int fail, ...)

if (VIR_APPEND_ELEMENT_QUIET(pciDrivers, nPCIDrivers, driver) < 0) ABORT_OOM(); - - VIR_FREE(driverpath); }

static struct pciDriver * @@ -586,13 +575,13 @@ static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev) { - int ret = -1; - char *devpath = NULL, *driverpath = NULL; + VIR_AUTOFREE(char *) devpath = NULL; + VIR_AUTOFREE(char *) driverpath = NULL;

if (dev->driver) { /* Device already bound */ errno = ENODEV; - return ret; + return -1; }

/* Make symlink under device tree */ @@ -601,11 +590,11 @@ pci_driver_bind(struct pciDriver *driver, virAsprintfQuiet(&driverpath, "%s/drivers/%s", fakesysfspcidir, driver->name) < 0) { errno = ENOMEM; - goto cleanup; + return -1; }

if (symlink(driverpath, devpath) < 0) - goto cleanup; + return -1;

/* Make symlink under driver tree */ VIR_FREE(devpath); @@ -615,31 +604,27 @@ pci_driver_bind(struct pciDriver *driver, virAsprintfQuiet(&driverpath, "%s/drivers/%s/%s", fakesysfspcidir, driver->name, dev->id) < 0) { errno = ENOMEM; - goto cleanup; + return -1; }

if (symlink(devpath, driverpath) < 0) - goto cleanup; + return -1;

dev->driver = driver; - ret = 0; - cleanup: - VIR_FREE(devpath); - VIR_FREE(driverpath); - return ret; + return 0; }

static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev) { - int ret = -1; - char *devpath = NULL, *driverpath = NULL; + VIR_AUTOFREE(char *) devpath = NULL; + VIR_AUTOFREE(char *) driverpath = NULL;

if (dev->driver != driver) { /* Device not bound to the @driver */ errno = ENODEV; - return ret; + return -1; }

/* Make symlink under device tree */ @@ -648,19 +633,15 @@ pci_driver_unbind(struct pciDriver *driver, virAsprintfQuiet(&driverpath, "%s/drivers/%s/%s", fakesysfspcidir, driver->name, dev->id) < 0) { errno = ENOMEM; - goto cleanup; + return -1; }

if (unlink(devpath) < 0 || unlink(driverpath) < 0) - goto cleanup; + return -1;

dev->driver = NULL; - ret = 0; - cleanup: - VIR_FREE(devpath); - VIR_FREE(driverpath); - return ret; + return 0; }

static int @@ -909,20 +890,15 @@ init_env(void) int access(const char *path, int mode) { - int ret; + VIR_AUTOFREE(char *) newpath = NULL;

init_syms();

- if (STRPREFIX(path, SYSFS_PCI_PREFIX)) { - char *newpath; - if (getrealpath(&newpath, path) < 0) - return -1; - ret = real_access(newpath, mode); - VIR_FREE(newpath); - } else { - ret = real_access(path, mode); - } - return ret; + if (STRPREFIX(path, SYSFS_PCI_PREFIX) && + getrealpath(&newpath, path) < 0) + return -1; + + return real_access(newpath ? newpath : path, mode); }

@@ -941,7 +917,7 @@ int open(const char *path, int flags, ...) { int ret; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL;

init_syms();

@@ -968,15 +944,13 @@ open(const char *path, int flags, ...) ret = -1; }

- VIR_FREE(newpath); return ret; }

DIR * opendir(const char *path) { - DIR *ret; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL;

init_syms();

@@ -984,10 +958,7 @@ opendir(const char *path) getrealpath(&newpath, path) < 0) return NULL;

- ret = real_opendir(newpath ? newpath : path); - - VIR_FREE(newpath); - return ret; + return real_opendir(newpath ? newpath : path); }

int @@ -1001,23 +972,15 @@ close(int fd) char * virFileCanonicalizePath(const char *path) { - char *ret; + VIR_AUTOFREE(char *) newpath = NULL;

init_syms();

- if (STRPREFIX(path, SYSFS_PCI_PREFIX)) { - char *newpath; + if (STRPREFIX(path, SYSFS_PCI_PREFIX) && + getrealpath(&newpath, path) < 0) + return NULL;

- if (getrealpath(&newpath, path) < 0) - return NULL; - - ret = real_virFileCanonicalizePath(newpath); - VIR_FREE(newpath); - } else { - ret = real_virFileCanonicalizePath(path); - } - - return ret; + return real_virFileCanonicalizePath(newpath ? newpath : path); }

# include "virmockstathelpers.c"

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

The @fakesysfspcidir is derived from @fakerootdir. We don't need two global variables that contain nearly the same content, especially when we construct the actual path anyways.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 0950f3ba00..296da9b453 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -41,7 +41,6 @@ static char *(*real_virFileCanonicalizePath)(const char *path); * vircgroupmock.c:462:22: error: static variable 'fakesysfsdir' is used in an inline function with external linkage [-Werror,-Wstatic-in-inline] */ char *fakerootdir; -char *fakesysfspcidir;

# define SYSFS_PCI_PREFIX "/sys/bus/pci/"

@@ -212,9 +211,7 @@ pci_read_file(const char *path, int fd = -1; VIR_AUTOFREE(char *) newpath = NULL;

- if (virAsprintfQuiet(&newpath, "%s/%s", - fakesysfspcidir, - path + strlen(SYSFS_PCI_PREFIX)) < 0) { + if (virAsprintfQuiet(&newpath, "%s/%s", fakerootdir, path) < 0) { errno = ENOMEM; goto cleanup; } @@ -245,8 +242,8 @@ getrealpath(char **newpath, init_env();

if (STRPREFIX(path, SYSFS_PCI_PREFIX)) { - if (virAsprintfQuiet(newpath, "%s/%s", - fakesysfspcidir, + if (virAsprintfQuiet(newpath, "%s/sys/bus/pci/%s", + fakerootdir, path + strlen(SYSFS_PCI_PREFIX)) < 0) { errno = ENOMEM; return -1; @@ -356,7 +353,7 @@ pci_device_new_from_stub(const struct pciDevice *data) if (VIR_ALLOC_QUIET(dev) < 0 || virAsprintfQuiet(&configSrc, "%s/virpcitestdata/%s.config", abs_srcdir, id) < 0 || - virAsprintfQuiet(&devpath, "%s/devices/%s", fakesysfspcidir, data->id) < 0) + virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s", fakerootdir, data->id) < 0) ABORT_OOM();

memcpy(dev, data, sizeof(*dev)); @@ -478,7 +475,7 @@ pci_driver_new(const char *name, int fail, ...)

if (VIR_ALLOC_QUIET(driver) < 0 || VIR_STRDUP_QUIET(driver->name, name) < 0 || - virAsprintfQuiet(&driverpath, "%s/drivers/%s", fakesysfspcidir, name) < 0) + virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s", fakerootdir, name) < 0) ABORT_OOM();

driver->fail = fail; @@ -585,10 +582,10 @@ pci_driver_bind(struct pciDriver *driver, }

/* Make symlink under device tree */ - if (virAsprintfQuiet(&devpath, "%s/devices/%s/driver", - fakesysfspcidir, dev->id) < 0 || - virAsprintfQuiet(&driverpath, "%s/drivers/%s", - fakesysfspcidir, driver->name) < 0) { + if (virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s/driver", + fakerootdir, dev->id) < 0 || + virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s", + fakerootdir, driver->name) < 0) { errno = ENOMEM; return -1; } @@ -599,10 +596,10 @@ pci_driver_bind(struct pciDriver *driver, /* Make symlink under driver tree */ VIR_FREE(devpath); VIR_FREE(driverpath); - if (virAsprintfQuiet(&devpath, "%s/devices/%s", - fakesysfspcidir, dev->id) < 0 || - virAsprintfQuiet(&driverpath, "%s/drivers/%s/%s", - fakesysfspcidir, driver->name, dev->id) < 0) { + if (virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s", + fakerootdir, dev->id) < 0 || + virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s/%s", + fakerootdir, driver->name, dev->id) < 0) { errno = ENOMEM; return -1; } @@ -628,10 +625,10 @@ pci_driver_unbind(struct pciDriver *driver, }

/* Make symlink under device tree */ - if (virAsprintfQuiet(&devpath, "%s/devices/%s/driver", - fakesysfspcidir, dev->id) < 0 || - virAsprintfQuiet(&driverpath, "%s/drivers/%s/%s", - fakesysfspcidir, driver->name, dev->id) < 0) { + if (virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s/driver", + fakerootdir, dev->id) < 0 || + virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s/%s", + fakerootdir, driver->name, dev->id) < 0) { errno = ENOMEM; return -1; } @@ -834,7 +831,9 @@ init_syms(void) static void init_env(void) { - if (fakerootdir && fakesysfspcidir) + VIR_AUTOFREE(char *) fakesysfspcidir = NULL; + + if (fakerootdir) return;

if (!(fakerootdir = getenv("LIBVIRT_FAKE_ROOT_DIR")))

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

We will need to create more directories and instead of introducing bunch of new variables to hold their actual paths, we can have one and reuse it.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 296da9b453..e97dbd81f8 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -831,7 +831,7 @@ init_syms(void) static void init_env(void) { - VIR_AUTOFREE(char *) fakesysfspcidir = NULL; + VIR_AUTOFREE(char *) tmp = NULL;

if (fakerootdir) return; @@ -839,14 +839,14 @@ init_env(void) if (!(fakerootdir = getenv("LIBVIRT_FAKE_ROOT_DIR"))) ABORT("Missing LIBVIRT_FAKE_ROOT_DIR env variable\n");

- if (virAsprintfQuiet(&fakesysfspcidir, "%s%s", + if (virAsprintfQuiet(&tmp, "%s%s", fakerootdir, SYSFS_PCI_PREFIX) < 0) ABORT_OOM();

- if (virFileMakePath(fakesysfspcidir) < 0) - ABORT("Unable to create: %s", fakesysfspcidir); + if (virFileMakePath(tmp) < 0) + ABORT("Unable to create: %s", tmp);

- make_file(fakesysfspcidir, "drivers_probe", NULL, -1); + make_file(tmp, "drivers_probe", NULL, -1);

# define MAKE_PCI_DRIVER(name, ...) \ pci_driver_new(name, 0, __VA_ARGS__, -1, -1)

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

In near future, we will be creating devices under different location and just symlink them under devices/. Just like real kernel does. But for that we need the directories to exists.

nit: s/exists/exist

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index e97dbd81f8..213f906500 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -187,6 +187,19 @@ make_file(const char *path, VIR_FORCE_CLOSE(fd); }

+static void +make_dir(const char *path, + const char *name) +{ + VIR_AUTOFREE(char *) dirpath = NULL; + + if (virAsprintfQuiet(&dirpath, "%s/%s", path, name) < 0) + ABORT_OOM(); + + if (virFileMakePath(dirpath) < 0) + ABORT("Unable to create: %s", dirpath); +} + static void make_symlink(const char *path, const char *name, @@ -846,6 +859,8 @@ init_env(void) if (virFileMakePath(tmp) < 0) ABORT("Unable to create: %s", tmp);

+ make_dir(tmp, "devices"); + make_dir(tmp, "drivers");

make_dir could also be used to create the tmp directory as well, like make_dir(fakerootdir, SYSFS_PCI_PREFIX), getting rid of all the virFileMakePath() calls inside init_env(). Not worth rerolling the patch just because of that though. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

make_file(tmp, "drivers_probe", NULL, -1);

# define MAKE_PCI_DRIVER(name, ...) \

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

Have just one function to generate path to a PCI device so that when we change it in near future there's only few of the places we need to fix.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 213f906500..52a585d0cc 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -337,6 +337,29 @@ remove_fd(int fd) /* * PCI Device functions */ +static char * +pci_device_get_path(const struct pciDevice *dev, + const char *file, + bool faked) +{ + char *ret = NULL; + const char *prefix = ""; + + if (faked) + prefix = fakerootdir; + + if (file) { + ignore_value(virAsprintfQuiet(&ret, "%s" SYSFS_PCI_PREFIX "devices/%s/%s", + prefix, dev->id, file)); + } else { + ignore_value(virAsprintfQuiet(&ret, "%s" SYSFS_PCI_PREFIX "devices/%s", + prefix, dev->id)); + } + + return ret; +} + + static void pci_device_new_from_stub(const struct pciDevice *data) { @@ -365,12 +388,14 @@ pci_device_new_from_stub(const struct pciDevice *data)

if (VIR_ALLOC_QUIET(dev) < 0 || virAsprintfQuiet(&configSrc, "%s/virpcitestdata/%s.config", - abs_srcdir, id) < 0 || - virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s", fakerootdir, data->id) < 0) + abs_srcdir, id) < 0) ABORT_OOM();

memcpy(dev, data, sizeof(*dev));

+ if (!(devpath = pci_device_get_path(dev, NULL, true))) + ABORT_OOM(); + if (virFileMakePath(devpath) < 0) ABORT("Unable to create: %s", devpath);

@@ -563,9 +588,7 @@ pci_driver_find_by_driver_override(struct pciDevice *dev) char tmp[32]; size_t i;

- if (virAsprintfQuiet(&path, - SYSFS_PCI_PREFIX "devices/%s/driver_override", - dev->id) < 0) + if (!(path = pci_device_get_path(dev, "driver_override", false))) return NULL;

if (pci_read_file(path, tmp, sizeof(tmp), false) < 0) @@ -595,8 +618,7 @@ pci_driver_bind(struct pciDriver *driver, }

/* Make symlink under device tree */ - if (virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s/driver", - fakerootdir, dev->id) < 0 || + if (!(devpath = pci_device_get_path(dev, "driver", true)) || virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s", fakerootdir, driver->name) < 0) { errno = ENOMEM; @@ -609,8 +631,7 @@ pci_driver_bind(struct pciDriver *driver, /* Make symlink under driver tree */ VIR_FREE(devpath); VIR_FREE(driverpath); - if (virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s", - fakerootdir, dev->id) < 0 || + if (!(devpath = pci_device_get_path(dev, NULL, true)) || virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s/%s", fakerootdir, driver->name, dev->id) < 0) { errno = ENOMEM; @@ -638,8 +659,7 @@ pci_driver_unbind(struct pciDriver *driver, }

/* Make symlink under device tree */ - if (virAsprintfQuiet(&devpath, "%s/sys/bus/pci/devices/%s/driver", - fakerootdir, dev->id) < 0 || + if (!(devpath = pci_device_get_path(dev, "driver", true)) || virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s/%s", fakerootdir, driver->name, dev->id) < 0) { errno = ENOMEM;

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

Have just one function to generate path to a PCI driver so that when we change it in near future there's only few of the places we need to fix.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 52a585d0cc..de365cdb78 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -503,6 +503,29 @@ pci_device_autobind(struct pciDevice *dev) /* * PCI Driver functions */ +static char * +pci_driver_get_path(const struct pciDriver *driver, + const char *file, + bool faked) +{ + char *ret = NULL; + const char *prefix = ""; + + if (faked) + prefix = fakerootdir; + + if (file) { + ignore_value(virAsprintfQuiet(&ret, "%s" SYSFS_PCI_PREFIX "drivers/%s/%s", + prefix, driver->name, file)); + } else { + ignore_value(virAsprintfQuiet(&ret, "%s" SYSFS_PCI_PREFIX "drivers/%s", + prefix, driver->name)); + } + + return ret; +} + + static void pci_driver_new(const char *name, int fail, ...) { @@ -513,7 +536,7 @@ pci_driver_new(const char *name, int fail, ...)

if (VIR_ALLOC_QUIET(driver) < 0 || VIR_STRDUP_QUIET(driver->name, name) < 0 || - virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s", fakerootdir, name) < 0) + !(driverpath = pci_driver_get_path(driver, NULL, true))) ABORT_OOM();

driver->fail = fail; @@ -619,8 +642,7 @@ pci_driver_bind(struct pciDriver *driver,

/* Make symlink under device tree */ if (!(devpath = pci_device_get_path(dev, "driver", true)) || - virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s", - fakerootdir, driver->name) < 0) { + !(driverpath = pci_driver_get_path(driver, NULL, true))) { errno = ENOMEM; return -1; } @@ -632,8 +654,7 @@ pci_driver_bind(struct pciDriver *driver, VIR_FREE(devpath); VIR_FREE(driverpath); if (!(devpath = pci_device_get_path(dev, NULL, true)) || - virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s/%s", - fakerootdir, driver->name, dev->id) < 0) { + !(driverpath = pci_driver_get_path(driver, dev->id, true))) { errno = ENOMEM; return -1; } @@ -660,8 +681,7 @@ pci_driver_unbind(struct pciDriver *driver,

/* Make symlink under device tree */ if (!(devpath = pci_device_get_path(dev, "driver", true)) || - virAsprintfQuiet(&driverpath, "%s/sys/bus/pci/drivers/%s/%s", - fakerootdir, driver->name, dev->id) < 0) { + !(driverpath = pci_driver_get_path(driver, dev->id, true))) { errno = ENOMEM; return -1; }

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

In upcoming patches we will need only some portions of the PCI address. To construct that easily, it's better if the PCI address of a device is stored as four integers rather than one string.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 76 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 61 insertions(+), 15 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c index de365cdb78..c10764dcdd 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -118,8 +118,16 @@ struct pciDriver { unsigned int fail; /* Bitwise-OR of driverActions that should fail */ };

+struct pciDeviceAddress { + unsigned int domain; + unsigned int bus; + unsigned int device; + unsigned int function; +}; +# define ADDR_STR_FMT "%04x:%02x:%02x.%d" +

I was going to complain "this stuff is similar to what we already have in utils/virpci.c, why can't we use it here", but in a second thought I realized virpci.h is too big of a import just for a macro and a couple of parse functions. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

struct pciDevice { - const char *id; + struct pciDeviceAddress addr; int vendor; int device; int klass; @@ -145,7 +153,7 @@ static void init_env(void);

static int pci_device_autobind(struct pciDevice *dev); static void pci_device_new_from_stub(const struct pciDevice *data); -static struct pciDevice *pci_device_find_by_id(const char *id); +static struct pciDevice *pci_device_find_by_id(struct pciDeviceAddress const *addr); static struct pciDevice *pci_device_find_by_content(const char *path);

static void pci_driver_new(const char *name, int fail, ...); @@ -337,6 +345,29 @@ remove_fd(int fd) /* * PCI Device functions */ +static char * +pci_address_format(struct pciDeviceAddress const *addr) +{ + char *ret; + + ignore_value(virAsprintfQuiet(&ret, ADDR_STR_FMT, + addr->domain, addr->bus, + addr->device, addr->function)); + return ret; +} + +static int +pci_address_parse(struct pciDeviceAddress *addr, + const char *buf) +{ + if (sscanf(buf, ADDR_STR_FMT, + &addr->domain, &addr->bus, + &addr->device, &addr->function) != 4) + return -1; + return 0; +} + + static char * pci_device_get_path(const struct pciDevice *dev, const char *file, @@ -344,16 +375,20 @@ pci_device_get_path(const struct pciDevice *dev, { char *ret = NULL; const char *prefix = ""; + VIR_AUTOFREE(char *) devid = NULL;

if (faked) prefix = fakerootdir;

+ if (!(devid = pci_address_format(&dev->addr))) + return NULL; + if (file) { ignore_value(virAsprintfQuiet(&ret, "%s" SYSFS_PCI_PREFIX "devices/%s/%s", - prefix, dev->id, file)); + prefix, devid, file)); } else { ignore_value(virAsprintfQuiet(&ret, "%s" SYSFS_PCI_PREFIX "devices/%s", - prefix, dev->id)); + prefix, devid)); }

return ret; @@ -366,13 +401,15 @@ pci_device_new_from_stub(const struct pciDevice *data) struct pciDevice *dev; VIR_AUTOFREE(char *) devpath = NULL; VIR_AUTOFREE(char *) id = NULL; + VIR_AUTOFREE(char *) devid = NULL; char *c; VIR_AUTOFREE(char *) configSrc = NULL; char tmp[256]; struct stat sb; bool configSrcExists = false;

- if (VIR_STRDUP_QUIET(id, data->id) < 0) + if (!(devid = pci_address_format(&data->addr)) || + VIR_STRDUP_QUIET(id, devid) < 0) ABORT_OOM();

/* Replace ':' with '-' to create the config filename from the @@ -452,20 +489,20 @@ pci_device_new_from_stub(const struct pciDevice *data) make_symlink(devpath, "iommu_group", tmp);

if (pci_device_autobind(dev) < 0) - ABORT("Unable to bind: %s", data->id); + ABORT("Unable to bind: %s", devid);

if (VIR_APPEND_ELEMENT_QUIET(pciDevices, nPCIDevices, dev) < 0) ABORT_OOM(); }

static struct pciDevice * -pci_device_find_by_id(const char *id) +pci_device_find_by_id(struct pciDeviceAddress const *addr) { size_t i; for (i = 0; i < nPCIDevices; i++) { struct pciDevice *dev = pciDevices[i];

- if (STREQ(dev->id, id)) + if (!memcmp(&dev->addr, addr, sizeof(*addr))) return dev; }

@@ -476,11 +513,13 @@ static struct pciDevice * pci_device_find_by_content(const char *path) { char tmp[32]; + struct pciDeviceAddress addr;

- if (pci_read_file(path, tmp, sizeof(tmp), true) < 0) + if (pci_read_file(path, tmp, sizeof(tmp), true) < 0 || + pci_address_parse(&addr, tmp) < 0) return NULL;

- return pci_device_find_by_id(tmp); + return pci_device_find_by_id(&addr); }

static int @@ -607,6 +646,7 @@ pci_driver_find_by_path(const char *path) static struct pciDriver * pci_driver_find_by_driver_override(struct pciDevice *dev) { + VIR_AUTOFREE(char *) devid = NULL; VIR_AUTOFREE(char *) path = NULL; char tmp[32]; size_t i; @@ -631,6 +671,7 @@ static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev) { + VIR_AUTOFREE(char *) devid = NULL; VIR_AUTOFREE(char *) devpath = NULL; VIR_AUTOFREE(char *) driverpath = NULL;

@@ -653,8 +694,9 @@ pci_driver_bind(struct pciDriver *driver, /* Make symlink under driver tree */ VIR_FREE(devpath); VIR_FREE(driverpath); - if (!(devpath = pci_device_get_path(dev, NULL, true)) || - !(driverpath = pci_driver_get_path(driver, dev->id, true))) { + if (!(devid = pci_address_format(&dev->addr)) || + !(devpath = pci_device_get_path(dev, NULL, true)) || + !(driverpath = pci_driver_get_path(driver, devid, true))) { errno = ENOMEM; return -1; } @@ -670,6 +712,7 @@ static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev) { + VIR_AUTOFREE(char *) devid = NULL; VIR_AUTOFREE(char *) devpath = NULL; VIR_AUTOFREE(char *) driverpath = NULL;

@@ -680,8 +723,9 @@ pci_driver_unbind(struct pciDriver *driver, }

/* Make symlink under device tree */ - if (!(devpath = pci_device_get_path(dev, "driver", true)) || - !(driverpath = pci_driver_get_path(driver, dev->id, true))) { + if (!(devid = pci_address_format(&dev->addr)) || + !(devpath = pci_device_get_path(dev, "driver", true)) || + !(driverpath = pci_driver_get_path(driver, devid, true))) { errno = ENOMEM; return -1; } @@ -913,8 +957,10 @@ init_env(void)

# define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \ do { \ - struct pciDevice dev = {.id = Id, .vendor = Vendor, \ + struct pciDevice dev = {.vendor = Vendor, \ .device = Device, __VA_ARGS__}; \ + if (pci_address_parse(&dev.addr, Id) < 0) \ + ABORT("Unable to parse PCI address " Id); \ pci_device_new_from_stub(&dev); \ } while (0)

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> 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 <mprivozn@redhat.com> --- 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, &hdrType) < 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, &data)) + 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, &data) < 0) goto cleanup; - } else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) { - goto cleanup; } }

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

The pci-assign device is so old school that no one uses it. All modern systems have adapted VFIO. Switch our xml2argv test too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- .../hostdev-pci-address-device.args | 2 +- tests/qemuxml2argvdata/hostdev-pci-address.args | 2 +- tests/qemuxml2argvdata/net-hostdev-bootorder.args | 3 +-- .../qemuxml2argvdata/net-hostdev-multidomain.args | 2 +- tests/qemuxml2argvdata/net-hostdev.args | 2 +- tests/qemuxml2argvdata/pci-rom.args | 4 ++-- tests/qemuxml2argvtest.c | 14 +++++++------- 7 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/tests/qemuxml2argvdata/hostdev-pci-address-device.args b/tests/qemuxml2argvdata/hostdev-pci-address-device.args index 5159b00ef1..79654f44bb 100644 --- a/tests/qemuxml2argvdata/hostdev-pci-address-device.args +++ b/tests/qemuxml2argvdata/hostdev-pci-address-device.args @@ -27,5 +27,5 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --device pci-assign,host=0000:06:12.5,id=hostdev0,bus=pci.0,addr=0x3 \ +-device vfio-pci,host=0000:06:12.5,id=hostdev0,bus=pci.0,addr=0x3 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/hostdev-pci-address.args b/tests/qemuxml2argvdata/hostdev-pci-address.args index fe6acaaf62..0a57a8f29e 100644 --- a/tests/qemuxml2argvdata/hostdev-pci-address.args +++ b/tests/qemuxml2argvdata/hostdev-pci-address.args @@ -27,4 +27,4 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --device pci-assign,host=0000:06:12.5,id=hostdev0,bus=pci.0,addr=0x3 +-device vfio-pci,host=0000:06:12.5,id=hostdev0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/net-hostdev-bootorder.args b/tests/qemuxml2argvdata/net-hostdev-bootorder.args index eefc247eed..515b0c58d3 100644 --- a/tests/qemuxml2argvdata/net-hostdev-bootorder.args +++ b/tests/qemuxml2argvdata/net-hostdev-bootorder.args @@ -27,5 +27,4 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=2 \ --device pci-assign,host=0000:03:07.1,id=hostdev0,bootindex=1,bus=pci.0,\ -addr=0x3 +-device vfio-pci,host=0000:03:07.1,id=hostdev0,bootindex=1,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/net-hostdev-multidomain.args b/tests/qemuxml2argvdata/net-hostdev-multidomain.args index bf7686f49c..18c5e98188 100644 --- a/tests/qemuxml2argvdata/net-hostdev-multidomain.args +++ b/tests/qemuxml2argvdata/net-hostdev-multidomain.args @@ -27,5 +27,5 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --device pci-assign,host=2424:21:1c.6,id=hostdev0,bus=pci.0,addr=0x3 \ +-device vfio-pci,host=2424:21:1c.6,id=hostdev0,bus=pci.0,addr=0x3 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/net-hostdev.args b/tests/qemuxml2argvdata/net-hostdev.args index 94eac176f3..aa9e91db82 100644 --- a/tests/qemuxml2argvdata/net-hostdev.args +++ b/tests/qemuxml2argvdata/net-hostdev.args @@ -27,5 +27,5 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --device pci-assign,host=0000:03:07.1,id=hostdev0,bus=pci.0,addr=0x3 \ +-device vfio-pci,host=0000:03:07.1,id=hostdev0,bus=pci.0,addr=0x3 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/pci-rom.args b/tests/qemuxml2argvdata/pci-rom.args index 7235642ee8..4a5dc4161a 100644 --- a/tests/qemuxml2argvdata/pci-rom.args +++ b/tests/qemuxml2argvdata/pci-rom.args @@ -33,7 +33,7 @@ addr=0x3,rombar=1 \ -netdev user,id=hostnet1 \ -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:24:a5:9e,bus=pci.0,\ addr=0x4,romfile=/etc/fake/bootrom.bin \ --device pci-assign,host=0000:06:12.5,id=hostdev0,bus=pci.0,addr=0x5,rombar=0 \ --device pci-assign,host=0000:06:12.6,id=hostdev1,bus=pci.0,addr=0x6,rombar=1,\ +-device vfio-pci,host=0000:06:12.5,id=hostdev0,bus=pci.0,addr=0x5,rombar=0 \ +-device vfio-pci,host=0000:06:12.6,id=hostdev1,bus=pci.0,addr=0x6,rombar=1,\ romfile=/etc/fake/bootrom.bin \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c166fd18d6..26b512d246 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -496,7 +496,7 @@ testCompareXMLToArgv(const void *data) if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && hostdev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { - hostdev->source.subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM; + hostdev->source.subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO; } }

@@ -1303,9 +1303,9 @@ mymain(void) DO_TEST("net-many-models", NONE); DO_TEST("net-mcast", NONE); DO_TEST("net-udp", NONE); - DO_TEST("net-hostdev", NONE); - DO_TEST("net-hostdev-bootorder", NONE); - DO_TEST("net-hostdev-multidomain", NONE); + DO_TEST("net-hostdev", QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("net-hostdev-bootorder", QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("net-hostdev-multidomain", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("net-hostdev-vfio", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("net-hostdev-vfio-multidomain", @@ -1576,8 +1576,8 @@ mymain(void) DO_TEST("hostdev-usb-address", NONE); DO_TEST("hostdev-usb-address-device", NONE); DO_TEST("hostdev-usb-address-device-boot", NONE); - DO_TEST("hostdev-pci-address", NONE); - DO_TEST("hostdev-pci-address-device", NONE); + DO_TEST("hostdev-pci-address", QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("hostdev-pci-address-device", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("hostdev-vfio", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("hostdev-vfio-multidomain", @@ -1613,7 +1613,7 @@ mymain(void) QEMU_CAPS_DEVICE_ZPCI); DO_TEST_PARSE_ERROR("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_VFIO_PCI); - DO_TEST("pci-rom", NONE); + DO_TEST("pci-rom", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE);

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

The pci-stub is so old school that no one uses it. All modern systems have adapted VFIO. Switch our virhostdevtest too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 20984f3442..f860426678 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -96,7 +96,7 @@ myInit(void) subsys.u.pci.addr.bus = 0; subsys.u.pci.addr.slot = i + 1; subsys.u.pci.addr.function = 0; - subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM; + subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO; hostdevs[i]->source.subsys = subsys; }

@@ -104,7 +104,7 @@ myInit(void) if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) goto cleanup;

- virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); }

if (VIR_ALLOC(mgr) < 0)

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 8/14/19 8:57 AM, Michal Privoznik wrote:

The pci-stub is so old school that no one uses it. All modern systems have adapted VFIO. Switch our virpcitest too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcitest.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 9ecd1b7d27..0bd37268ef 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -106,12 +106,12 @@ testVirPCIDeviceDetach(const void *opaque ATTRIBUTE_UNUSED) if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) goto cleanup;

- virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);

if (virPCIDeviceDetach(dev[i], activeDevs, inactiveDevs) < 0) goto cleanup;

- if (testVirPCIDeviceCheckDriver(dev[i], "pci-stub") < 0) + if (testVirPCIDeviceCheckDriver(dev[i], "vfio-pci") < 0) goto cleanup;

CHECK_LIST_COUNT(activeDevs, 0); @@ -147,7 +147,7 @@ testVirPCIDeviceReset(const void *opaque ATTRIBUTE_UNUSED) if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) goto cleanup;

- virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);

if (virPCIDeviceReset(dev[i], activeDevs, inactiveDevs) < 0) goto cleanup; @@ -187,7 +187,7 @@ testVirPCIDeviceReattach(const void *opaque ATTRIBUTE_UNUSED) CHECK_LIST_COUNT(activeDevs, 0); CHECK_LIST_COUNT(inactiveDevs, i + 1);

- virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); }

CHECK_LIST_COUNT(activeDevs, 0); @@ -245,7 +245,7 @@ testVirPCIDeviceDetachSingle(const void *opaque) if (!dev) goto cleanup;

- virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO);

if (virPCIDeviceDetach(dev, NULL, NULL) < 0) goto cleanup; @@ -405,7 +405,7 @@ mymain(void) /* Detach an unbound device */ DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, NULL); DO_TEST_PCI(testVirPCIDeviceDetachSingle, 0, 0x0a, 2, 0); - DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, "pci-stub"); + DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, "vfio-pci");

/* Reattach an unknown unbound device */ DO_TEST_PCI_DRIVER(0, 0x0a, 3, 0, NULL);