Re: [libvirt] [PATCHv2 07/17] conf: add new <target> subelement with chassisNr attribute to <controller>

On 07/17/2015 02:43 PM, Laine Stump wrote: > There are some configuration options to some types of pci > controllers that are currently automatically derived from other parts > of the controller's configuration. For example, a pci-bridge > controller has an option that is called "chassis_nr" in qemu; libvirt > always sets chassis_nr to the index of the pci-bridge. So this: > > <controller type='pci' model='pci-bridge' index='2'/> > > will always result in: > > -device pci-bridge,chassis_nr=2,... > > on the qemu commandline. In the future we may decide there is a better > way to derive that option, but even in that case we will need for > existing domains to retain the same chassis_nr they were using in the > past - that is something that is visible to the guest so it is part of > the guest ABI and changing it would lead to problems for migrating > guests (or just guests with very picky OSes). > > The <target> subelement has been added as a place to put the new > "chassisNr" attribute that will be filled in by libvirt when it > auto-generates the chassisNr; it will be saved in the config, then > reused any time the domain is started: > > <controller type='pci' model='pci-bridge' index='2'> > <model type='pci-bridge'/> > <target chassisNr='2'/> > </controller> > > The one oddity of all this is that if the controller configuration > is changed (for example to change the index or the pci address > where the controller is plugged in), the items in <target> will > *not* be re-generated, which might lead to conflict. I can't > really see any way around this, but fortunately if there is a > material conflict qemu will let us know and we will pass that on > to the user. > --- > > new in V2 (previously was a part of the patch to add pcie-root-port, > but adding the attributes under <model> instead of <target>) > > docs/formatdomain.html.in | 23 ++++++++++++++++++++ > docs/schemas/domaincommon.rng | 10 +++++++++ > src/conf/domain_conf.c | 28 +++++++++++++++++++++++-- > src/conf/domain_conf.h | 10 ++++++++- > tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 1 + > tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 1 + > 6 files changed, 70 insertions(+), 3 deletions(-) > Again assuming this is the generally acceptable mechanism. It seems fine to me though > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index fa46276..ae0d66a 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -3049,6 +3049,29 @@ > libvirt. <span class="since">Since 1.3.0 (QEMU only).</span> > </p> > <p> > + PCI controllers also have an optional s/also// > + subelement <code>&lt;target&gt;</code> with the attributes > + listed below. These are configurable items that 1) are visible > + to the guest OS so must be preserved for guest ABI > + compatibility, and 2) are usually left to default values or > + derived automatically by libvirt. In almost all cases, you > + should not manually add a <code>&lt;target&gt;</code> subelement > + to a controller, nor should you modify the values in the those > + that are automatically generated by > + libvirt. <span class="since">Since 1.3.0 (QEMU only).</span> 1.2.18 > + </p> > + <dl> > + <dt><code>chassisNr</code></dt> > + <dd> > + PCI controllers that have attribute model="pci-bridge", can > + also have a <code>chassisNr</code> attribute in > + the <code>&lt;target&gt;</code> subelement, which is used to > + control QEMU's "chassis_nr" option for the pci-bridge device > + (normally libvirt automatically sets this to the same value as > + the index attribute of the pci controller). Is there a valid range that needs to be documented? See comments for rng and parse.

> + </dd> > + </dl> > + <p> > For machine types which provide an implicit PCI bus, the pci-root > controller with index=0 is auto-added and required to use PCI devices. > pci-root has no address. > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 66518f9..1b1f592 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -1744,6 +1744,16 @@ > <empty/> > </element> > </optional> > + <optional> > + <element name="target"> > + <optional> > + <attribute name='chassisNr'> > + <ref name='uint8range'/> This is an int in domain_conf.h and is set to -1 in virDomainControllerDefNew. So one or the other seems to need adjustment since by this rule we can only be between 0 and 255 inclusive.

