1:29 p.m.
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