1:34 a.m.
On Wed, 2017-06-28 at 14:12 -0400, Laine Stump wrote:
> + /* Figure out whether this is the first device we're
plugging
> + * into the bus by looking at all the slots */
> + firstDevice = true;
> + for (slot = bus->minSlot; slot <= bus->maxSlot; slot++) {
> + if (bus->slot[slot].functions) {
> + firstDevice = false;
> + break;
> + }
> + }
You have the same loop 3 times in the code (although in one case the
bool is called "lastDevice" instead of "firstDevice". How about a
short
utility function called virDomainPCIAddressBusIsEmpty() (or something
like that)?
That makes perfect sense, and I'm not sure why I didn't
think of it myself :/
> + if (firstDevice && !bus->isolationGroupLocked)
{
> + /* The first device decides the isolation group for the
> + * entire bus */
> + bus->isolationGroup = isolationGroup;
> + VIR_DEBUG("PCI bus %.4x:%.2x assigned isolation group %d because of
"
> + "first device %s",
> + addr->domain, addr->bus, isolationGroup, addrStr);
I haven't tried out these patches, but it looks like all this stuff
happens for everybody all the time, not just for pSeries, correct? I
suppose it's an effective NOP if everything is group 0 though, right?
That's the idea :)
For all guests except pSeries, the isolation group for both
devices and controllers will never change from the default,
so it will not influence address allocation.
If it did, either this patch or the next one would cause a
massive test suite churn, which they don't ;)
> + } else if (bus->isolationGroup != isolationGroup
&& fromConfig) {
> + /* If this is not the first function and its isolation group
> + * doesn't match the bus', then it should not be using this
> + * address. However, if the address comes from the user then
> + * we comply with the request and change the isolation group
> + * back to the default (because at that point isolation can't
> + * be guaranteed anymore) */
> + bus->isolationGroup = 0;
> + VIR_DEBUG("PCI bus %.4x:%.2x assigned isolation group %d because of
"
> + "user assigned address %s",
> + addr->domain, addr->bus, isolationGroup, addrStr);
Is this what we want to do? Or is it a bad enough situation that we want
to log an error and fail? (I don't have any idea, that's why I'm asking :-)
If we did that, all existing pSeries guests using hostdevs
would suddenly fail to start :(
Degraded isolation (multiple indipendent hostdevs on the
same PHB) is something we want to avoid but doesn't cause
any harm in practice, you just don't get the advantages of
isolation. It's worked fine until now.
Splitting isolation groups (devices that are in the same
host IOMMU group being assigned to separate PHBs), on the
other hand, is problematic. Maybe I can add try to check
for that situation and error out when it happens.
> + /* If the one we just unplugged was the last device on the
bus,
> + * we can reset the isolation group for the bus so that any
> + * device we'll try to plug in from now on will be accepted */
> + if (lastDevice && !bus->isolationGroupLocked)
> + bus->isolationGroup = 0;
Ah, for awhile I had been thinking that isolationGroupLocked would be
set as soon as an isolationGroup was set for a bus. But I guess instead
it's only set when specifically requested as the bus is added; otherwise
an isolationGroup of 0 means "none selected". (Or is it that "BusIsEmpty
== true && !isolationGroupLocked" means "none selected"?)
Yes, isolationGroupLocked is set when the controller is
created and never changed afterwards.
There's not really a notion of "none selected" for isolation
groups: 0 is not special in that regard, it just happens to
be the default for both devices and controllers.
... Now what I look at that code again, I'm pretty sure
resetting the isolation group for the bus when the last
device is removed is not even needed: it will be changed
back whenever another device is plugged in anyway. I'll
try getting rid of that hunk and verify everything still
works.
> + /* We can only change the isolation group for a bus
when
> + * plugging in the first device; moreover, some buses are
> + * prevented from ever changing it */
> + if (!firstDevice || bus->isolationGroupLocked)
Yeah, how does that 2nd thing happen? (setting isolationGroupLocked on
an empty bus) I see that happens in the next patch for the default
(first) PHB, but will it ever happen otherwise?
That's the only use case at the moment.
We want to assing all emulated devices to the default PHB,
and locking its isolation group to the default one is a way
to achieve that.
--
Andrea Bolognani / Red Hat / Virtualization