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 <mpriv...@redhat.com> 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 <sb...@linux.vnet.ibm.com>
>> ---
>> 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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
