On 07/22/2015 03:30 PM, John Ferlan wrote:
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><model></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><model></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
There are so many examples of this in the code (including the parsing of the <driver> subelement just preceding this new parsing of <model>), it's easy to replicate it in new code :-P I've fixed it in mine, but maybe this should go on a list somewhere of nice beginner patches (I remember someone mentioning that idea - where was it going to go?)
+ </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.
But of course validation against the RNG isn't always done.
} } 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/
Done.
+ } + 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
I try to not add new test cases when an existing and related case can be made to serve the purpose *without eliminating testing of any other code paths*. It already takes enough time to run make check.
ACK - with the adjustments - whether you update/add a new test is your call.
I've changed it from "type" to "name", and made that into an enum, so it will need to be reviewed again anyway.