Re: [libvirt] [PATCH v4 3/4] qemu: Isolate hostdevs on pSeries guests
On Sat, 2017-07-15 at 22:28 -0400, Laine Stump wrote:
> + /* The isolation group for a host device is its IOMMU
group,
> + * increased by one: this is because zero is a valid IOMMU group but
> + * that's also the default isolation group, which we want to save
> + * for emulated devices. Shifting isolation groups for host devices
> + * by one ensures there is no overlap */
Which is fine because this is only used internally - we never actually
try to relate this internal number to the external number.
Of course, the whole reason you're doing this is so that the default
value can be 0, making it easier to initialize objects. Otherwise, we
could make the default group -1 (or UINT_MAX) and then there would be no
confusion (e.g. if someone was looking at the debug log message a few
lines down from here).
So if the simplified initialization of objects is a good tradeoff in
exchange for confusion when looking at debug output which probably
nobody will ever look at, then I guess this works.
I tried pushing for having explicit allocation/initialization
functions for structs rather than using VIR_ALLOC(), which for
all intents and purposes means mandating the default value to
be zero, but there was pushback. Since this alternative
approach ultimately achieves the same result, I thought it
would be best to just drop it.
Another factor to keep in mind is that I took great care in
making sure "isolation groups" is the only name used for the
concept outside of this function, which means unless you're
looking at this specific bit of code you should not even
realize there is a correlation between isolation groups and
IOMMU groups.
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1476,6 +1476,13 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
>
> if (qemuAssignDeviceHostdevAlias(vm->def, &info->alias, -1) < 0)
> goto error;
> +
> + if (qemuDomainIsPSeries(vm->def)) {
> + /* Isolation groups are only relevant for pSeries guests */
> + if (qemuDomainFillDeviceIsolationGroup(vm->def, &dev) < 0)
> + goto error;
> + }
Hotplug of course will work only if a PHB already exists that has the
right iommu group.
It only needs to exist and not have any device with a
different isolation group plugged in: an existing, empty
PHB will simply change its isolation group to accept the
hotplugged device.
--
Andrea Bolognani / Red Hat / Virtualization