> + </attribute> > + </optional> > + <empty/> > + </element> > + </optional> > <!-- *-root controllers have an optional element "pcihole64"--> > <choice> > <group> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 380b758..17526d4 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1544,6 +1544,8 @@ virDomainControllerDefNew(virDomainControllerType type) > def->opts.vioserial.vectors = -1; > break; > case VIR_DOMAIN_CONTROLLER_TYPE_PCI: > + def->opts.pciopts.chassisNr = -1; > + break; > case VIR_DOMAIN_CONTROLLER_TYPE_IDE: > case VIR_DOMAIN_CONTROLLER_TYPE_FDC: > case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: > @@ -7638,6 +7640,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, > char *cmd_per_lun = NULL; > char *max_sectors = NULL; > char *guestModel = NULL; > + char *chassisNr = NULL; > xmlNodePtr saved = ctxt->node; > int rc; > > @@ -7686,6 +7689,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, > } else if (xmlStrEqual(cur->name, BAD_CAST "model")) { > if (!guestModel) > guestModel = virXMLPropString(cur, "type"); > + } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { > + if (!chassisNr) > + chassisNr = virXMLPropString(cur, "chassisNr"); Similar to note about guestModel - multiple chassisNr aren't allowed by RNG rules... > } > } > cur = cur->next; > @@ -7798,6 +7804,13 @@ virDomainControllerDefParseXML(xmlNodePtr node, > def->opts.pciopts.type = guestModel; > guestModel = 0; > } > + if (chassisNr && virStrToLong_i(chassisNr, NULL, 0, > + &def->opts.pciopts.chassisNr) < 0) { ^ one extra space too many (just had a shot of coffee so I saw it!) Also if the range is truly 0 to 255 inclusive, then don't we need to test for that here as well?

> + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid chassisNr '%s' in PCI controller"), > + chassisNr); > + goto error; > + } > break; > > default: > @@ -7824,6 +7837,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, > VIR_FREE(cmd_per_lun); > VIR_FREE(max_sectors); > VIR_FREE(guestModel); > + VIR_FREE(chassisNr); > > return def; > > @@ -18833,7 +18847,7 @@ virDomainControllerDefFormat(virBufferPtr buf, > { > const char *type = virDomainControllerTypeToString(def->type); > const char *model = NULL; > - bool pcihole64 = false, pciModel = false; > + bool pcihole64 = false, pciModel = false, pciTarget = false; > > if (!type) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -18875,13 +18889,15 @@ virDomainControllerDefFormat(virBufferPtr buf, > pcihole64 = true; > if (def->opts.pciopts.type) > pciModel = true; > + if (def->opts.pciopts.chassisNr != -1) > + pciTarget = true; [this] > break; > > default: > break; > } > > - if (pciModel || > + if (pciModel || pciTarget || > def->queues || def->cmd_per_lun || def->max_sectors || > virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) { > virBufferAddLit(buf, ">\n"); > @@ -18893,6 +18909,14 @@ virDomainControllerDefFormat(virBufferPtr buf, > virBufferAddLit(buf, "/>\n"); > } > > + if (pciTarget) { This can only be true if chassisNr != -1 > + virBufferAddLit(buf, "<target"); > + if (def->opts.pciopts.chassisNr != -1) [which means this if isn't necessary]

> + virBufferAsprintf(buf, " chassisNr='%d'", > + def->opts.pciopts.chassisNr); > + virBufferAddLit(buf, "/>\n"); and of course means the whole thing can be formatted in one line

> + } > + > if (def->queues || def->cmd_per_lun || def->max_sectors) { > virBufferAddLit(buf, "<driver"); > if (def->queues) > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 09fe3c0..32d1073 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -805,7 +805,15 @@ struct _virDomainPCIControllerOpts { > * similar to the model of <interface> devices. > */ > char *type; /* the exact name of the device in hypervisor */ > -}; > + > + /* the following items are attributes of the "target" subelement > + * of controller type='pci'. They are bits of configuration that > + * are specified on the qemu commandline and are visible to the > + * guest OS, so they must be preserved to ensure ABI > + * compatibility. > + */ > + int chassisNr; /* used by pci-bridge, -1 == unspecified */ > + }; > > /* Stores the virtual disk controller configuration */ > struct _virDomainControllerDef { Similar to previous comment about "reusing" a test - IDC, but perhaps this should have been a "new" test which we're building upon rather than stealing an existing one.