3:12 a.m.
On 04/19/2013 01:02 AM, Eric Blake wrote:
On 04/17/2013 01:00 PM, Ján Tomko wrote:
> @@ -1321,7 +1363,39 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> qemuDomainObjPrivatePtr priv = NULL;
>
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> - if (!(addrs = qemuDomainPCIAddressSetCreate(def)))
> + int nbuses = 1;
> + int i;
> + int rv;
> +
> + for (i = 0; i < def->ncontrollers; i++) {
> + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI
&&
> + def->controllers[i]->model ==
VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)
> + nbuses++;
> + }
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
> + virDomainDeviceInfo info;
> + /* 1st pass to figure out how many PCI bridges we need */
> + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
> + goto cleanup;
> + if (nbuses && qemuAssignDevicePCISlots(def, qemuCaps, addrs)
< 0)
> + goto cleanup;
> + /* Reserve 1 extra slot for a (potential) bridge */
> + if (qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0)
> + goto cleanup;
> +
> + for (i = 1; i < addrs->nbuses; i++) {
> + if ((rv = virDomainDefMaybeAddController(
> + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
> + i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)) < 0)
> + goto cleanup;
> + /* If we added a new bridge, we will need one more address */
> + if (rv > 0 && qemuDomainPCIAddressSetNextAddr(addrs,
&info) < 0)
> + goto cleanup;
> + }
> + qemuDomainPCIAddressSetFree(addrs);
> + addrs = NULL;
> + }
Do we need an else that fails if we have no pci bridge support, but more
than one pci controller is present?
I'd rather disallow multiple PCI controllers with the same index (and
check if pci-root has index 0).
Specifying two bridges with indexes 1 and 3 also doesn't mean that buses
0-2 are available (which is what my code above would think if bus 3
would be unused).
And it would also be nice to add them to qemu command line without
referencing bridges that don't exist yet by requiring that bus < index
in the bridge address and inserting them in order (but I'm worried that
doing so in virDomainDefMaybeAddController might break other things).
> + if (!(addrs = qemuDomainPCIAddressSetCreate(def,
addrs->nbuses, false)))
> goto cleanup;
>
> if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> @@ -1503,30 +1593,44 @@
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr next_addr)
> {
> virDevicePCIAddress tmp_addr = addrs->lastaddr;
> - int i;
> + int i,j;
Space after comma.
> char *addr;
>
> tmp_addr.slot++;
> - for (i = 0; i < QEMU_PCI_ADDRESS_SLOT_LAST; i++, tmp_addr.slot++) {
> - if (QEMU_PCI_ADDRESS_SLOT_LAST <= tmp_addr.slot) {
> - tmp_addr.slot = 0;
> - }
> + for (j = 0; j < addrs->nbuses; j++) {
> + for (i = 0; i < QEMU_PCI_ADDRESS_SLOT_LAST; i++, tmp_addr.slot++) {
> + if (QEMU_PCI_ADDRESS_SLOT_LAST <= tmp_addr.slot) {
> + /* slot 0 is unusable */
> + tmp_addr.slot = 1;
> + i++;
> + }
I found this use of 'i' a bit confusing - any time you change the
iteration conditions inside the loop body, it becomes trickier to follow
the logic. I think you got it right, in that basically you want to
iterate over 31 slots, instead of 32 (by skipping slot 0), but where the
slot you probe starts from addrs->lastaddr and wraps around rather than
starting at 1.
Would it be any clearer if the inner loop iterated over i=1; i <
SLOT_LAST, so that the only increment of i is in the loop header?
Yes, that would make it slightly less disgusting. Or maybe i=0; i <
SLOT_LAST-1?
Also, I think I see why keeping this cache logic is essential for bus 0
for back-compat reasons (we want to avoid hot-plugging a new device into
a previously-used but now unplugged slot, at least until we have run out
of unique slots to plug into, in order to minimize guest confusion about
different devices trying to reuse a slot). But do we really need to
extend it to bus 1? That is, if one call fills the last slot of bus 0
and sets addrs->lastaddr to 5, do we really want the next call to start
at slot 5 of bus 1, or even though nothing has ever probed slots 1-4 of
bus 1? I'm worried that your caching of last-used slot is not quite
right; in fact, I wonder if you need to cache both last-used bus and
last-used slot on that bus, or even last-used slot on all buses, rather
than just a single last-used slot without relation to bus.
It does cache the domain, bus and slot.
>
> - if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
> - return -1;
> + if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
> + return -1;
>
> - if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
> - VIR_DEBUG("PCI addr %s already in use", addr);
> - VIR_FREE(addr);
> - continue;
> - }
> + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
> + VIR_DEBUG("PCI addr %s already in use", addr);
> + VIR_FREE(addr);
> + continue;
> + }
>
> - VIR_DEBUG("Found free PCI addr %s", addr);
> - VIR_FREE(addr);
> + VIR_DEBUG("Found free PCI addr %s", addr);
> + VIR_FREE(addr);
>
> - addrs->lastaddr = tmp_addr;
> - *next_addr = tmp_addr;
> - return 0;
> + addrs->lastaddr = tmp_addr;
> + *next_addr = tmp_addr;
> + return 0;
Again, thinking about last-used slot, it seems like if this filled up
all available slots on the bus, wouldn't it be better to set
addrs->lastaddr to 1 (to start the next bus correctly)? Does the outer
loop (on 'j') need to start iterating from the last-used bus instead of
starting at bus 0 and finding all 31 slots full?
j is just there to make sure we stop if all the buses are full.
tmp_addr.bus will be set to whatever bus was in lastaddr and yes,
advancing to the next bus and starting from slot 1 would make more
sense. And it might even make the loop look like it's from this planet.
Or is caching the last-used slot something that we can completely avoid
in the case of a multibus support, and only worry about it for older qemu?
Without it, we'd need to go over all the buses every time a new address
is needed. But I'm not sure if we want to cache reservation of slot 7
when slot 6 is empty (which could only happen with hotplug, since
addresses from the XML config are not cached and the rest is assigned
sequentially).
(Btw: I accidentally removed the caching from ReserveSlot in 5/11 which
is where explicitly specified addresses of hotplugged devices are cached).
> + }
> + tmp_addr.bus++;
> + if (addrs->nbuses <= tmp_addr.bus) {
> + if (addrs->dryRun) {
> + if (qemuPCIAddressSetGrow(addrs, &tmp_addr) < 0)
> + return -1;
> + } else {
> + tmp_addr.bus = 0;
> + }
> + tmp_addr.slot = 1;
> + }
> }
>
> virReportError(VIR_ERR_INTERNAL_ERROR,