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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list