Thanks for the comments Michal.. I'll fix them all in my next version.
Regards,
Shivaprasad
On Mon, Nov 23, 2015 at 10:34 PM, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
> On 14.11.2015 09:38, Shivaprasad G Bhat wrote:
>> The iommu group info can be used to check if the devices are bound/unbound
>> from vfio at the group level granualrity. Add the info to mock as to help
>> add test cases later.
>>
>> Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
>> ---
>> tests/virpcimock.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 164 insertions(+), 16 deletions(-)
>>
>> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
>> index 0724a36..837976a 100644
>> --- a/tests/virpcimock.c
>> +++ b/tests/virpcimock.c
>> @@ -127,9 +127,15 @@ struct pciDevice {
>> int vendor;
>> int device;
>> int class;
>> + int iommu;
>> struct pciDriver *driver; /* Driver attached. NULL if attached to no driver */
>> };
>>
>> +struct pciIommuGroup {
>> + int iommu;
>> + size_t nDevicesBoundToVFIO; /* Indicates the devices in the group */
>> +};
>> +
>> struct fdCallback {
>> int fd;
>> char *path;
>> @@ -141,6 +147,9 @@ size_t nPCIDevices = 0;
>> struct pciDriver **pciDrivers = NULL;
>> size_t nPCIDrivers = 0;
>>
>> +struct pciIommuGroup **pciIommuGroups = NULL;
>> +size_t npciIommuGroups = 0;
>> +
>> struct fdCallback *callbacks = NULL;
>> size_t nCallbacks = 0;
>>
>> @@ -325,6 +334,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
>> char *configSrc;
>> char tmp[32];
>> struct stat sb;
>> + char *iommugrouppath, *deviommupath, *iommugroupdevs = NULL;
>>
>> if (VIR_STRDUP_QUIET(id, data->id) < 0)
>> ABORT_OOM();
>> @@ -387,6 +397,25 @@ pci_device_new_from_stub(const struct pciDevice *data)
>> ABORT("@tmp overflow");
>> make_file(devpath, "class", tmp, -1);
>>
>> + if (virAsprintfQuiet(&deviommupath, "%s/iommu_group", devpath) < 0 ||
>> + virAsprintfQuiet(&iommugrouppath, "%s/iommu_groups/%d",
>> + fakesysfsdir, dev->iommu) < 0)
>> + ABORT("@deviommupath overflow");
>
> Technically this is not overflow rather than OOM. I guess you've just copied a pattern just above these lines (not to be seen in this patch though), where we really are snprintf()-ing into 32 bytes wide string. Here we have all the memory, so this should be ABORT_OOM();
>
>> +
>> + if (symlink(iommugrouppath, deviommupath) < 0)
>> + ABORT("Unable to link device to iommu group");
>> +
>> + VIR_FREE(deviommupath);
>> + if (virAsprintfQuiet(&iommugroupdevs, "%s/devices/%s",
>> + iommugrouppath, dev->id) < 0)
>> + ABORT("@iommugroupdevs overflow");
>
> Same here.
>
>> +
>> + if (symlink(devpath, iommugroupdevs) < 0)
>> + ABORT("Unable to link iommu group devices to current device");
>> +
>> + VIR_FREE(iommugrouppath);
>> + VIR_FREE(iommugroupdevs);
>> +
>> if (pci_device_autobind(dev) < 0)
>> ABORT("Unable to bind: %s", data->id);
>>
>> @@ -435,7 +464,95 @@ pci_device_autobind(struct pciDevice *dev)
>> return pci_driver_bind(driver, dev);
>> }
>>
>> +static void
>> +pci_iommu_new(int num)
>> +{
>> + char *iommupath;
>> + struct pciIommuGroup *iommuGroup;
>> +
>> + if (VIR_ALLOC_QUIET(iommuGroup) < 0)
>> + ABORT_OOM();
>> +
>> + iommuGroup->iommu = num;
>> + iommuGroup->nDevicesBoundToVFIO = 0; /* No device bound to vfio by default */
>> +
>> + if (virAsprintfQuiet(&iommupath, "%s/iommu_groups/%d/devices", fakesysfsdir, num) < 0)
>> + ABORT_OOM();
>> +
>> + if (virFileMakePath(iommupath) < 0)
>> + ABORT("Unable to create: %s", iommupath);
>> +
>> + if (VIR_APPEND_ELEMENT_QUIET(pciIommuGroups, npciIommuGroups, iommuGroup) < 0)
>> + ABORT_OOM();
>
> @iommupath is no longer needed and leaked.
>
>> +}
>> +
>> +static int
>> +pci_vfio_release_iommu(struct pciDevice *device)
>> +{
>> + char *vfiopath = NULL;
>> + int ret = -1;
>> + size_t i = 0;
>> +
>> + for (i = 0; i < npciIommuGroups; i++) {
>> + if (pciIommuGroups[i]->iommu == device->iommu)
>> + break;
>> + }
>> +
>> + if (i != npciIommuGroups) {
>> + if (pciIommuGroups[i]->nDevicesBoundToVFIO == 0) {
>> + ret = 0;
>> + goto cleanup;
>> + }
>
> This is somewhat confusing to me. This can happen due to a bug in our code - in that case I'd expect an error to be thrown.
>
>> + pciIommuGroups[i]->nDevicesBoundToVFIO--;
>> + if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
>> + if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
>> + fakesysfsdir, device->iommu) < 0) {
>> + errno = ENOMEM;
>> + goto cleanup;
>> + }
>> + if (unlink(vfiopath) < 0)
>> + goto cleanup;
>> + }
>> + }
>> +
>> + ret = 0;
>> + cleanup:
>> + VIR_FREE(vfiopath);
>> + return ret;
>> +}
>> +
>> +static int
>> +pci_vfio_lock_iommu(struct pciDevice *device)
>> +{
>> + char *vfiopath = NULL;
>> + int ret = -1;
>> + size_t i = 0;
>> + int fd = -1;
>> +
>> + for (i = 0; i < npciIommuGroups; i++) {
>> + if (pciIommuGroups[i]->iommu == device->iommu)
>> + break;
>> + }
>>
>> + if (i != npciIommuGroups) {
>> + if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
>> + if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
>> + fakesysfsdir, device->iommu) < 0) {
>> + errno = ENOMEM;
>> + goto cleanup;
>> + }
>> + if ((fd = realopen(vfiopath, O_CREAT)) < 0)
>> + goto cleanup;
>> + }
>> + pciIommuGroups[i]->nDevicesBoundToVFIO++;
>> + }
>> +
>> + ret = 0;
>> + cleanup:
>> + realclose(fd);
>> + VIR_FREE(vfiopath);
>> + return ret;
>> +}
>
> Missing empty line.
>
>> /*
>> * PCI Driver functions
>> */
>> @@ -556,6 +673,10 @@ pci_driver_bind(struct pciDriver *driver,
>> if (symlink(devpath, driverpath) < 0)
>> goto cleanup;
>>
>> + if (STREQ(driver->name, "vfio-pci"))
>> + if (pci_vfio_lock_iommu(dev) < 0)
>> + goto cleanup;
>> +
>> dev->driver = driver;
>> ret = 0;
>> cleanup:
>> @@ -590,6 +711,10 @@ pci_driver_unbind(struct pciDriver *driver,
>> unlink(driverpath) < 0)
>> goto cleanup;
>>
>> + if (STREQ(driver->name, "vfio-pci"))
>> + if (pci_vfio_release_iommu(dev) < 0)
>> + goto cleanup;
>> +
>> dev->driver = NULL;
>> ret = 0;
>> cleanup:
>> @@ -801,6 +926,8 @@ init_syms(void)
>> static void
>> init_env(void)
>> {
>> + char *devvfio;
>> +
>> if (fakesysfsdir)
>> return;
>>
>> @@ -809,12 +936,33 @@ init_env(void)
>>
>> make_file(fakesysfsdir, "drivers_probe", NULL, -1);
>>
>> + if (virAsprintfQuiet(&devvfio, "%s/dev/vfio", fakesysfsdir) < 0)
>> + ABORT_OOM();
>> +
>> + if (virFileMakePath(devvfio) < 0)
>> + ABORT("Unable to create: %s", devvfio);
>> + VIR_FREE(devvfio);
>> +
>> + pci_iommu_new(1);
>> + pci_iommu_new(2);
>> + pci_iommu_new(3);
>> + pci_iommu_new(4);
>> + pci_iommu_new(5);
>> + pci_iommu_new(6);
>> + pci_iommu_new(7);
>> + pci_iommu_new(8);
>> + pci_iommu_new(9);
>> + pci_iommu_new(10);
>> + pci_iommu_new(11);
>> +
>> +
>> # define MAKE_PCI_DRIVER(name, ...) \
>> pci_driver_new(name, 0, __VA_ARGS__, -1, -1)
>>
>> - MAKE_PCI_DRIVER("iwlwifi", 0x8086, 0x0044);
>> - MAKE_PCI_DRIVER("i915", 0x8086, 0x0046, 0x8086, 0x0047);
>> + MAKE_PCI_DRIVER("i915", 0x8086, 0x0046, 0x8086, 0x0047, 0x8086, 0x0048, 0x1033, 0x0035, 0x1033, 0x00e0);
>> + MAKE_PCI_DRIVER("iwlwifi", 0x8086, 0x0044, 0x0086, 0x105e, 0x0086, 0x105d);
>
> The whole idea was that we have some PCI devices not attached to any driver. I'd like to keep a device or two that way.
>
>> MAKE_PCI_DRIVER("pci-stub", -1, -1);
>> + MAKE_PCI_DRIVER("vfio-pci", -1, -1);
>> pci_driver_new("pciback", PCI_ACTION_BIND, -1, -1);
>>
>> # define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \
>> @@ -824,20 +972,20 @@ init_env(void)
>> pci_device_new_from_stub(&dev); \
>> } while (0)
>>
>> - MAKE_PCI_DEVICE("0000:00:00.0", 0x8086, 0x0044);
>> - MAKE_PCI_DEVICE("0000:00:01.0", 0x8086, 0x0044);
>> - MAKE_PCI_DEVICE("0000:00:02.0", 0x8086, 0x0046);
>> - MAKE_PCI_DEVICE("0000:00:03.0", 0x8086, 0x0048);
>> - MAKE_PCI_DEVICE("0001:00:00.0", 0x1014, 0x03b9, .class = 0x060400);
>> - MAKE_PCI_DEVICE("0001:01:00.0", 0x8086, 0x105e);
>> - MAKE_PCI_DEVICE("0001:01:00.1", 0x8086, 0x105e);
>> - MAKE_PCI_DEVICE("0005:80:00.0", 0x10b5, 0x8112, .class = 0x060400);
>> - MAKE_PCI_DEVICE("0005:90:01.0", 0x1033, 0x0035);
>> - MAKE_PCI_DEVICE("0005:90:01.1", 0x1033, 0x0035);
>> - MAKE_PCI_DEVICE("0005:90:01.2", 0x1033, 0x00e0);
>> - MAKE_PCI_DEVICE("0000:0a:01.0", 0x8086, 0x0047);
>> - MAKE_PCI_DEVICE("0000:0a:02.0", 0x8286, 0x0048);
>> - MAKE_PCI_DEVICE("0000:0a:03.0", 0x8386, 0x0048);
>> + MAKE_PCI_DEVICE("0000:00:00.0", 0x8086, 0x0044, .iommu = 1);
>> + MAKE_PCI_DEVICE("0000:00:01.0", 0x8086, 0x0044, .iommu = 2);
>> + MAKE_PCI_DEVICE("0000:00:02.0", 0x8086, 0x0046, .iommu = 3);
>> + MAKE_PCI_DEVICE("0000:00:03.0", 0x8086, 0x0048, .iommu = 4);
>> + MAKE_PCI_DEVICE("0001:00:00.0", 0x1014, 0x03b9, .iommu = 5, .class = 0x060400);
>> + MAKE_PCI_DEVICE("0001:01:00.0", 0x8086, 0x105e, .iommu = 6);
>> + MAKE_PCI_DEVICE("0001:01:00.1", 0x8086, 0x105d, .iommu = 6);
>
> Okay, you want two slightly different WiFi cards. Thank God for git show --word-diff.
>
>> + MAKE_PCI_DEVICE("0005:80:00.0", 0x10b5, 0x8112, .iommu = 7, .class = 0x060400);
>> + MAKE_PCI_DEVICE("0005:90:01.0", 0x1033, 0x0035, .iommu = 8);
>> + MAKE_PCI_DEVICE("0005:90:01.1", 0x1033, 0x0035, .iommu = 8);
>> + MAKE_PCI_DEVICE("0005:90:01.2", 0x1033, 0x00e0, .iommu = 8);
>> + MAKE_PCI_DEVICE("0000:0a:01.0", 0x8086, 0x0047, .iommu = 9);
>> + MAKE_PCI_DEVICE("0000:0a:02.0", 0x8286, 0x0048, .iommu = 10);
>> + MAKE_PCI_DEVICE("0000:0a:03.0", 0x8386, 0x0048, .iommu = 11);
>> }
>>
>>
>
> This is the diff that I've came up with so far:
>
> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index 837976a..5ef5eac 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -400,7 +400,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
> if (virAsprintfQuiet(&deviommupath, "%s/iommu_group", devpath) < 0 ||
> virAsprintfQuiet(&iommugrouppath, "%s/iommu_groups/%d",
> fakesysfsdir, dev->iommu) < 0)
> - ABORT("@deviommupath overflow");
> + ABORT_OOM();
>
> if (symlink(iommugrouppath, deviommupath) < 0)
> ABORT("Unable to link device to iommu group");
> @@ -408,7 +408,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
> VIR_FREE(deviommupath);
> if (virAsprintfQuiet(&iommugroupdevs, "%s/devices/%s",
> iommugrouppath, dev->id) < 0)
> - ABORT("@iommugroupdevs overflow");
> + ABORT_OOM();
>
> if (symlink(devpath, iommugroupdevs) < 0)
> ABORT("Unable to link iommu group devices to current device");
> @@ -484,6 +484,8 @@ pci_iommu_new(int num)
>
> if (VIR_APPEND_ELEMENT_QUIET(pciIommuGroups, npciIommuGroups, iommuGroup) < 0)
> ABORT_OOM();
> +
> + VIR_FREE(iommupath);
> }
>
> static int
> @@ -499,10 +501,9 @@ pci_vfio_release_iommu(struct pciDevice *device)
> }
>
> if (i != npciIommuGroups) {
> - if (pciIommuGroups[i]->nDevicesBoundToVFIO == 0) {
> - ret = 0;
> - goto cleanup;
> - }
> + if (pciIommuGroups[i]->nDevicesBoundToVFIO == 0)
> + ABORT("nDevicesBoundToVFIO has unexpected value");
> +
> pciIommuGroups[i]->nDevicesBoundToVFIO--;
> if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
> if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
> @@ -553,6 +554,7 @@ pci_vfio_lock_iommu(struct pciDevice *device)
> VIR_FREE(vfiopath);
> return ret;
> }
> +
> /*
> * PCI Driver functions
> */
>
>
>
> Plus what's needed to have at least one or two device not attached to any driver. I'm okay if you send it as reply that will be squashed in before pushing.
>
> Otherwise looking good. weak ACK.
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list