Re: [libvirt] [PATCH v4 06/14] security: Enable labeling of vfio mediated devices
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.