[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes



On 05/14/2016 10:56 AM, Cole Robinson wrote:
> On 05/11/2016 10:58 AM, Laine Stump wrote:
>> Hand-entering indexes for 20 PCI controllers is not as tedious as
>> manually determining and entering their PCI addresses, but it's still
>> annoying, and the algorithm for determining the proper index is
>> incredibly simple (in all cases except one) - just pick the lowest
>> unused index.
>>
>> The one exception is USB2 controllers because multiple controllers in
>> the same group have the same index. For these we look to see if 1) the
>> most recently added USB controller is also a USB2 controller, and 2)
>> the group *that* controller belongs to doesn't yet have a controller
>> of the exact model we're just now adding - if both are true, the new
>> controller gets the same index, but in all other cases we just assign
>> the lowest unused index.
>>
>> With this patch in place and combined with the automatic PCI address
>> assignment, we can define a PCIe switch with several ports like this:
>>
>>   <controller type='pci' model='pcie-root-port'/>
>>   <controller type='pci' model='pcie-switch-upstream-port'/>
>>   <controller type='pci' model='pcie-switch-downstream-port'/>
>>   <controller type='pci' model='pcie-switch-downstream-port'/>
>>   <controller type='pci' model='pcie-switch-downstream-port'/>
>>   <controller type='pci' model='pcie-switch-downstream-port'/>
>>   <controller type='pci' model='pcie-switch-downstream-port'/>
>>   ...
>>
>> These will each get a unique index, and PCI addresses that connect
>> them together appropriately with no pesky numbers required.
>> ---
>>  docs/schemas/domaincommon.rng |  8 ++--
>>  src/conf/domain_conf.c        | 89 +++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 82 insertions(+), 15 deletions(-)
>>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 273715c..6abd771 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1701,9 +1701,11 @@
>>    </define>
>>    <define name="controller">
>>      <element name="controller">
>> -      <attribute name="index">
>> -        <ref name="unsignedInt"/>
>> -      </attribute>
>> +      <optional>
>> +        <attribute name="index">
>> +          <ref name="unsignedInt"/>
>> +        </attribute>
>> +      </optional>
>>        <interleave>
>>          <optional>
>>            <ref name="alias"/>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 728949b..85ffcac 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -7850,6 +7850,7 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
>>  static virDomainControllerDefPtr
>>  virDomainControllerDefParseXML(xmlNodePtr node,
>>                                 xmlXPathContextPtr ctxt,
>> +                               virDomainDef const *dom,
>>                                 unsigned int flags)
>>  {
>>      virDomainControllerDefPtr def = NULL;
>> @@ -7890,6 +7891,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>      if (!(def = virDomainControllerDefNew(type)))
>>          goto error;
>>  
>> +    model = virXMLPropString(node, "model");
>> +    if (model) {
>> +        if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("Unknown model type '%s'"), model);
>> +            goto error;
>> +        }
>> +    }
>> +
>>      idx = virXMLPropString(node, "index");
>>      if (idx) {
>>          if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 ||
>> @@ -7898,15 +7908,70 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>                             _("Cannot parse controller index %s"), idx);
>>              goto error;
>>          }
>> -    }
>> +    } else {
>> +        /* When no index is provided, find one by looking at what
>> +         * indexes are already used for this controller type in the
>> +         * domain.
>> +         */
>> +        virDomainControllerDefPtr prev = NULL;
>> +
>> +        if (IS_USB2_CONTROLLER(def)) {
>> +            /* Attempt to put new USB2 controller at the same index as
>> +             * other USB2 controllers, but only if this appears to be
>> +             * the intent. To prove intent: The last USB controller
>> +             * already on the list must also be a USB2 controller, and
>> +             * there must not yet be a controller with the exact same
>> +             * model of this one and the same index as the previously
>> +             * added controller (e.g., if this controller is a UHCI1,
>> +             * then the previous controller must be an EHCI1 or a
>> +             * UHCI[23], and there must not already be a UHCI1
>> +             * controller with the same index as the previous
>> +             * controller). If all of these are satisfied, set this
>> +             * controller to the same index as the previous
>> +             * controller.  NB: we are of course assuming that this
>> +             * new controller is about to be appended to the domain's
>> +             * controller list.
>> +             */
>> +            int prevIdx;
>> +            size_t i;
>>  
>> -    model = virXMLPropString(node, "model");
>> -    if (model) {
>> -        if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0) {
>> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> -                           _("Unknown model type '%s'"), model);
>> -            goto error;
>> +            if (dom->ncontrollers) {
>> +                prevIdx = dom->ncontrollers - 1;
>> +                while (prevIdx >= 0 &&
>> +                       dom->controllers[prevIdx]->type != VIR_DOMAIN_CONTROLLER_TYPE_USB)
>> +                    prevIdx--;
>> +                if (prevIdx >= 0)
>> +                    prev = dom->controllers[prevIdx];
>> +                }
>> +                /* if the last USB controller isn't USB2, that breaks
>> +                 * the chain, so we need a new index for this new
>> +                 * controller
>> +                 */
>> +                if (!IS_USB2_CONTROLLER(prev))
>> +                    prev = NULL;
>> +
>> +            /* if prev != NULL, we've found a potential index to
>> +             * use. Now we need to make sure that index isn't
>> +             * already used by an existing USB2 controller of the
>> +             * same model as this new one.
>> +             */
>> +            for (i = 0; prev && i < dom->ncontrollers; i++) {
>> +                if (dom->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
>> +                    dom->controllers[i]->idx == prev->idx &&
>> +                    dom->controllers[i]->model == def->model) {
>> +                    /* already have a controller of this model
>> +                     * with the proposed index, so we need to move
>> +                     * to a new index
>> +                     */
>> +                    prev = NULL;
>> +                }
>> +            }
>> +            if (prev)
>> +                def->idx = prev->idx;
>>          }
>> +        /* if none of the above applied, prev will be NULL */
>> +        if (!prev)
>> +            def->idx = virDomainControllerFindUnusedIndex(dom, def->type);
>>      }
>>  
>>      cur = node->children;
>> @@ -12958,7 +13023,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>>          break;
>>      case VIR_DOMAIN_DEVICE_CONTROLLER:
>>          if (!(dev->data.controller = virDomainControllerDefParseXML(node, ctxt,
>> -                                                                    flags)))
>> +                                                                    def, flags)))
>>              goto error;
>>          break;
>>      case VIR_DOMAIN_DEVICE_GRAPHICS:
>> @@ -16157,10 +16222,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>>          goto error;
>>  
>>      for (i = 0; i < n; i++) {
>> -        virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i],
>> -                                                                              ctxt,
>> -                                                                              flags);
>> -        if (!controller)
>> +        virDomainControllerDefPtr controller;
>> +
>> +        if (!(controller = virDomainControllerDefParseXML(nodes[i], ctxt,
>> +                                                          def, flags)))
>>              goto error;
>>  
>>          /* sanitize handling of "none" usb controller */
>>
> 
> I agree with the goal of the patch, but I think all the index assignment code
> should be moved to somewhere in the PostParse call path. The fact that the
> controller ParseXML now needs to act on the entire domain def is a giveaway
> that it's in the wrong place.
> 
> Also this seems like it should have test suite changes too

Also, formatdomain.html.in has a reference to 'index' being mandatory

- Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]