Re: [libvirt] [PATCHv2 05/17] conf: add new <model> subelement with type attribute to <controller>

On 07/17/2015 02:43 PM, Laine Stump wrote: > This new subelement is used in PCI controllers: the toplevel > *attribute* "model" of a controller denotes what kind of PCI > controller is being described, e.g. a "dmi-to-pci-bridge", > "pci-bridge", or "pci-root". But in the future there will be different > implementations of some of those types of PCI controllers, which > behave similarly from libvirt's point of view (and so should have the > same model), but use a different device in qemu (and present > themselves as a different piece of hardware in the guest). In an ideal > world we (i.e. "I") would have thought of that back when the pci > controllers were added, and used some sort of type/class/model > notation (where class was used in the way we are now using model, and > model was used for the actual manufacturer's model number of a > particular family of PCI controller), but that opportunity is long > past, so as an alternative, this patch allows selecting a particular > implementation of a pci controller with the "type" attribute of the > <model> subelement, e.g.: > > <controller type='pci' model='dmi-to-pci-bridge' index='1'> > <model type='i82801b11-bridge'/> > </controller> > > In this case, "dmi-to-pci-bridge" is the kind of controller (one that > has a single PCIe port upstream, and 32 standard PCI ports downstream, > which are not hotpluggable), and the qemu device to be used to > implement this kind of controller is named "i82801b11-bridge". > > Implementing the above now will allow us in the future to add a new > kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge > device, but instead uses something else (which doesn't yet exist, but > qemu people have been discussing it), all without breaking existing > configs. > > (note that for the existing "pci-bridge" type of PCI controller, both > the model attribute and <model> type are 'pci-bridge'. This is just a > coincidence, since it turns out that in this case the device name in > qemu really is a generic 'pci-bridge' rather than being the name of > some real-world chip) > --- > new in V2 (previously was a part of the patch to add pcie-root-port) > > docs/formatdomain.html.in | 12 ++++++++++++ > docs/schemas/domaincommon.rng | 13 +++++++++++++ > src/conf/domain_conf.c | 23 +++++++++++++++++++++-- > src/conf/domain_conf.h | 8 ++++++++ > tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++-- > tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++-- > 6 files changed, 66 insertions(+), 6 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 8cd8d09..fa46276 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -3037,6 +3037,18 @@ > (set to 0). <span class="since">Since 1.1.2 (QEMU only)</span> > </p> > <p> > + PCI controllers also have an optional > + subelement <code>&lt;model&gt;</code> with an attribute named > + "type". The type attribute holds the name of the specific device The <code>type</code> attribute... > + that qemu is emulating (e.g. "i82801b11-bridge") rather than > + simply the class of device ("dmi-to-pci-bridge", "pci-bridge"), > + which is set in the controller element's model <b>attribute</b>. > + In almost all cases, you should not manually add > + a <code>&lt;model&gt;</code> subelement to a controller, nor > + should you modify one that is automatically generated by > + libvirt. <span class="since">Since 1.3.0 (QEMU only).</span> 1.2.18 (at least for now) NB: As I read the code, only the *first* <model type='%s'> listed will be used, as virDomainControllerDefParseXML not parse a second entry nor does a second entry cause an error

> + </p> > + <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 1120003..66518f9 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -1731,6 +1731,19 @@ > <attribute name="type"> > <value>pci</value> > </attribute> > + <optional> > + <element name="model"> > + <attribute name="type"> > + <choice> > + <!-- implementations of 'pci-bridge' --> > + <value>pci-bridge</value> > + <!-- implementations of 'dmi-to-pci-bridge' --> > + <value>i82801b11-bridge</value> > + </choice> > + </attribute> > + <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 8dd4bf0..380b758 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -7637,6 +7637,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, > char *queues = NULL; > char *cmd_per_lun = NULL; > char *max_sectors = NULL; > + char *guestModel = NULL; > xmlNodePtr saved = ctxt->node; > int rc; > > @@ -7682,6 +7683,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, > queues = virXMLPropString(cur, "queues"); > cmd_per_lun = virXMLPropString(cur, "cmd_per_lun"); > max_sectors = virXMLPropString(cur, "max_sectors"); > + } else if (xmlStrEqual(cur->name, BAD_CAST "model")) { > + if (!guestModel) > + guestModel = virXMLPropString(cur, "type"); So subsequent "<model type='%s'>" are gleefully ignored? Should there be an error? IDC either way, as long as it's described/noted because you know there's someone from QA looking to add two <model...> entries and expecting the second one to be used or an error to be generated. Of course scrolling back to the RNG - syntactically there can only be one it seems.

> } > } > cur = cur->next; > @@ -7790,6 +7794,11 @@ virDomainControllerDefParseXML(xmlNodePtr node, > def->opts.pciopts.pcihole64size = VIR_DIV_UP(bytes, 1024); > } > } > + if (guestModel) { > + def->opts.pciopts.type = guestModel; > + guestModel = 0; s/0/NULL/

> + } > + break; > > default: > break; > @@ -7814,6 +7823,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, > VIR_FREE(queues); > VIR_FREE(cmd_per_lun); > VIR_FREE(max_sectors); > + VIR_FREE(guestModel); > > return def; > > @@ -18823,7 +18833,7 @@ virDomainControllerDefFormat(virBufferPtr buf, > { > const char *type = virDomainControllerTypeToString(def->type); > const char *model = NULL; > - bool pcihole64 = false; > + bool pcihole64 = false, pciModel = false; > > if (!type) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -18863,17 +18873,26 @@ virDomainControllerDefFormat(virBufferPtr buf, > case VIR_DOMAIN_CONTROLLER_TYPE_PCI: > if (def->opts.pciopts.pcihole64) > pcihole64 = true; > + if (def->opts.pciopts.type) > + pciModel = true; > break; > > default: > break; > } > > - if (def->queues || def->cmd_per_lun || def->max_sectors || > + if (pciModel || > + def->queues || def->cmd_per_lun || def->max_sectors || > virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) { > virBufferAddLit(buf, ">\n"); > virBufferAdjustIndent(buf, 2); > > + if (pciModel) { > + virBufferAddLit(buf, "<model"); > + virBufferEscapeString(buf, " type='%s'", def->opts.pciopts.type); > + virBufferAddLit(buf, "/>\n"); > + } > + > 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 50750c1..09fe3c0 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; > struct _virDomainPCIControllerOpts { > bool pcihole64; > unsigned long pcihole64size; > + > + /* the type is in the "model" subelement, e.g.: > + * <controller type='pci' model='pcie-root-port'> > + * <model type='ioh3420''/> > + * ... > + * similar to the model of <interface> devices. > + */ > + char *type; /* the exact name of the device in hypervisor */ > }; > > /* Stores the virtual disk controller configuration */ Since examples still exist that do not have the <model type='%s'> I suppose it's OK to hijack an existing test, but having a "new" test probably would have been better

ACK - with the adjustments - whether you update/add a new test is your call.