On Sun, Mar 26, 2017 at 02:25:02PM -0400, Laine Stump wrote:
On 03/22/2017 11:27 AM, Erik Skultety wrote:
Label the VFIO IOMMU devices under /dev/vfio/ referenced by the symlinks in the sysfs (e.g. /sys/class/mdev_bus/<uuid>/iommu_group) which what qemu actually gets formatted on the command line.
The sentence above is confused (i.e. I don't understand it), but I won't know how to unconfuse it until I've gone through the patch.
Now that I'm reading it again, of course I know what I meant, but that's only because I wrote it, but from the native speaker's perspective, I guess the reaction must have been something like "wut?!". So I simplified it to the bare minimum in terms of the patch just adding labeling for mdevs as well. [...]
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { + char *vfiodev = NULL; + virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr, + mdevsrc->model); + + if (!mdev) + goto done; + + if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) { + virMediatedDeviceFree(mdev); + goto done; + }
Going through the various patches and seeing this (or similar) sequences so often makes me think it might be cleaner to have APIs that take a uuidstr and model instead (or maybe define virDomainHostdevSubsysMediatedDevPtr in util instead of conf, then pass the mdevsrc directly - that would make it continue to work if/once we add different ways to specify the device.
But things currently work exactly the same way for PCI devices, so no sense rewriting just for that. These are all internal APIs, so we can tweak them to our hearts' content in the future.
Yeah, there's definitely some room for 'refurbishment' of the hostdev code. I'll put that on my TODO list.
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { + char *vfiodev = NULL; + virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr, + mdevsrc->model); + + if (!mdev) + goto done; + + if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) { + virMediatedDeviceFree(mdev); + goto done; + }
(see what I mean?)
Yep.