On 07/17/2015 02:43 PM, Laine Stump wrote:
> The function that auto-assigns PCI addresses was written with the
> hardcoded assumptions that any PCI bus would have slots available
> starting at 1 and ending at 31. This isn't true for many types of
> controller (some have a single slot/port at 0, some have slots/ports
> from 0 to 31). This patch updates that function to remove the
> hardcoded assumptions. It will properly find/assign addresses for
> devices that can only connect to pcie-(root|downstream)-port (which
> have minSlot/maxSlot of 0/0) or a pcie-switch-upstream-port (0/31).
>
> It still will not auto-create a new bus of the proper kind for these
> connections when one doesn't exist, that task is for another day.
> ---
> new in V2
>
> src/conf/domain_addr.c | 65 +++++++++++++++++++++++++++++---------------------
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 2be98c5..bc09279 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -471,24 +471,30 @@
> virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
> virDomainPCIConnectFlags flags)
> {
> /* default to starting the search for a free slot from
> - * 0000:00:00.0
> + * the first slot of domain 0 bus 0...
> */
> virDevicePCIAddress a = { 0, 0, 0, 0, false };
> char *addrStr = NULL;
>
> - /* except if this search is for the exact same type of device as
> - * last time, continue the search from the previous match
> - */
> - if (flags == addrs->lastFlags)
> - a = addrs->lastaddr;
> -
> if (addrs->nbuses == 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> goto error;
> }
>
> - /* Start the search at the last used bus and slot */
> - for (a.slot++; a.bus < addrs->nbuses; a.bus++) {
> + /* ...unless this search is for the exact same type of device as
> + * last time, then continue the search from the next slot after
> + * the previous match.
next slot and possibly first slot of next bus
> + */
> + if (flags == addrs->lastFlags) {
> + a = addrs->lastaddr;
> + if (++a.slot > addrs->buses[a.bus].maxSlot &&
> + ++a.bus < addrs->nbuses)
> + a.slot = addrs->buses[a.bus].minSlot;
> + } else {
> + a.slot = addrs->buses[0].minSlot;
> + }
> +
> + while (a.bus < addrs->nbuses) {
> if (!(addrStr = virDomainPCIAddressAsString(&a)))
> goto error;
> if (!virDomainPCIAddressFlagsCompatible(&a, addrStr,
> @@ -497,29 +503,33 @@
> virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
> VIR_FREE(addrStr);
I think with the new logic to use "if / else" rather than "if ...
continue;", this VIR_FREE is unnecessary since it's done at then end of
the while loop
> VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
> a.domain, a.bus);
> - continue;
> - }
> - for (; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) {
> - if (!virDomainPCIAddressSlotInUse(addrs, &a))
> - goto success;
> + } else {
> + while (a.slot <= addrs->buses[a.bus].maxSlot) {
> + if (!virDomainPCIAddressSlotInUse(addrs, &a))
> + goto success;
>
> - VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> - a.domain, a.bus, a.slot);
> + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> + a.domain, a.bus, a.slot);
> + a.slot++;
> + }
> }
> - a.slot = 1;
> + if (++a.bus < addrs->nbuses)
> + a.slot = addrs->buses[a.bus].minSlot;
> VIR_FREE(addrStr);
> }
>
> /* There were no free slots after the last used one */
So essentially we're going to search everything before to see if there's
any openings to use.
> if (addrs->dryRun) {
> - /* a is already set to the first new bus and slot 1 */
> + /* a is already set to the first new bus */
> if (virDomainPCIAddressSetGrow(addrs, &a, flags) < 0)
> goto error;
> + /* this device will use the first slot of the new bus */
> + a.slot = addrs->buses[a.bus].minSlot;
> goto success;
> } else if (flags == addrs->lastFlags) {
> /* Check the buses from 0 up to the last used one */
> for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
> - addrStr = NULL;
> + a.slot = addrs->buses[a.bus].minSlot;
> if (!(addrStr = virDomainPCIAddressAsString(&a)))
> goto error;
> if (!virDomainPCIAddressFlagsCompatible(&a, addrStr,
> @@ -527,14 +537,15 @@
> virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
> flags, false, false)) {
> VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the
> device",
> a.domain, a.bus);
> - continue;
> - }
> - for (a.slot = 1; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) {
> - if (!virDomainPCIAddressSlotInUse(addrs, &a))
> - goto success;
> -
> - VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> - a.domain, a.bus, a.slot);
> + } else {
> + while (a.slot <= addrs->buses[a.bus].maxSlot) {
> + if (!virDomainPCIAddressSlotInUse(addrs, &a))
> + goto success;
> +
> + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> + a.domain, a.bus, a.slot);
> + a.slot++;
> + }
> }
Perhaps preexisting, but one would think a VIR_FREE(addrStr) would be
needed here just as it was in the first pass... [Coverity didn't find
this either]
ACK with the adjustment
John
> }
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
