On 04/17/2013 01:00 PM, Ján Tomko wrote:
> Add a "dry run" address allocation to figure out how many bridges
> will be needed for all the devices without explicit addresses.
>
> Auto-add just enough bridges to put all the devices on, or up to the
> bridge with the largest specified index.
> ---
> src/conf/domain_conf.c | 9 +--
> src/conf/domain_conf.h | 5 ++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 177 +++++++++++++++++++++++++++++++++++++++--------
> src/qemu/qemu_command.h | 4 +-
> 5 files changed, 161 insertions(+), 35 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a5764fb..68518a7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9762,10 +9762,11 @@ virDomainLookupVcpuPin(virDomainDefPtr def,
> return NULL;
> }
>
> -static int virDomainDefMaybeAddController(virDomainDefPtr def,
> - int type,
> - int idx,
> - int model)
> +int
> +virDomainDefMaybeAddController(virDomainDefPtr def,
> + int type,
> + int idx,
> + int model)
I wonder if it's worth squashing the indentation change into 7/11,
leaving only the s/static// for this patch. Then again, it's the same
net number of line changes.
> +++ b/src/qemu/qemu_command.c
> @@ -1195,6 +1195,9 @@ cleanup:
> typedef uint8_t qemuDomainPCIAddressBus[QEMU_PCI_ADDRESS_SLOT_LAST];
> struct _qemuDomainPCIAddressSet {
> qemuDomainPCIAddressBus *used;
> + size_t nbuses; /* allocation of used */
> + bool dryRun; /* on a dry run, new buses are auto-added
> + and addresses aren't saved in device infos */
> virDevicePCIAddress lastaddr;
> };
>
> @@ -1211,9 +1214,10 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
> _("Only PCI domain 0 is available"));
> return false;
> }
> - if (addr->bus != 0) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("Only PCI bus 0 is available"));
> + if (addr->bus >= addrs->nbuses) {
> + virReportError(VIR_ERR_XML_ERROR, _("Only PCI buses up to %u are"
> + " available"),
If you write this as:
if (addr->bus >= addrs->nbuses) {
virReportError(VIR_ERR_XML_ERROR,
_("Only PCI buses up to %u are available"),
then you don't have to split the string.
> + (unsigned int) addrs->nbuses - 1);
Instead of casting, use %zu (since addrs->nbuses is size_t).
>
> +/* grows the address set to fit addr in
I did a double-take reading that; how about:
Ensure addr fits in the address set, by expanding it if needed.
> + * -1 = OOM
> + * 0 = no action required
> + * >0 = number of buses added
> + */
> +static int qemuPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
> + virDevicePCIAddressPtr addr)
static int
qemuPCIAddressSetGrow(...
> @@ -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?
> + if (!(addrs = qemuDomainPCIAddressSetCreate(def, addrs->nbuses, false)))
> goto cleanup;
>
> if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> @@ -1366,15 +1440,25 @@ int qemuDomainAssignAddresses(virDomainDefPtr def,
> return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
> }
>
> -qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def)
> +qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
> + unsigned int nbuses,
> + bool dryRun)
> {
> qemuDomainPCIAddressSetPtr addrs;
> + int i;
>
> if (VIR_ALLOC(addrs) < 0)
> goto no_memory;
>
> - if (VIR_ALLOC_N(addrs->used, 1) < 0)
> + if (VIR_ALLOC_N(addrs->used, nbuses) < 0)
> goto no_memory;
> + addrs->nbuses = nbuses;
> + addrs->dryRun = dryRun;
> +
> + /* reserve slot 0 in every bus - it's used by the host bridge on bus 0
> + * and unusable on PCI bridges */
> + for (i = 0; i < nbuses; i++)
> + addrs->used[i][0] = 0xFF;
>
> if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0)
> goto error;
> @@ -1402,6 +1486,9 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
> {
> char *str;
>
> + if (addrs->dryRun && qemuPCIAddressSetGrow(addrs, addr) < 0)
> + return -1;
> +
> if (!(str = qemuPCIAddressAsString(addr)))
> return -1;
>
> @@ -1428,6 +1515,9 @@ int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
> {
> char *str;
>
> + if (addrs->dryRun && qemuPCIAddressSetGrow(addrs, addr) < 0)
> + return -1;
> +
> if (!(str = qemuPCIAddressAsString(addr)))
> return -1;
>
> @@ -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?
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.
>
> - 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?
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?
> + }
> + 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,
> @@ -1544,8 +1648,10 @@ int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
I didn't spot any other logic questions, but making sure we get next
address allocation done correctly is probably still worth a v3.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list