On 07/22/2015 02:50 PM, John Ferlan wrote:
>
> 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
Correct. Thanks!
>
>> 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]
Yes! Another good catch! I'm surprised that this didn't lead to any
valgrind or coverity reports before now - that loop has been missing a
VIR_FREE(addrStr) for a very long time.
>
> ACK with the adjustment
>
> John
>> }
>> }
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list