Since the first 3 patches of V2 were ACKed and uncontroversial, I fixed the small problems pointed out in the reviews and pushed them. Thus, Patches 01-13 here correspond to Patches 04-17 in V2. Most of these patches were already ACKed in V2 (pending my making small fixes pointed out in review), but the main things that need review are: 1) changing of model name from a char* to an enum in Patch 01, and corresponding blowback in patches 02, 07, 10, and 13 2) range checking of chassisNr in Patch 03 and chassis+port in patch 06. 3) check for duplicate <model> in patch 01, and duplicate <target> in patch 03. I did add one new negative test, and reworded some documentation, but I'm about to go mostly offline for 10 days, and would rather not have these patches bitrotting during that time if they are okay other than that. (also, I see both of those tasks as having no practical end, but do give my word to add more to both in later followups). If by chance everything is ACKed before DV freezes for RC1, but after I'm already offline (which will happen Sunday morning U.S. east coast time), I would appreciate if the reviewer could push the patches so they'll get the RC testing and be in the 1.2.18 release. (If not, I'll take care of it when I return). P.S. I ran "git rebase -i master -x "make -j8 check && make -j8 syntax-check" before sending. Laine Stump (13): conf: add new <model> subelement with name attribute to <controller> qemu: implement <model> subelement to <controller> conf: add new <target> subelement with chassisNr attribute to <controller> qemu: implement <target chassisNr='n'/> subelement/attribute of <controller> qemu: add capabilities bit for device ioh3420 conf: new pci controller model "pcie-root-port" qemu: support new pci controller model "pcie-root-port" qemu: add capabilities bit for device x3130-upstream conf: new pci controller model "pcie-switch-upstream-port" qemu: support new pci controller model "pcie-switch-upstream-port" qemu: add capabilities bit for device xio3130-downstream conf: new pcie-controller model "pcie-switch-downstream-port" qemu: support new pci controller model "pcie-switch-downstream-port" docs/formatdomain.html.in | 90 +++++++- docs/schemas/domaincommon.rng | 42 ++++ src/conf/domain_addr.c | 32 ++- src/conf/domain_addr.h | 12 +- src/conf/domain_conf.c | 162 +++++++++++++- src/conf/domain_conf.h | 34 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 8 +- src/qemu/qemu_capabilities.h | 5 +- src/qemu/qemu_command.c | 238 ++++++++++++++++++++- tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 3 + tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 3 + tests/qemuhelptest.c | 10 +- .../qemuxml2argv-pcie-root-port-too-many.xml | 60 ++++++ .../qemuxml2argv-pcie-root-port.args | 10 + .../qemuxml2argv-pcie-root-port.xml | 36 ++++ .../qemuxml2argv-pcie-switch-downstream-port.args | 18 ++ .../qemuxml2argv-pcie-switch-downstream-port.xml | 44 ++++ .../qemuxml2argv-pcie-switch-upstream-port.args | 12 ++ .../qemuxml2argv-pcie-switch-upstream-port.xml | 37 ++++ tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 9 +- tests/qemuxml2argvtest.c | 33 +++ tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 9 +- tests/qemuxml2xmltest.c | 4 + 30 files changed, 909 insertions(+), 21 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port-too-many.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml -- 2.1.0
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 "name" attribute of the <model> subelement, e.g.: <controller type='pci' model='dmi-to-pci-bridge' index='1'> <model name='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> name 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) --- change from V2: * attribute is now called "name" instead of "type" * different possible model names are stored internally as an enum value rather than a string. * more than one occurence of <model> in a single controller is now considered an error * docs say "1.2.18" instead of "1.3.0" docs/formatdomain.html.in | 13 +++++++ docs/schemas/domaincommon.rng | 13 +++++++ src/conf/domain_conf.c | 46 +++++++++++++++++++++++-- src/conf/domain_conf.h | 16 +++++++++ src/libvirt_private.syms | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- 7 files changed, 100 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..9abceee 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3042,6 +3042,19 @@ (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 + <code>name</code>. The name attribute holds the name of the + specific device 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.2.18 (QEMU + only).</span> + </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..a4c1c9b 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="name"> + <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 6b557d1..d58e679 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -326,6 +326,12 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-bridge", "dmi-to-pci-bridge") +VIR_ENUM_IMPL(virDomainControllerPCIModelName, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST, + "none", + "pci-bridge", + "i82801b11-bridge") + VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", "buslogic", @@ -7766,6 +7772,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *queues = NULL; char *cmd_per_lun = NULL; char *max_sectors = NULL; + bool processedModel = false; + char *modelName = NULL; xmlNodePtr saved = ctxt->node; int rc; @@ -7809,6 +7817,15 @@ 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 (processedModel) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple <model> elements in " + "controller definition not allowed")); + goto error; + } + modelName = virXMLPropString(cur, "name"); + processedModel = true; } } cur = cur->next; @@ -7917,6 +7934,15 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->opts.pciopts.pcihole64size = VIR_DIV_UP(bytes, 1024); } } + if (modelName && + (def->opts.pciopts.modelName + = virDomainControllerPCIModelNameTypeFromString(modelName)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown PCI controller model name '%s'"), + modelName); + goto error; + } + break; default: break; @@ -7941,6 +7967,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(queues); VIR_FREE(cmd_per_lun); VIR_FREE(max_sectors); + VIR_FREE(modelName); return def; @@ -18950,7 +18977,8 @@ virDomainControllerDefFormat(virBufferPtr buf, { const char *type = virDomainControllerTypeToString(def->type); const char *model = NULL; - bool pcihole64 = false; + const char *modelName = NULL; + bool pcihole64 = false, pciModel = false; if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -18990,17 +19018,31 @@ virDomainControllerDefFormat(virBufferPtr buf, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: if (def->opts.pciopts.pcihole64) pcihole64 = true; + if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + 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) { + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected model name value %d"), + def->opts.pciopts.modelName); + return -1; + } + virBufferAsprintf(buf, "<model name='%s'/>\n", modelName); + } + 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 0fe6b1a..fd65792 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -757,6 +757,14 @@ typedef enum { } virDomainControllerModelPCI; typedef enum { + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE, + + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST +} virDomainControllerPCIModelName; + +typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC, @@ -797,6 +805,13 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; struct _virDomainPCIControllerOpts { bool pcihole64; unsigned long pcihole64size; + + /* the exact controller name is in the "model" subelement, e.g.: + * <controller type='pci' model='pcie-root-port'> + * <model name='ioh3420''/> + * ... + */ + int modelName; /* the exact name of the device in hypervisor */ }; /* Stores the virtual disk controller configuration */ @@ -2978,6 +2993,7 @@ VIR_ENUM_DECL(virDomainDiskDiscard) VIR_ENUM_DECL(virDomainDiskMirrorState) VIR_ENUM_DECL(virDomainController) VIR_ENUM_DECL(virDomainControllerModelPCI) +VIR_ENUM_DECL(virDomainControllerPCIModelName) VIR_ENUM_DECL(virDomainControllerModelSCSI) VIR_ENUM_DECL(virDomainControllerModelUSB) VIR_ENUM_DECL(virDomainFS) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ff322d6..37323ad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -192,6 +192,8 @@ virDomainControllerModelSCSITypeFromString; virDomainControllerModelSCSITypeToString; virDomainControllerModelUSBTypeFromString; virDomainControllerModelUSBTypeToString; +virDomainControllerPCIModelNameTypeFromString; +virDomainControllerPCIModelNameTypeToString; virDomainControllerRemove; virDomainControllerTypeToString; virDomainCpuPlacementModeTypeFromString; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 05967a4..132c15f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -20,8 +20,12 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <controller type='pci' index='0' model='pcie-root'/> - <controller type='pci' index='1' model='dmi-to-pci-bridge'/> - <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + </controller> <video> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> </video> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 9dd4162..4d8fc5f 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -20,8 +20,12 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <controller type='pci' index='0' model='pcie-root'/> - <controller type='pci' index='1' model='dmi-to-pci-bridge'/> - <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + </controller> <controller type='sata' index='0'/> <video> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> -- 2.1.0
On Sat, Jul 25, 2015 at 03:58:25PM -0400, 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 "name" attribute of the <model> subelement, e.g.:
<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model name='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> name 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) --- change from V2:
* attribute is now called "name" instead of "type" * different possible model names are stored internally as an enum value rather than a string. * more than one occurence of <model> in a single controller is now considered an error * docs say "1.2.18" instead of "1.3.0"
docs/formatdomain.html.in | 13 +++++++ docs/schemas/domaincommon.rng | 13 +++++++ src/conf/domain_conf.c | 46 +++++++++++++++++++++++-- src/conf/domain_conf.h | 16 +++++++++ src/libvirt_private.syms | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- 7 files changed, 100 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6b557d1..d58e679 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7809,6 +7817,15 @@ 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 (processedModel) {
Unnecessary variable, you could just use 'modelName', but I have no opinion on that, so ACK either way.
On 08/03/2015 05:55 AM, Martin Kletzander wrote:
On Sat, Jul 25, 2015 at 03:58:25PM -0400, 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 "name" attribute of the <model> subelement, e.g.:
<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model name='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> name 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) --- change from V2:
* attribute is now called "name" instead of "type" * different possible model names are stored internally as an enum value rather than a string. * more than one occurence of <model> in a single controller is now considered an error * docs say "1.2.18" instead of "1.3.0"
docs/formatdomain.html.in | 13 +++++++ docs/schemas/domaincommon.rng | 13 +++++++ src/conf/domain_conf.c | 46 +++++++++++++++++++++++-- src/conf/domain_conf.h | 16 +++++++++ src/libvirt_private.syms | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- 7 files changed, 100 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6b557d1..d58e679 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7809,6 +7817,15 @@ 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 (processedModel) {
Unnecessary variable, you could just use 'modelName',
If it was by itself, yes. (As a matter of fact that's how I original added the "don't allow multiples" to the patch). But as you found out in patch 3, similar checking is needed for <target>, and target has multiple attributes; rather than checking for "chassis || chassisNr || port" there, I gave that one a single bool that gets set, so I came back and made this one the same to be consistent - the more similar code looks, the easier it is to follow the logic (and to potentially make a utility function or combine code in some other way in the future). Besides, I like to pretend that I'm consistent :-)
but I have no opinion on that, so ACK either way.
On Sat, Jul 25, 2015 at 03:58:25PM -0400, 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 "name" attribute of the <model> subelement, e.g.:
<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model name='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> name 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) --- change from V2:
* attribute is now called "name" instead of "type" * different possible model names are stored internally as an enum value rather than a string. * more than one occurence of <model> in a single controller is now considered an error * docs say "1.2.18" instead of "1.3.0"
docs/formatdomain.html.in | 13 +++++++ docs/schemas/domaincommon.rng | 13 +++++++ src/conf/domain_conf.c | 46 +++++++++++++++++++++++-- src/conf/domain_conf.h | 16 +++++++++ src/libvirt_private.syms | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- 7 files changed, 100 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..9abceee 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3042,6 +3042,19 @@ (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 + <code>name</code>. The name attribute holds the name of the + specific device 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.2.18 (QEMU
Oh, I forgot to ask you to change this to 1.2.19 ^^, sorry about missing not just the rc1, but the whole release. But I was partly offline for the whole week as well and I hoped someone else might pick this up... Martin
This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline): 1) pci-bridge - sets <model> name attribute default as "pci-bridge" 2) dmi-to-pci-bridge - sets <model> name attribute default as "i82801b11-bridge". These both match current hardcoded practice. The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added, and that hasn't been done at the time the PostParse callbacks are being run. qemuDomainAssignPCIAddresses() is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent. qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. --- Changes from V2: use enum instead of string model name. src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f30c4..6a19d15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options; if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue; addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + /* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = def->model; + const char *modelName = NULL; if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) @@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", - def->idx, def->info.alias); + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pci-bridge"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", + modelName, def->idx, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown dmi-to-pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a dmi-to-pci-bridge"), + modelName); + goto error; + } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The dmi-to-pci-bridge (i82801b11-bridge) " + _("the dmi-to-pci-bridge (i82801b11-bridge) " "controller is not supported in this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias); + virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; } break; -- 2.1.0
[I reduced the Cc list so I don't need to hear jtomko's whining again] On Sat, Jul 25, 2015 at 03:58:26PM -0400, Laine Stump wrote:
This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline):
1) pci-bridge - sets <model> name attribute default as "pci-bridge"
2) dmi-to-pci-bridge - sets <model> name attribute default as "i82801b11-bridge".
These both match current hardcoded practice.
The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added, and that hasn't been done at the time the PostParse callbacks are being run. qemuDomainAssignPCIAddresses() is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent.
qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. ---
Changes from V2: use enum instead of string model name.
src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f30c4..6a19d15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options;
if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue;
addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + /* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = def->model; + const char *modelName = NULL;
if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) @@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", - def->idx, def->info.alias); + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pci-bridge"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", + modelName, def->idx, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown dmi-to-pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a dmi-to-pci-bridge"), + modelName); + goto error; + }
I see why you didn't care whether it will be implemented this way or the other. That's because you're checking for stuff we usually leave the parsing to handle. I haven't checked that there is a possibility of having domain parsed without assigning PCI addresses, but the def->opts.pciopts.modelName cannot be invalid in this case. Anyway, being _too_ careful cannot hurt, no need to change it unless you want to.
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The dmi-to-pci-bridge (i82801b11-bridge) " + _("the dmi-to-pci-bridge (i82801b11-bridge) " "controller is not supported in this QEMU binary")); goto error; }
You could also save a bunch of lines using what I suggested, that is concentrating device->caps mapping in a separate function and using one error message for all models, not one for every model, e.g.: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the '%s' ('%s') controller is not supported " "in this QEMU binary"), blahTypeToString(def->model), blehTypeToString(def->opts.pciopts.modelName)); but that can be done later on, the code works properly as it is now. ACK
- virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias); + virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; } break; -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/03/2015 06:05 AM, Martin Kletzander wrote:
[I reduced the Cc list so I don't need to hear jtomko's whining again]
On Sat, Jul 25, 2015 at 03:58:26PM -0400, Laine Stump wrote:
This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline):
1) pci-bridge - sets <model> name attribute default as "pci-bridge"
2) dmi-to-pci-bridge - sets <model> name attribute default as "i82801b11-bridge".
These both match current hardcoded practice.
The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added, and that hasn't been done at the time the PostParse callbacks are being run. qemuDomainAssignPCIAddresses() is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent.
qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. ---
Changes from V2: use enum instead of string model name.
src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f30c4..6a19d15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options;
if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue;
addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + /* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = def->model; + const char *modelName = NULL;
if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) @@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", - def->idx, def->info.alias); + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pci-bridge"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", + modelName, def->idx, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown dmi-to-pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a dmi-to-pci-bridge"), + modelName); + goto error; + }
I see why you didn't care whether it will be implemented this way or the other. That's because you're checking for stuff we usually leave the parsing to handle.
Checking whether the particular modelName given in the XML (or auto-assigned later) is supported by this QEMU isn't something that's done by the parser or post-parse callbacks. It's done while we're building the commandline because it's then that we have accurate and up-to-date capability info. (Actually I just looked and up until just a few weeks ago, qemuCaps wasn't used anywhere in the post-parse callbacks; just recently code was added to the domain post-parse callback to retrieve the qemuCaps and use it when deciding whether to add an implicit pcie-root controller to an ARM virtual machine. This is done because apparently you need to check the capabilities to see if the machine has a pcie-root, but that seems troublesome to me, because a machine that has a pcie-root and one that don't should *not* have the same machinetype name in my opinion (that is a *very* serious difference in ABI). Also, I wonder if this ARM "virt-*" machinetype really supports the i82801b11-bridge controller (since it is modelled on a controller that in the real world is only available integrated into an Intel x86 chipset). The things you find while cscoping around at midnight on Saturday...)
I haven't checked that there is a possibility of having domain parsed without assigning PCI addresses,
not in the code as it exists now - PCI addresses are assigned by a separate function that gets called after return from the parser (and post parse callbacks), but every place that calls the parser also calls the function that assigns PCI addresses.
but the def->opts.pciopts.modelName cannot be invalid in this case. Anyway, being _too_ careful cannot hurt, no need to change it unless you want to.
See your previous sentence. Checking for an empty modelName is a sanity check. I like to be careful, especially when there is separation in space and time between when the value should have been set and when it is used. Today it's true that it's always set, but if someone unknowingly changes the code over in "the other place" some day, I'd rather get a clear error about it than a segv.
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The dmi-to-pci-bridge (i82801b11-bridge) " + _("the dmi-to-pci-bridge (i82801b11-bridge) " "controller is not supported in this QEMU binary")); goto error; }
You could also save a bunch of lines using what I suggested, that is concentrating device->caps mapping in a separate function and using one error message for all models, not one for every model, e.g.:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the '%s' ('%s') controller is not supported " "in this QEMU binary"), blahTypeToString(def->model), blehTypeToString(def->opts.pciopts.modelName));
but that can be done later on, the code works properly as it is now.
I missed that suggestion, but would welcome the consolidation. However we still have the situation that each modelName only works with one particular model, so it would take some extra thought to get the right amount of the duplicated code into the common function and in the right order (so I'll just leave it as is for now.
ACK
On Sun, Aug 09, 2015 at 01:11:27AM -0400, Laine Stump wrote:
On 08/03/2015 06:05 AM, Martin Kletzander wrote:
[I reduced the Cc list so I don't need to hear jtomko's whining again]
On Sat, Jul 25, 2015 at 03:58:26PM -0400, Laine Stump wrote:
This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline):
1) pci-bridge - sets <model> name attribute default as "pci-bridge"
2) dmi-to-pci-bridge - sets <model> name attribute default as "i82801b11-bridge".
These both match current hardcoded practice.
The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added, and that hasn't been done at the time the PostParse callbacks are being run. qemuDomainAssignPCIAddresses() is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent.
qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. ---
Changes from V2: use enum instead of string model name.
src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f30c4..6a19d15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options;
if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue;
addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + /* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = def->model; + const char *modelName = NULL;
if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) @@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", - def->idx, def->info.alias); + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pci-bridge"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", + modelName, def->idx, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown dmi-to-pci-bridge model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a dmi-to-pci-bridge"), + modelName); + goto error; + }
I see why you didn't care whether it will be implemented this way or the other. That's because you're checking for stuff we usually leave the parsing to handle.
Checking whether the particular modelName given in the XML (or auto-assigned later) is supported by this QEMU isn't something that's done by the parser or post-parse callbacks. It's done while we're building the commandline because it's then that we have accurate and up-to-date capability info.
I meant all the checks except the one you mentioned :) Anyway, as I said, there is no problem keeping it this way. I just wanted to say that some space could be saved with methods similar to the ones used with the following functions. But that can be cleaned up at any point. qemuControllerModelUSBToCaps() qemuSoundCodecTypeToCaps()
(Actually I just looked and up until just a few weeks ago, qemuCaps wasn't used anywhere in the post-parse callbacks; just recently code was added to the domain post-parse callback to retrieve the qemuCaps and use it when deciding whether to add an implicit pcie-root controller to an ARM virtual machine. This is done because apparently you need to check the capabilities to see if the machine has a pcie-root, but that seems troublesome to me, because a machine that has a pcie-root and one that don't should *not* have the same machinetype name in my opinion (that is a *very* serious difference in ABI). Also, I wonder if this ARM "virt-*" machinetype really supports the i82801b11-bridge controller (since it is modelled on a controller that in the real world is only available integrated into an Intel x86 chipset). The things you find while cscoping around at midnight on Saturday...)
Well, yes. IMHO this was pushed harshly and I said that I don't like it. It won't work properly *and* it needs hack to work properly with our test suite. Unfortunately nobody else looks like replying with their opinion on what should be the next step (I'd say fixing, of course).
I haven't checked that there is a possibility of having domain parsed without assigning PCI addresses,
not in the code as it exists now - PCI addresses are assigned by a separate function that gets called after return from the parser (and post parse callbacks), but every place that calls the parser also calls the function that assigns PCI addresses.
but the def->opts.pciopts.modelName cannot be invalid in this case. Anyway, being _too_ careful cannot hurt, no need to change it unless you want to.
See your previous sentence. Checking for an empty modelName is a sanity check. I like to be careful, especially when there is separation in space and time between when the value should have been set and when it is used. Today it's true that it's always set, but if someone unknowingly changes the code over in "the other place" some day, I'd rather get a clear error about it than a segv.
I understood that and it's sensible. It's just not what we do everywhere else :) In the future I'd like to move at least part of the parsing/formatting code into data, which would deal partly with this, but that's a long shot for now.
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The dmi-to-pci-bridge (i82801b11-bridge) " + _("the dmi-to-pci-bridge (i82801b11-bridge) " "controller is not supported in this QEMU binary")); goto error; }
You could also save a bunch of lines using what I suggested, that is concentrating device->caps mapping in a separate function and using one error message for all models, not one for every model, e.g.:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("the '%s' ('%s') controller is not supported " "in this QEMU binary"), blahTypeToString(def->model), blehTypeToString(def->opts.pciopts.modelName));
but that can be done later on, the code works properly as it is now.
I missed that suggestion, but would welcome the consolidation. However we still have the situation that each modelName only works with one particular model, so it would take some extra thought to get the right amount of the duplicated code into the common function and in the right order (so I'll just leave it as is for now.
That could be dealt with some function that would map 'models' and 'model names' and check it, then this error could be used. I'll keep this in mind and will eventually cook a patch. Unless I forget since this is really just a cosmetic thing. Thanks for the reply, I like that we're on the same note. I managed to rebase my patch onto the pushed series and can send it now, thanks.
ACK
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, in qemu a pci-bridge controller has an option that is called "chassis_nr"; up until now libvirt has always set 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. --- changes from V2: * check chassisNr for 0-255 range * multiple <target>s in a single controller is now an error * 1.3.0 -> 1.2.18 docs/formatdomain.html.in | 24 +++++++++++++ docs/schemas/domaincommon.rng | 10 ++++++ src/conf/domain_conf.c | 45 +++++++++++++++++++++++-- src/conf/domain_conf.h | 10 +++++- tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 1 + tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 1 + 6 files changed, 88 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9abceee..fdf7e82 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3055,6 +3055,30 @@ only).</span> </p> <p> + PCI controllers also have an optional + subelement <code><target></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><target></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.2.18 (QEMU only).</span> + </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><target></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). If set, chassisNr + must be between 0 and 255. + </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 a4c1c9b..a61e209 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'/> + </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 d58e679..fbad7e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1551,6 +1551,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: @@ -7774,6 +7776,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *max_sectors = NULL; bool processedModel = false; char *modelName = NULL; + bool processedTarget = false; + char *chassisNr = NULL; xmlNodePtr saved = ctxt->node; int rc; @@ -7826,6 +7830,15 @@ virDomainControllerDefParseXML(xmlNodePtr node, } modelName = virXMLPropString(cur, "name"); processedModel = true; + } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { + if (processedTarget) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple <target> elements in " + "controller definition not allowed")); + goto error; + } + chassisNr = virXMLPropString(cur, "chassisNr"); + processedTarget = true; } } cur = cur->next; @@ -7942,6 +7955,23 @@ virDomainControllerDefParseXML(xmlNodePtr node, modelName); goto error; } + if (chassisNr) { + if (virStrToLong_i(chassisNr, NULL, 0, + &def->opts.pciopts.chassisNr) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassisNr '%s' in PCI controller"), + chassisNr); + goto error; + } + if (def->opts.pciopts.chassisNr < 0 || + def->opts.pciopts.chassisNr > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassisNr '%s' out of range " + "- must be 0-255"), + chassisNr); + goto error; + } + } break; default: @@ -7968,6 +7998,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(cmd_per_lun); VIR_FREE(max_sectors); VIR_FREE(modelName); + VIR_FREE(chassisNr); return def; @@ -18978,7 +19009,7 @@ virDomainControllerDefFormat(virBufferPtr buf, const char *type = virDomainControllerTypeToString(def->type); const char *model = NULL; const char *modelName = NULL; - bool pcihole64 = false, pciModel = false; + bool pcihole64 = false, pciModel = false, pciTarget = false; if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -19020,13 +19051,15 @@ virDomainControllerDefFormat(virBufferPtr buf, pcihole64 = true; if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) pciModel = true; + if (def->opts.pciopts.chassisNr != -1) + pciTarget = true; 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"); @@ -19043,6 +19076,14 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<model name='%s'/>\n", modelName); } + if (pciTarget) { + virBufferAddLit(buf, "<target"); + if (def->opts.pciopts.chassisNr != -1) + virBufferAsprintf(buf, " chassisNr='%d'", + def->opts.pciopts.chassisNr); + 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 fd65792..761ce8b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -812,7 +812,15 @@ struct _virDomainPCIControllerOpts { * ... */ int modelName; /* 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 { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 132c15f..0c3da85 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -25,6 +25,7 @@ </controller> <controller type='pci' index='2' model='pci-bridge'> <model name='pci-bridge'/> + <target chassisNr='56'/> </controller> <video> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 4d8fc5f..abb3a0f 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -25,6 +25,7 @@ </controller> <controller type='pci' index='2' model='pci-bridge'> <model name='pci-bridge'/> + <target chassisNr='56'/> </controller> <controller type='sata' index='0'/> <video> -- 2.1.0
On Sat, Jul 25, 2015 at 03:58:27PM -0400, 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, in qemu a pci-bridge controller has an option that is called "chassis_nr"; up until now libvirt has always set 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. ---
changes from V2:
* check chassisNr for 0-255 range * multiple <target>s in a single controller is now an error * 1.3.0 -> 1.2.18
docs/formatdomain.html.in | 24 +++++++++++++ docs/schemas/domaincommon.rng | 10 ++++++ src/conf/domain_conf.c | 45 +++++++++++++++++++++++-- src/conf/domain_conf.h | 10 +++++- tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 1 + tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 1 + 6 files changed, 88 insertions(+), 3 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9abceee..fdf7e82 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3055,6 +3055,30 @@ only).</span> </p> <p> + PCI controllers also have an optional + subelement <code><target></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><target></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.2.18 (QEMU only).</span>
s/18/19/
+ </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><target></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). If set, chassisNr + must be between 0 and 255. + </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/src/conf/domain_conf.c b/src/conf/domain_conf.c index d58e679..fbad7e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7826,6 +7830,15 @@ virDomainControllerDefParseXML(xmlNodePtr node, } modelName = virXMLPropString(cur, "name"); processedModel = true; + } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { + if (processedTarget) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple <target> elements in " + "controller definition not allowed")); + goto error; + } + chassisNr = virXMLPropString(cur, "chassisNr"); + processedTarget = true;
Similarly to the previous patch you could omit the boolean here. Also there will be way more similarities in following patches that could be coupled in a function. But that's more like suggestion for future since it wouldn't go with the rest of the file, the whole parsing could use a re-factor :) ACK
This uses the new subelement/attribute in two ways: 1) If a "pci-bridge" pci controller has no chassisNr attribute, it will automatically be set to the controller's index as soon as the controller's PCI address is known (during qemuDomainAssignPCIAddresses()). 2) when creating the commandline for a pci-bridge device, chassisNr will be used to set qemu's chassis_nr option (rather than the previous practice of hard-coding it to the controller's index). --- changes from V2: readjust for rebase on earlier patches' changes src/qemu/qemu_command.c | 8 ++++++-- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a19d15..078c365 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2267,6 +2267,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; + if (options->chassisNr == -1) + options->chassisNr = cont->idx; break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) @@ -4650,7 +4652,8 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: if (def->opts.pciopts.modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || + def->opts.pciopts.chassisNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pci-bridge options not set")); goto error; @@ -4678,7 +4681,8 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, goto error; } virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", - modelName, def->idx, def->info.alias); + modelName, def->opts.pciopts.chassisNr, + def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: if (def->opts.pciopts.modelName diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 888aa6b..e42022d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -2,7 +2,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device pci-bridge,chassis_nr=56,id=pci.2,bus=pci.1,addr=0x1 \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ -vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 -- 2.1.0
This is a PCIE "root port". It connects only to a port of the integrated pcie.0 bus of a Q35 machine (can't be hotplugged), and provides a single PCIe port that can have PCI or PCIe devices hotplugged into it. This device will be used to implement the "pcie-root-port" model of pci controller. --- change from V2: rebase src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_capabilities.h | 3 ++- tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemuhelptest.c | 6 ++++-- 10 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d8cb32d..a9dc395 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1,7 +1,7 @@ /* * qemu_capabilities.c: QEMU capabilities generation * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -288,6 +288,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "vhost-user-multiqueue", /* 190 */ "migration-event", + "ioh3420", ); @@ -1568,6 +1569,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "ivshmem", QEMU_CAPS_DEVICE_IVSHMEM }, { "pc-dimm", QEMU_CAPS_DEVICE_PC_DIMM }, { "pci-serial", QEMU_CAPS_DEVICE_PCI_SERIAL }, + { "ioh3420", QEMU_CAPS_DEVICE_IOH3420 }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f77bd06..f179e0b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -1,7 +1,7 @@ /* * qemu_capabilities.h: QEMU capabilities generation * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -231,6 +231,7 @@ typedef enum { QEMU_CAPS_CPU_AARCH64_OFF = 189, /* -cpu ...,aarch64=off */ QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */ QEMU_CAPS_MIGRATION_EVENT = 191, /* MIGRATION event */ + QEMU_CAPS_DEVICE_IOH3420 = 192, /* -device ioh3420 */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps index 30239df..a1fafa6 100644 --- a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps @@ -120,4 +120,5 @@ <flag name='vmware-svga.vgamem_mb'/> <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps index ea3d850..824ef02 100644 --- a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps @@ -135,4 +135,5 @@ <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps index 2c19ddc..7ef5199 100644 --- a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps @@ -136,4 +136,5 @@ <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps index aadccd5..8c3d48e 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps @@ -145,4 +145,5 @@ <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps index 3e81cbf..72f7625 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps @@ -151,4 +151,5 @@ <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps index 84c357f..d81c80c 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps @@ -151,4 +151,5 @@ <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index b1ee8df..1a39dce 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -167,4 +167,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='pc-dimm'/> <flag name='pci-serial'/> + <flag name='ioh3420'/> </qemuCaps> diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 507831c..6211640 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -753,7 +753,8 @@ mymain(void) QEMU_CAPS_DEVICE_USB_KBD, QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_SPLASH_TIMEOUT, - QEMU_CAPS_DEVICE_IVSHMEM); + QEMU_CAPS_DEVICE_IVSHMEM, + QEMU_CAPS_DEVICE_IOH3420); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -853,7 +854,8 @@ mymain(void) QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_OBJECT_USB_AUDIO, QEMU_CAPS_SPLASH_TIMEOUT, - QEMU_CAPS_DEVICE_IVSHMEM); + QEMU_CAPS_DEVICE_IVSHMEM, + QEMU_CAPS_DEVICE_IOH3420); DO_TEST_FULL("qemu-1.2.0", 1002000, 0, 0, VIR_ERR_CONFIG_UNSUPPORTED, QEMU_CAPS_LAST); DO_TEST_FULL("qemu-kvm-1.2.0", 1002000, 1, 0, VIR_ERR_CONFIG_UNSUPPORTED, -- 2.1.0
This controller can be connected (at domain startup time only - not hotpluggable) only to a port on the pcie root complex ("pcie-root" in libvirt config), hence the new connect type VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that will accept any PCI or PCIe device. New attributes must be added to the controller <target> subelement for this - chassis and port are guest-visible option values that will be set by libvirt with values derived from the controller's index and pci address information. --- change from V2: * check chassis/port for 0-255 range * 1.3.0 -> 1.2.18 docs/formatdomain.html.in | 33 +++++++++- docs/schemas/domaincommon.rng | 13 ++++ src/conf/domain_addr.c | 10 ++- src/conf/domain_addr.h | 5 +- src/conf/domain_conf.c | 75 +++++++++++++++++++++- src/conf/domain_conf.h | 8 ++- src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-root-port.xml | 36 +++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fdf7e82..a9db924 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3029,10 +3029,11 @@ <p> PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, - <code>pci-bridge</code>, or <code>dmi-to-pci-bridge</code>. + <code>pcie-root-port</code>, <code>pci-bridge</code>, + or <code>dmi-to-pci-bridge</code>. (pci-root and pci-bridge <span class="since">since 1.0.5</span>, pcie-root and dmi-to-pci-bridge <span class="since">since - 1.1.2</span>) + 1.1.2</span>, pcie-root-port <span class="since">since 1.2.18</span>) The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in kilobytes, or in the unit specified by <code>pcihole64</code>'s @@ -3077,6 +3078,23 @@ the index attribute of the pci controller). If set, chassisNr must be between 0 and 255. </dd> + <dt><code>chassis</code></dt> + <dd> + pcie-root-port controllers can also have + a <code>chassis</code> attribute in + the <code><target></code> subelement, which is used to + set the controller's "chassis" configuration value, which is + visible to the virtual machine. If set, chassis must be + between 0 and 255. + </dd> + <dt><code>port</code></dt> + <dd> + pcie-root-port controllers can also have a <code>port</code> + attribute in the <code><target></code> subelement, which + is used to set the controller's "port" configuration value, + which is visible to the virtual machine. If set, port must be + between 0 and 255. + </dd> </dl> <p> For machine types which provide an implicit PCI bus, the pci-root @@ -3123,6 +3141,17 @@ auto-determined by libvirt will be placed on this pci-bridge device. (<span class="since">since 1.1.2</span>). </p> + <p> + Domains with an implicit pcie-root can also add controllers + with <code>model='pcie-root-port'</code>. This is a simple type of + bridge device that can connect only to one of the 31 slots on + the pcie-root bus on the upstream side, and makes a single + (PCIe, hotpluggable) port (at slot='0') available on the + downstream side. This controller can be used to provide a single + slot to later hotplug a PCIe device (but is not itself + hotpluggable - it must be in the configuration when the domain + is started). (<span class="since">since 1.2.18</span>) + </p> <pre> ... <devices> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a61e209..7d7f412 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1739,6 +1739,8 @@ <value>pci-bridge</value> <!-- implementations of 'dmi-to-pci-bridge' --> <value>i82801b11-bridge</value> + <!-- implementations of 'pcie-root-port' --> + <value>ioh3420</value> </choice> </attribute> <empty/> @@ -1751,6 +1753,16 @@ <ref name='uint8range'/> </attribute> </optional> + <optional> + <attribute name="chassis"> + <ref name='uint8range'/> + </attribute> + </optional> + <optional> + <attribute name="port"> + <ref name='uint8range'/> + </attribute> + </optional> <empty/> </element> </optional> @@ -1774,6 +1786,7 @@ <choice> <value>pci-bridge</value> <value>dmi-to-pci-bridge</value> + <value>pcie-root-port</value> </choice> </attribute> </group> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 9519f79..c2a207b 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -183,9 +183,9 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: /* slots 1 - 31, no hotplug, PCIe only unless the address was * specified in user config *and* the particular device being - * attached also allows it + * attached also allows it. */ - bus->flags = VIR_PCI_CONNECT_TYPE_PCIE; + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE | VIR_PCI_CONNECT_TYPE_PCIE_ROOT; bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; @@ -196,6 +196,12 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + /* provides one slot which is pcie and hotpluggable */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE | VIR_PCI_CONNECT_HOTPLUGGABLE; + bus->minSlot = 0; + bus->maxSlot = 0; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index dcfd2e5..2a0ff96 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -39,6 +39,8 @@ typedef enum { /* PCI devices can connect to this bus */ VIR_PCI_CONNECT_TYPE_PCIE = 1 << 3, /* PCI Express devices can connect to this bus */ + VIR_PCI_CONNECT_TYPE_PCIE_ROOT = 1 << 4, + /* for devices that can only connect to pcie-root (i.e. root-port) */ } virDomainPCIConnectFlags; typedef struct { @@ -70,7 +72,8 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; * allowed, e.g. PCI, PCIe, switch */ # define VIR_PCI_CONNECT_TYPES_MASK \ - (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE) + (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE | \ + VIR_PCI_CONNECT_TYPE_PCIE_ROOT) /* combination of all bits that could be used to connect a normal * endpoint device (i.e. excluding the connection possible between an diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbad7e9..f1723c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -324,13 +324,15 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", "pcie-root", "pci-bridge", - "dmi-to-pci-bridge") + "dmi-to-pci-bridge", + "pcie-root-port") VIR_ENUM_IMPL(virDomainControllerPCIModelName, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST, "none", "pci-bridge", - "i82801b11-bridge") + "i82801b11-bridge", + "ioh3420") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", @@ -1552,6 +1554,8 @@ virDomainControllerDefNew(virDomainControllerType type) break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: def->opts.pciopts.chassisNr = -1; + def->opts.pciopts.chassis = -1; + def->opts.pciopts.port = -1; break; case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: @@ -7778,6 +7782,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *modelName = NULL; bool processedTarget = false; char *chassisNr = NULL; + char *chassis = NULL; + char *port = NULL; xmlNodePtr saved = ctxt->node; int rc; @@ -7838,6 +7844,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } chassisNr = virXMLPropString(cur, "chassisNr"); + chassis = virXMLPropString(cur, "chassis"); + port = virXMLPropString(cur, "port"); processedTarget = true; } } @@ -7972,6 +7980,57 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } } + if (chassis) { + if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (def->opts.pciopts.chassis < 0 || + def->opts.pciopts.chassisNr > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); + goto error; + } + } + if (chassis) { + if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (def->opts.pciopts.chassis < 0 || + def->opts.pciopts.chassis > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); + goto error; + } + } + if (port) { + if (virStrToLong_i(port, NULL, 0, + &def->opts.pciopts.port) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid port '%s' in PCI controller"), + port); + goto error; + } + if (def->opts.pciopts.port < 0 || + def->opts.pciopts.port > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller port '%s' out of range " + "- must be 0-255"), + port); + goto error; + } + } break; default: @@ -7999,6 +8058,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(max_sectors); VIR_FREE(modelName); VIR_FREE(chassisNr); + VIR_FREE(chassis); + VIR_FREE(port); return def; @@ -19051,7 +19112,9 @@ virDomainControllerDefFormat(virBufferPtr buf, pcihole64 = true; if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) pciModel = true; - if (def->opts.pciopts.chassisNr != -1) + if (def->opts.pciopts.chassisNr != -1 || + def->opts.pciopts.chassis != -1 || + def->opts.pciopts.port != -1) pciTarget = true; break; @@ -19081,6 +19144,12 @@ virDomainControllerDefFormat(virBufferPtr buf, if (def->opts.pciopts.chassisNr != -1) virBufferAsprintf(buf, " chassisNr='%d'", def->opts.pciopts.chassisNr); + if (def->opts.pciopts.chassis != -1) + virBufferAsprintf(buf, " chassis='%d'", + def->opts.pciopts.chassis); + if (def->opts.pciopts.port != -1) + virBufferAsprintf(buf, " port='0x%x'", + def->opts.pciopts.port); virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 761ce8b..614a2a6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -752,6 +752,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; @@ -760,6 +761,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST } virDomainControllerPCIModelName; @@ -820,7 +822,11 @@ struct _virDomainPCIControllerOpts { * compatibility. */ int chassisNr; /* used by pci-bridge, -1 == unspecified */ - }; + /* chassis & port used by + * pcie-root-port/pcie-switch-downstream-port, -1 = unspecified */ + int chassis; + int port; +}; /* Stores the virtual disk controller configuration */ struct _virDomainControllerDef { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 078c365..7f58f97 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2274,6 +2274,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml new file mode 100644 index 0000000..bc28a71 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='40' port='0x1a'/> + </controller> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7a41a18..6b48bf4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -567,6 +567,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); DO_TEST_DIFFERENT("q35"); + DO_TEST("pcie-root-port"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 2.1.0
On Sat, Jul 25, 2015 at 03:58:30PM -0400, Laine Stump wrote:
This controller can be connected (at domain startup time only - not hotpluggable) only to a port on the pcie root complex ("pcie-root" in libvirt config), hence the new connect type VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that will accept any PCI or PCIe device.
New attributes must be added to the controller <target> subelement for this - chassis and port are guest-visible option values that will be set by libvirt with values derived from the controller's index and pci address information. --- change from V2:
* check chassis/port for 0-255 range * 1.3.0 -> 1.2.18
docs/formatdomain.html.in | 33 +++++++++- docs/schemas/domaincommon.rng | 13 ++++ src/conf/domain_addr.c | 10 ++- src/conf/domain_addr.h | 5 +- src/conf/domain_conf.c | 75 +++++++++++++++++++++- src/conf/domain_conf.h | 8 ++- src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-root-port.xml | 36 +++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fdf7e82..a9db924 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3123,6 +3141,17 @@ auto-determined by libvirt will be placed on this pci-bridge device. (<span class="since">since 1.1.2</span>). </p> + <p> + Domains with an implicit pcie-root can also add controllers + with <code>model='pcie-root-port'</code>. This is a simple type of + bridge device that can connect only to one of the 31 slots on + the pcie-root bus on the upstream side, and makes a single + (PCIe, hotpluggable) port (at slot='0') available on the + downstream side. This controller can be used to provide a single + slot to later hotplug a PCIe device (but is not itself + hotpluggable - it must be in the configuration when the domain + is started). (<span class="since">since 1.2.18</span>)
s/18/19/
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbad7e9..f1723c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7972,6 +7980,57 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } } + if (chassis) { + if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (def->opts.pciopts.chassis < 0 || + def->opts.pciopts.chassisNr > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); + goto error; + } + }
This gets parsed twice, up here ^^ and down here vv, copy-paste error? Remove one of those, please. I, personally don't care much which one, none of them is my favourite :) ACK with that changed
+ if (chassis) { + if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (def->opts.pciopts.chassis < 0 || + def->opts.pciopts.chassis > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); + goto error; + } + } + if (port) { + if (virStrToLong_i(port, NULL, 0, + &def->opts.pciopts.port) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid port '%s' in PCI controller"), + port); + goto error; + } + if (def->opts.pciopts.port < 0 || + def->opts.pciopts.port > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller port '%s' out of range " + "- must be 0-255"), + port); + goto error; + } + } break;
default:
On Mon, Aug 03, 2015 at 12:17:45PM +0200, Martin Kletzander wrote:
On Sat, Jul 25, 2015 at 03:58:30PM -0400, Laine Stump wrote:
This controller can be connected (at domain startup time only - not hotpluggable) only to a port on the pcie root complex ("pcie-root" in libvirt config), hence the new connect type VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that will accept any PCI or PCIe device.
New attributes must be added to the controller <target> subelement for this - chassis and port are guest-visible option values that will be set by libvirt with values derived from the controller's index and pci address information. --- change from V2:
* check chassis/port for 0-255 range * 1.3.0 -> 1.2.18
docs/formatdomain.html.in | 33 +++++++++- docs/schemas/domaincommon.rng | 13 ++++ src/conf/domain_addr.c | 10 ++- src/conf/domain_addr.h | 5 +- src/conf/domain_conf.c | 75 +++++++++++++++++++++- src/conf/domain_conf.h | 8 ++- src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-root-port.xml | 36 +++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fdf7e82..a9db924 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3123,6 +3141,17 @@ auto-determined by libvirt will be placed on this pci-bridge device. (<span class="since">since 1.1.2</span>). </p> + <p> + Domains with an implicit pcie-root can also add controllers + with <code>model='pcie-root-port'</code>. This is a simple type of + bridge device that can connect only to one of the 31 slots on + the pcie-root bus on the upstream side, and makes a single + (PCIe, hotpluggable) port (at slot='0') available on the + downstream side. This controller can be used to provide a single + slot to later hotplug a PCIe device (but is not itself + hotpluggable - it must be in the configuration when the domain + is started). (<span class="since">since 1.2.18</span>)
s/18/19/
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbad7e9..f1723c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7972,6 +7980,57 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } } + if (chassis) { + if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (def->opts.pciopts.chassis < 0 || + def->opts.pciopts.chassisNr > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); + goto error; + } + }
This gets parsed twice, up here ^^ and down here vv, copy-paste error? Remove one of those, please. I, personally don't care much which one, none of them is my favourite :)
Well, John has a favourite, though. And it makes sense as well... The upper one is comparing chassisNr > 255 but it should be 'chassis' instead, I totally missed that. Thanks John!
ACK with that changed
+ if (chassis) { + if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (def->opts.pciopts.chassis < 0 || + def->opts.pciopts.chassis > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); + goto error; + } + } + if (port) { + if (virStrToLong_i(port, NULL, 0, + &def->opts.pciopts.port) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid port '%s' in PCI controller"), + port); + goto error; + } + if (def->opts.pciopts.port < 0 || + def->opts.pciopts.port > 255) { + virReportError(VIR_ERR_XML_ERROR, + _("PCI controller port '%s' out of range " + "- must be 0-255"), + port); + goto error; + } + } break;
default:
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This is backed by the qemu device ioh3420. chassis and port from the <target> subelement are used to store/set the respective qemu device options for the ioh3420. Currently, chassis is set to be the index of the controller, and port is set to "(slot << 3) + function" (per suggestion from Alex Williamson). --- change from V2: * adjust for enum-based model name * verify valid chassis/port when building commandline * add a negative test for too many root-ports src/qemu/qemu_command.c | 50 ++++++++++++++++++ .../qemuxml2argv-pcie-root-port-too-many.xml | 60 ++++++++++++++++++++++ .../qemuxml2argv-pcie-root-port.args | 10 ++++ tests/qemuxml2argvtest.c | 15 ++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 136 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port-too-many.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7f58f97..187195f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1606,6 +1606,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = VIR_PCI_CONNECT_TYPE_PCIE; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + /* pcie-root-port can only connect to pcie-root, isn't + * hot-pluggable + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; + break; default: break; } @@ -2275,6 +2281,13 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420; + if (options->chassis == -1) + options->chassis = cont->idx; + if (options->port == -1) + options->port = (addr->slot << 3) + addr->function; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: @@ -2386,6 +2399,10 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, */ flags = VIR_PCI_CONNECT_TYPE_PCIE; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + /* pcie-root-port can only plug into pcie-root */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; + break; default: flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI; break; @@ -4716,6 +4733,39 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-root-port options not set")); + goto error; + } + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-root-port model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pcie-root-port"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-root-port (ioh3420) " + "controller is not supported in this QEMU binary")); + goto error; + } + + virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", + modelName, def->opts.pciopts.port, + def->opts.pciopts.chassis, def->info.alias); + break; } break; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port-too-many.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port-too-many.xml new file mode 100644 index 0000000..910183e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port-too-many.xml @@ -0,0 +1,60 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='pcie-root-port'/> + <controller type='pci' index='5' model='pcie-root-port'/> + <controller type='pci' index='6' model='pcie-root-port'/> + <controller type='pci' index='7' model='pcie-root-port'/> + <controller type='pci' index='8' model='pcie-root-port'/> + <controller type='pci' index='9' model='pcie-root-port'/> + <controller type='pci' index='10' model='pcie-root-port'/> + <controller type='pci' index='11' model='pcie-root-port'/> + <controller type='pci' index='12' model='pcie-root-port'/> + <controller type='pci' index='13' model='pcie-root-port'/> + <controller type='pci' index='14' model='pcie-root-port'/> + <controller type='pci' index='15' model='pcie-root-port'/> + <controller type='pci' index='16' model='pcie-root-port'/> + <controller type='pci' index='17' model='pcie-root-port'/> + <controller type='pci' index='18' model='pcie-root-port'/> + <controller type='pci' index='19' model='pcie-root-port'/> + <controller type='pci' index='20' model='pcie-root-port'/> + <controller type='pci' index='21' model='pcie-root-port'/> + <controller type='pci' index='22' model='pcie-root-port'/> + <controller type='pci' index='23' model='pcie-root-port'/> + <controller type='pci' index='24' model='pcie-root-port'/> + <controller type='pci' index='25' model='pcie-root-port'/> + <controller type='pci' index='26' model='pcie-root-port'/> + <controller type='pci' index='27' model='pcie-root-port'/> + <controller type='pci' index='28' model='pcie-root-port'/> + <controller type='pci' index='29' model='pcie-root-port'/> + <controller type='pci' index='30' model='pcie-root-port'/> + <controller type='pci' index='31' model='pcie-root-port'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args new file mode 100644 index 0000000..5e4891c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device ioh3420,port=0x1a,chassis=40,id=pci.4,bus=pcie.0,addr=0x3 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f9b30d9..b429bc5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1500,6 +1500,21 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("pcie-root-port", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + + DO_TEST_ERROR("pcie-root-port-too-many", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6b48bf4..74afdb4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -568,6 +568,7 @@ mymain(void) DO_TEST_DIFFERENT("pcie-root"); DO_TEST_DIFFERENT("q35"); DO_TEST("pcie-root-port"); + DO_TEST("pcie-root-port-too-many"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 2.1.0
This is the upstream part of a PCIe switch. It connects to a PCIe port (but not PCI) on the upstream side, and can have up to 31 xio3130-downstream controllers (but no other types of devices) connected to its downstream side. This device will be used to implement the "pcie-switch" model of pci controller. --- change for V2: rebase src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemuhelptest.c | 6 ++++-- 10 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a9dc395..c2ec46d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -289,6 +289,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "vhost-user-multiqueue", /* 190 */ "migration-event", "ioh3420", + "x3130-upstream", ); @@ -1570,6 +1571,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pc-dimm", QEMU_CAPS_DEVICE_PC_DIMM }, { "pci-serial", QEMU_CAPS_DEVICE_PCI_SERIAL }, { "ioh3420", QEMU_CAPS_DEVICE_IOH3420 }, + { "x3130-upstream", QEMU_CAPS_DEVICE_X3130_UPSTREAM }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f179e0b..e677065 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -232,6 +232,7 @@ typedef enum { QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */ QEMU_CAPS_MIGRATION_EVENT = 191, /* MIGRATION event */ QEMU_CAPS_DEVICE_IOH3420 = 192, /* -device ioh3420 */ + QEMU_CAPS_DEVICE_X3130_UPSTREAM = 193, /* -device x3130-upstream */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps index a1fafa6..78d7b82 100644 --- a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps @@ -121,4 +121,5 @@ <flag name='qxl.vgamem_mb'/> <flag name='qxl-vga.vgamem_mb'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps index 824ef02..7cec7f9 100644 --- a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps @@ -136,4 +136,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps index 7ef5199..f5f0034 100644 --- a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps @@ -137,4 +137,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps index 8c3d48e..9f0461a 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps @@ -146,4 +146,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps index 72f7625..1b23b82 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps @@ -152,4 +152,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps index d81c80c..ff0427f 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps @@ -152,4 +152,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='pci-serial'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index 1a39dce..56b27e5 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -168,4 +168,5 @@ <flag name='pc-dimm'/> <flag name='pci-serial'/> <flag name='ioh3420'/> + <flag name='x3130-upstream'/> </qemuCaps> diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 6211640..62b9a0c 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -754,7 +754,8 @@ mymain(void) QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_SPLASH_TIMEOUT, QEMU_CAPS_DEVICE_IVSHMEM, - QEMU_CAPS_DEVICE_IOH3420); + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -855,7 +856,8 @@ mymain(void) QEMU_CAPS_OBJECT_USB_AUDIO, QEMU_CAPS_SPLASH_TIMEOUT, QEMU_CAPS_DEVICE_IVSHMEM, - QEMU_CAPS_DEVICE_IOH3420); + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM); DO_TEST_FULL("qemu-1.2.0", 1002000, 0, 0, VIR_ERR_CONFIG_UNSUPPORTED, QEMU_CAPS_LAST); DO_TEST_FULL("qemu-kvm-1.2.0", 1002000, 1, 0, VIR_ERR_CONFIG_UNSUPPORTED, -- 2.1.0
This controller can be connected only to a pcie-root-port or a pcie-switch-downstream-port (which will be added in a later patch), which is the reason for the new connect type VIR_PCI_CONNECT_TYPE_PCIE_PORT. A pcie-switch-upstream-port provides 32 ports (slot=0 to slot=31) on the downstream side, which can only have pci controllers of model "pcie-switch-downstream-port" plugged into them, which is the reason for the other new connect type VIR_PCI_CONNECT_TYPE_PCIE_SWITCH. --- change from V2: * 1.3.0 -> 1.2.18 * NB: fuller description of upstream-port usage is in commit for downstream-port, since it's unusable until then anyway. docs/formatdomain.html.in | 5 +-- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 15 +++++++-- src/conf/domain_addr.h | 9 +++++- src/conf/domain_conf.c | 6 ++-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-switch-upstream-port.xml | 37 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 72 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a9db924..d033542 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3030,10 +3030,11 @@ PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, <code>pcie-root-port</code>, <code>pci-bridge</code>, - or <code>dmi-to-pci-bridge</code>. + <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</code>. (pci-root and pci-bridge <span class="since">since 1.0.5</span>, pcie-root and dmi-to-pci-bridge <span class="since">since - 1.1.2</span>, pcie-root-port <span class="since">since 1.2.18</span>) + 1.1.2</span>, pcie-root-port and + pcie-switch-upstream-port <span class="since">since 1.2.18</span>) The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in kilobytes, or in the unit specified by <code>pcihole64</code>'s diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7d7f412..ce01230 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1741,6 +1741,8 @@ <value>i82801b11-bridge</value> <!-- implementations of 'pcie-root-port' --> <value>ioh3420</value> + <!-- implementations of 'pcie-switch-upstream-port' --> + <value>x3130-upstream</value> </choice> </attribute> <empty/> @@ -1787,6 +1789,7 @@ <value>pci-bridge</value> <value>dmi-to-pci-bridge</value> <value>pcie-root-port</value> + <value>pcie-switch-upstream-port</value> </choice> </attribute> </group> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index c2a207b..8bd4ac3 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -197,11 +197,22 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - /* provides one slot which is pcie and hotpluggable */ - bus->flags = VIR_PCI_CONNECT_TYPE_PCIE | VIR_PCI_CONNECT_HOTPLUGGABLE; + /* provides one slot which is pcie, can be used by devices + * that must connect to some type of "pcie-*-port", and + * is hotpluggable + */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE + | VIR_PCI_CONNECT_TYPE_PCIE_PORT + | VIR_PCI_CONNECT_HOTPLUGGABLE; bus->minSlot = 0; bus->maxSlot = 0; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + /* 31 slots, can only accept pcie-switch-port, no hotplug */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH; + bus->minSlot = 0; + bus->maxSlot = 31; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 2a0ff96..2220a79 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -41,6 +41,12 @@ typedef enum { /* PCI Express devices can connect to this bus */ VIR_PCI_CONNECT_TYPE_PCIE_ROOT = 1 << 4, /* for devices that can only connect to pcie-root (i.e. root-port) */ + VIR_PCI_CONNECT_TYPE_PCIE_PORT = 1 << 5, + /* devices that can only connect to a pcie-root-port + * or pcie-downstream-switch-port + */ + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH = 1 << 6, + /* devices that can only connect to a pcie-switch */ } virDomainPCIConnectFlags; typedef struct { @@ -73,7 +79,8 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; */ # define VIR_PCI_CONNECT_TYPES_MASK \ (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE | \ - VIR_PCI_CONNECT_TYPE_PCIE_ROOT) + VIR_PCI_CONNECT_TYPE_PCIE_ROOT | VIR_PCI_CONNECT_TYPE_PCIE_PORT | \ + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH) /* combination of all bits that could be used to connect a normal * endpoint device (i.e. excluding the connection possible between an diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f1723c0..f63972d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -325,14 +325,16 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pcie-root", "pci-bridge", "dmi-to-pci-bridge", - "pcie-root-port") + "pcie-root-port", + "pcie-switch-upstream-port") VIR_ENUM_IMPL(virDomainControllerPCIModelName, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST, "none", "pci-bridge", "i82801b11-bridge", - "ioh3420") + "ioh3420", + "x3130-upstream") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 614a2a6..a82e955 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -753,6 +753,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; @@ -762,6 +763,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST } virDomainControllerPCIModelName; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 187195f..e3ee911 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2288,6 +2288,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (options->port == -1) options->port = (addr->slot << 3) + addr->function; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml new file mode 100644 index 0000000..a451d6c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='pcie-root-port'/> + <controller type='pci' index='5' model='pcie-switch-upstream-port'/> + <controller type='pci' index='6' model='pcie-switch-upstream-port'> + <model name='x3130-upstream'/> + </controller> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 74afdb4..9a6fb16 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -569,6 +569,7 @@ mymain(void) DO_TEST_DIFFERENT("q35"); DO_TEST("pcie-root-port"); DO_TEST("pcie-root-port-too-many"); + DO_TEST("pcie-switch-upstream-port"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 2.1.0
this is backed by the qemu device x3130-upstream. It can only plug into a pcie-root-port or pcie-switch-downstream-port. --- change from V2: * adjust for enum-based model name src/qemu/qemu_command.c | 46 ++++++++++++++++++++++ .../qemuxml2argv-pcie-switch-upstream-port.args | 12 ++++++ tests/qemuxml2argvtest.c | 9 +++++ 3 files changed, 67 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e3ee911..f7f102f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1612,6 +1612,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + /* pcie-switch can only connect to a true + * pcie bus, and can't be hot-plugged. + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; + break; default: break; } @@ -2289,6 +2295,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, options->port = (addr->slot << 3) + addr->function; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: @@ -2404,6 +2413,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* pcie-root-port can only plug into pcie-root */ flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + /* pcie-switch really does need a real PCIe + * port, but it doesn't need to be pcie-root + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; + break; default: flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI; break; @@ -4767,6 +4782,37 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, modelName, def->opts.pciopts.port, def->opts.pciopts.chassis, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-switch-upstream-port options not set")); + goto error; + } + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-switch-upstream-port model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pcie-switch-upstream-port"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-switch-upstream-port (x3130-upstream) " + "controller is not supported in this QEMU binary")); + goto error; + } + + virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); + break; } break; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args new file mode 100644 index 0000000..c08cd4a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args @@ -0,0 +1,12 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device ioh3420,port=0x18,chassis=4,id=pci.4,bus=pcie.0,addr=0x3 \ +-device x3130-upstream,id=pci.5,bus=pci.3,addr=0x0 \ +-device x3130-upstream,id=pci.6,bus=pci.4,addr=0x0 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b429bc5..651b860 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1516,6 +1516,15 @@ mymain(void) QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("pcie-switch-upstream-port", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, -- 2.1.0
The downstream ports of an x3130-upstream switch can each have one of these plugged into them (and that is the only place they can be connected). Each xio3130-downstream provides a single PCIe port that can have PCI or PCIe devices hotplugged into it. Apparently an entire set of x3130-upstream + several xio3130-downstreams can be hotplugged as a unit, but it's not clear to me yet how that would be done, since qemu only allows attaching a single device at a time. This device will be used to implement the "pcie-switch-port" model of pci controller. --- change for V2: rebase src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemuhelptest.c | 6 ++++-- 10 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c2ec46d..54f51c1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -290,6 +290,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "migration-event", "ioh3420", "x3130-upstream", + "xio3130-downstream", ); @@ -1572,6 +1573,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pci-serial", QEMU_CAPS_DEVICE_PCI_SERIAL }, { "ioh3420", QEMU_CAPS_DEVICE_IOH3420 }, { "x3130-upstream", QEMU_CAPS_DEVICE_X3130_UPSTREAM }, + { "xio3130-downstream", QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e677065..2429ff7 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -233,6 +233,7 @@ typedef enum { QEMU_CAPS_MIGRATION_EVENT = 191, /* MIGRATION event */ QEMU_CAPS_DEVICE_IOH3420 = 192, /* -device ioh3420 */ QEMU_CAPS_DEVICE_X3130_UPSTREAM = 193, /* -device x3130-upstream */ + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM = 194, /* -device xio3130-downstream */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps index 78d7b82..ba16635 100644 --- a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps @@ -122,4 +122,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps index 7cec7f9..51cd6d9 100644 --- a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps @@ -137,4 +137,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps index f5f0034..03d0a3e 100644 --- a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps @@ -138,4 +138,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps index 9f0461a..e2f22e4 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps @@ -147,4 +147,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps index 1b23b82..874a050 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps @@ -153,4 +153,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps index ff0427f..dd3bcda 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps @@ -153,4 +153,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index 56b27e5..3ee2d6f 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -169,4 +169,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 62b9a0c..8f317d4 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -755,7 +755,8 @@ mymain(void) QEMU_CAPS_SPLASH_TIMEOUT, QEMU_CAPS_DEVICE_IVSHMEM, QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_DEVICE_X3130_UPSTREAM); + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -857,7 +858,8 @@ mymain(void) QEMU_CAPS_SPLASH_TIMEOUT, QEMU_CAPS_DEVICE_IVSHMEM, QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_DEVICE_X3130_UPSTREAM); + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); DO_TEST_FULL("qemu-1.2.0", 1002000, 0, 0, VIR_ERR_CONFIG_UNSUPPORTED, QEMU_CAPS_LAST); DO_TEST_FULL("qemu-kvm-1.2.0", 1002000, 1, 0, VIR_ERR_CONFIG_UNSUPPORTED, -- 2.1.0
This controller can be connected only to a port on a pcie-switch-upstream-port. It provides a single hotpluggable port that will accept any PCI or PCIe device, as well as any device requiring a pcie-*-port (the only current example of such a device is the pcie-switch-upstream-port). --- change for V2: * 1.3.0 -> 1.2.18 * possibly reworded documention a bit. docs/formatdomain.html.in | 49 +++++++++++++++------- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 11 +++++ src/conf/domain_conf.c | 6 ++- src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-switch-downstream-port.xml | 44 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 100 insertions(+), 17 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d033542..2f61ac8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3030,11 +3030,12 @@ PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, <code>pcie-root-port</code>, <code>pci-bridge</code>, - <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</code>. + <code>dmi-to-pci-bridge</code>, <code>pcie-switch-upstream-port</code>, or + <code>pcie-switch-downstream-port</code>. (pci-root and pci-bridge <span class="since">since 1.0.5</span>, pcie-root and dmi-to-pci-bridge <span class="since">since - 1.1.2</span>, pcie-root-port and - pcie-switch-upstream-port <span class="since">since 1.2.18</span>) + 1.1.2</span>, pcie-root-port, pcie-switch-upstream-port, and + pcie-switch-downstream-port <span class="since">since 1.2.18</span>) The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in kilobytes, or in the unit specified by <code>pcihole64</code>'s @@ -3081,8 +3082,8 @@ </dd> <dt><code>chassis</code></dt> <dd> - pcie-root-port controllers can also have - a <code>chassis</code> attribute in + pcie-root-port and pcie-switch-downstream-port controllers can + also have a <code>chassis</code> attribute in the <code><target></code> subelement, which is used to set the controller's "chassis" configuration value, which is visible to the virtual machine. If set, chassis must be @@ -3090,8 +3091,9 @@ </dd> <dt><code>port</code></dt> <dd> - pcie-root-port controllers can also have a <code>port</code> - attribute in the <code><target></code> subelement, which + pcie-root-port and pcie-switch-downstream-port controllers can + also have a <code>port</code> attribute in + the <code><target></code> subelement, which is used to set the controller's "port" configuration value, which is visible to the virtual machine. If set, port must be between 0 and 255. @@ -3144,14 +3146,31 @@ </p> <p> Domains with an implicit pcie-root can also add controllers - with <code>model='pcie-root-port'</code>. This is a simple type of - bridge device that can connect only to one of the 31 slots on - the pcie-root bus on the upstream side, and makes a single - (PCIe, hotpluggable) port (at slot='0') available on the - downstream side. This controller can be used to provide a single - slot to later hotplug a PCIe device (but is not itself - hotpluggable - it must be in the configuration when the domain - is started). (<span class="since">since 1.2.18</span>) + with <code>model='pcie-root-port'</code>, + <code>model='pcie-switch-upstream-port'</code>, + and <code>model='pcie-switch-downstream-port'</code>. pcie-root-port + is a simple type of bridge device that can connect only to one + of the 31 slots on the pcie-root bus on its upstream side, and + makes a single (PCIe, hotpluggable) port available on the + downstream side (at slot='0'). pcie-root-port can be used to + provide a single slot to later hotplug a PCIe device (but is not + itself hotpluggable - it must be in the configuration when the + domain is started). + (<span class="since">since 1.2.18</span>) + </p> + <p> + pcie-switch-upstream-port is a more flexible (but also more + complex) device that can only plug into a pcie-root-port or + pcie-switch-downstream-port on the upstream side (and only + before the domain is started - it is not hot-pluggable), and + provides 32 ports on the downstream side (slot='0' - slot='31') + that accept only pcie-switch-downstream-port devices; each + pcie-switch-downstream-port device can only plug into a + pcie-switch-upstream-port on its upstream side (again, not + hot-pluggable), and on its downstream side provides a single + hotpluggable pcie port that can accept any standard pci or pcie + device (or another pcie-switch-upstream-port). + (<span class="since">since 1.2.18</span>) </p> <pre> ... diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ce01230..ab02d14 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1743,6 +1743,8 @@ <value>ioh3420</value> <!-- implementations of 'pcie-switch-upstream-port' --> <value>x3130-upstream</value> + <!-- implementations of 'pcie-switch-downstream-port' --> + <value>xio3130-downstream</value> </choice> </attribute> <empty/> @@ -1790,6 +1792,7 @@ <value>dmi-to-pci-bridge</value> <value>pcie-root-port</value> <value>pcie-switch-upstream-port</value> + <value>pcie-switch-downstream-port</value> </choice> </attribute> </group> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8bd4ac3..561d8cc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -213,6 +213,17 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 0; bus->maxSlot = 31; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + /* provides one slot which is pcie, can be used by devices + * that must connect to some type of "pcie-*-port", and + * is hotpluggable + */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE + | VIR_PCI_CONNECT_TYPE_PCIE_PORT + | VIR_PCI_CONNECT_HOTPLUGGABLE; + bus->minSlot = 0; + bus->maxSlot = 0; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f63972d..0cf111a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -326,7 +326,8 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-bridge", "dmi-to-pci-bridge", "pcie-root-port", - "pcie-switch-upstream-port") + "pcie-switch-upstream-port", + "pcie-switch-downstream-port") VIR_ENUM_IMPL(virDomainControllerPCIModelName, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST, @@ -334,7 +335,8 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName, "pci-bridge", "i82801b11-bridge", "ioh3420", - "x3130-upstream") + "x3130-upstream", + "xio3130-downstream") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a82e955..440d74d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -754,6 +754,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; @@ -764,6 +765,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST } virDomainControllerPCIModelName; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f7f102f..4e20658 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2298,6 +2298,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml new file mode 100644 index 0000000..fca3149 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml @@ -0,0 +1,44 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='pcie-switch-upstream-port'/> + <controller type='pci' index='5' model='pcie-switch-downstream-port'/> + <controller type='pci' index='6' model='pcie-switch-downstream-port'/> + <controller type='pci' index='7' model='pcie-switch-downstream-port'/> + <controller type='pci' index='8' model='pcie-switch-downstream-port'> + <model name='xio3130-downstream'/> + <target chassis='30' port='0x27'/> + </controller> + <controller type='pci' index='9' model='pcie-switch-upstream-port'/> + <controller type='pci' index='10' model='pcie-switch-downstream-port'/> + <controller type='pci' index='11' model='pcie-switch-downstream-port'/> + <controller type='pci' index='12' model='pcie-switch-downstream-port'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 9a6fb16..b261db2 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -570,6 +570,7 @@ mymain(void) DO_TEST("pcie-root-port"); DO_TEST("pcie-root-port-too-many"); DO_TEST("pcie-switch-upstream-port"); + DO_TEST("pcie-switch-downstream-port"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 2.1.0
On Sat, Jul 25, 2015 at 03:58:36PM -0400, Laine Stump wrote:
This controller can be connected only to a port on a pcie-switch-upstream-port. It provides a single hotpluggable port that will accept any PCI or PCIe device, as well as any device requiring a pcie-*-port (the only current example of such a device is the pcie-switch-upstream-port). --- change for V2: * 1.3.0 -> 1.2.18 * possibly reworded documention a bit.
docs/formatdomain.html.in | 49 +++++++++++++++------- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 11 +++++ src/conf/domain_conf.c | 6 ++- src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-switch-downstream-port.xml | 44 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 100 insertions(+), 17 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d033542..2f61ac8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3030,11 +3030,12 @@ PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, <code>pcie-root-port</code>, <code>pci-bridge</code>, - <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</code>. + <code>dmi-to-pci-bridge</code>, <code>pcie-switch-upstream-port</code>, or + <code>pcie-switch-downstream-port</code>. (pci-root and pci-bridge <span class="since">since 1.0.5</span>, pcie-root and dmi-to-pci-bridge <span class="since">since - 1.1.2</span>, pcie-root-port and - pcie-switch-upstream-port <span class="since">since 1.2.18</span>) + 1.1.2</span>, pcie-root-port, pcie-switch-upstream-port, and + pcie-switch-downstream-port <span class="since">since 1.2.18</span>)
s/18/19/ everywhere
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8bd4ac3..561d8cc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -213,6 +213,17 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 0; bus->maxSlot = 31; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
This could be merged with VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, what's the difference then? ACK if this is just a readability enhancement and is supposed to be this way.
+ /* provides one slot which is pcie, can be used by devices + * that must connect to some type of "pcie-*-port", and + * is hotpluggable + */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE + | VIR_PCI_CONNECT_TYPE_PCIE_PORT + | VIR_PCI_CONNECT_HOTPLUGGABLE; + bus->minSlot = 0; + bus->maxSlot = 0; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model);
On 08/03/2015 06:23 AM, Martin Kletzander wrote:
On Sat, Jul 25, 2015 at 03:58:36PM -0400, Laine Stump wrote:
This controller can be connected only to a port on a pcie-switch-upstream-port. It provides a single hotpluggable port that will accept any PCI or PCIe device, as well as any device requiring a pcie-*-port (the only current example of such a device is the pcie-switch-upstream-port). --- change for V2: * 1.3.0 -> 1.2.18 * possibly reworded documention a bit.
docs/formatdomain.html.in | 49 +++++++++++++++------- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 11 +++++ src/conf/domain_conf.c | 6 ++- src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-switch-downstream-port.xml | 44 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 100 insertions(+), 17 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d033542..2f61ac8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3030,11 +3030,12 @@ PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, <code>pcie-root-port</code>, <code>pci-bridge</code>, - <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</code>. + <code>dmi-to-pci-bridge</code>, <code>pcie-switch-upstream-port</code>, or + <code>pcie-switch-downstream-port</code>. (pci-root and pci-bridge <span class="since">since 1.0.5</span>, pcie-root and dmi-to-pci-bridge <span class="since">since - 1.1.2</span>, pcie-root-port and - pcie-switch-upstream-port <span class="since">since 1.2.18</span>) + 1.1.2</span>, pcie-root-port, pcie-switch-upstream-port, and + pcie-switch-downstream-port <span class="since">since 1.2.18</span>)
s/18/19/ everywhere
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8bd4ac3..561d8cc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -213,6 +213,17 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 0; bus->maxSlot = 31; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
This could be merged with VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, what's the difference then? ACK if this is just a readability enhancement and is supposed to be this way.
It's that way because I didn't know for certain that there wouldn't be some difference between the two that needed to be accounted for (and at one point they were different, but that was due to my naive understanding of the differences between the different types of controllers). I noticed the similarity fairly late in the game, but forgot to do anything about it. I've now combined the two cases.
This is backed by the qemu device xio3130-downstream. It can only be connected to a pcie-switch-upstream-port (x3130-upstream) on the upstream side. --- change from V2: * adjust for enum-based model name src/qemu/qemu_command.c | 54 ++++++++++++++++++++++ .../qemuxml2argv-pcie-switch-downstream-port.args | 18 ++++++++ tests/qemuxml2argvtest.c | 9 ++++ 3 files changed, 81 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4e20658..11037c4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1618,6 +1618,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + /* pcie-switch-downstream-port can only connect to a + * pcie-switch-upstream-port, and can't be hot-plugged. + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH; + break; default: break; } @@ -2299,6 +2305,13 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM; + if (options->chassis == -1) + options->chassis = cont->idx; + if (options->port == -1) + options->port = addr->slot; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: @@ -2420,6 +2433,10 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, */ flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + /* pcie-switch-port can only plug into pcie-switch */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH; + break; default: flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI; break; @@ -4814,6 +4831,43 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (def->opts.pciopts.modelName + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || + def->opts.pciopts.chassis == -1 || + def->opts.pciopts.port == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-switch-downstream-port " + "options not set")); + goto error; + } + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-switch-downstream-port model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " + "is not valid for a pcie-switch-downstream-port"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The pcie-switch-downstream-port " + "(xio3130-downstream) controller " + "is not supported in this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", + modelName, def->opts.pciopts.port, + def->opts.pciopts.chassis, def->info.alias); + break; } break; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args new file mode 100644 index 0000000..fefefa2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args @@ -0,0 +1,18 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device x3130-upstream,id=pci.4,bus=pci.3,addr=0x0 \ +-device xio3130-downstream,port=0x0,chassis=5,id=pci.5,bus=pci.4,addr=0x0 \ +-device xio3130-downstream,port=0x1,chassis=6,id=pci.6,bus=pci.4,addr=0x1 \ +-device xio3130-downstream,port=0x2,chassis=7,id=pci.7,bus=pci.4,addr=0x2 \ +-device xio3130-downstream,port=0x27,chassis=30,id=pci.8,bus=pci.4,addr=0x3 \ +-device x3130-upstream,id=pci.9,bus=pci.5,addr=0x0 \ +-device xio3130-downstream,port=0x4,chassis=10,id=pci.10,bus=pci.4,addr=0x4 \ +-device xio3130-downstream,port=0x5,chassis=11,id=pci.11,bus=pci.4,addr=0x5 \ +-device xio3130-downstream,port=0x6,chassis=12,id=pci.12,bus=pci.4,addr=0x6 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 651b860..e743bbe 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1524,6 +1524,15 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("pcie-switch-downstream-port", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, -- 2.1.0
On Sat, Jul 25, 2015 at 03:58:24PM -0400, Laine Stump wrote:
Since the first 3 patches of V2 were ACKed and uncontroversial, I fixed the small problems pointed out in the reviews and pushed them. Thus, Patches 01-13 here correspond to Patches 04-17 in V2.
4 patches with the DHCP rework that gotten there by mistake :-) It took me a while to find that out.
Most of these patches were already ACKed in V2 (pending my making small fixes pointed out in review), but the main things that need review are:
1) changing of model name from a char* to an enum in Patch 01, and corresponding blowback in patches 02, 07, 10, and 13
2) range checking of chassisNr in Patch 03 and chassis+port in patch 06.
3) check for duplicate <model> in patch 01, and duplicate <target> in patch 03.
I did add one new negative test, and reworded some documentation, but I'm
I haven't found one, but it's not needed, that was just a suggestion from some ignorant guy I guess (me).
about to go mostly offline for 10 days, and would rather not have these patches bitrotting during that time if they are okay other than that. (also, I see both of those tasks as having no practical end, but do give my word to add more to both in later followups).
If by chance everything is ACKed before DV freezes for RC1, but after I'm already offline (which will happen Sunday morning U.S. east coast time), I would appreciate if the reviewer could push the patches so they'll get the RC testing and be in the 1.2.18 release. (If not, I'll take care of it when I return).
Unfortunately, I was mostly away for the whole week as well, I got to reading my mail for few minutes a day. And I haven't managed to go through this over the weekend, I figured since rc2 was out already, this needs to wait anyway. Only patches 06 and 12 have some needed work in, all other mails are just either suggestions for future re-factors or random rants. ACK series with a) all 1.2.18 occurrences changed to 1.2.19 (I've probably missed most of them) and b) reviews for 6 and 12 worked in. Have a nice day, Martin
On 07/25/2015 03:58 PM, Laine Stump wrote:
Since the first 3 patches of V2 were ACKed and uncontroversial, I fixed the small problems pointed out in the reviews and pushed them. Thus, Patches 01-13 here correspond to Patches 04-17 in V2.
Most of these patches were already ACKed in V2 (pending my making small fixes pointed out in review), but the main things that need review are:
1) changing of model name from a char* to an enum in Patch 01, and corresponding blowback in patches 02, 07, 10, and 13
2) range checking of chassisNr in Patch 03 and chassis+port in patch 06.
3) check for duplicate <model> in patch 01, and duplicate <target> in patch 03.
I did add one new negative test, and reworded some documentation, but I'm about to go mostly offline for 10 days, and would rather not have these patches bitrotting during that time if they are okay other than that. (also, I see both of those tasks as having no practical end, but do give my word to add more to both in later followups).
If by chance everything is ACKed before DV freezes for RC1, but after I'm already offline (which will happen Sunday morning U.S. east coast time), I would appreciate if the reviewer could push the patches so they'll get the RC testing and be in the 1.2.18 release. (If not, I'll take care of it when I return).
P.S. I ran "git rebase -i master -x "make -j8 check && make -j8 syntax-check" before sending.
Like Martin noted, I've been looking through this series as well. I too haven't had a ton of time to devote to it over the last week or so since I'm in the middle of a move. Also being so close to freeze - it just didn't feel right to shimmy it in there late. This should go in as soon as possible for 1.2.19 to hopefully weed out any weird issues! Some more specific/general comments... The 1.2.18 -> 1.2.19 everywhere will need to be done. The commit message in 2/13 seems to have incurred a disjoint thought or perhaps a forgotten edit in the paragraph that starts "The defaults are set..." - the last sentance just doesn't read well. I had some concern over confusion with 'chassis' and 'chassisNr'... I also had concern over whether the parser could confuse chassis values depending on order seen - as in was a parse of 'chassis' an exact match or a prefix match... I think it's ok especially since one is used for pci-bridge and the other for pcie-root-port... I'm still not sure if it's "best" to set chassisNr or make it some sort of tristate where if we know it wasn't read in from the XML, then we default or use the 'cont->idx' internally when formulating the qemu command. That way at least we don't run into the situation where if "index" changes, then we don't run into some issue since we don't follow on with the chassisNr change (because we cannot be sure who set it). This is patch 4/13 stuff. I think it's fine as a followup to adjust that logic if necessary - hopefully something that can be done in the 1.2.19 timeframe though. This would change the logic in qemuBuildControllerDevStr in the def->model switch for VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE to perhaps set a local chassisNr value based on opts.pciopts.chassisNr == -1, then use index, else use what was set. Beyond that and the duplicate 'chassis' parse in patch 6, things were find and ready to push with a few adjustments. Since I know you're on the road too, I'll watch to see if you get to it - if not, I will make a couple of adjustments for you. John
On 08/03/2015 08:33 AM, John Ferlan wrote:
Like Martin noted, I've been looking through this series as well. I too haven't had a ton of time to devote to it over the last week or so since I'm in the middle of a move. Also being so close to freeze - it just didn't feel right to shimmy it in there late. This should go in as soon as possible for 1.2.19 to hopefully weed out any weird issues! Some more specific/general comments... The 1.2.18 -> 1.2.19 everywhere will need to be done. The commit message in 2/13 seems to have incurred a disjoint thought or perhaps a forgotten edit in the paragraph that starts "The defaults are set..." - the last sentance just doesn't read well. I had some concern over confusion with 'chassis' and 'chassisNr'...
I've had problems in the past when I made the assumption that two apparently mutually exclusive things could share the same space. One day you get to a place where that isn't true, then you're screwed. Kind of like when you're time traveling and see a former/future version of yourself, and then time-space asplodes and there's no big sloppy mess to clean up because the universe has collapsed into a black hole or something... Kind of.
I also had concern over whether the parser could confuse chassis values depending on order seen - as in was a parse of 'chassis' an exact match or a prefix match...
I don't get this one - the comparison is on the entire string, not on a prefix of the string, so there can be no confusion.
I think it's ok especially since one is used for pci-bridge and the other for pcie-root-port...
I'm still not sure if it's "best" to set chassisNr or make it some sort of tristate where if we know it wasn't read in from the XML, then we default or use the 'cont->idx' internally when formulating the qemu command.
The whole point of auto-setting it in the xml is to assure that it will never change "by accident" in the future. If we imply some value when it isn't explicitly set in the xml, then at some later date decide to change how we derive the implicit value, then we will have broken ABI of the guest (albeit in a probably inconsequential way, since my understanding is that essentially nobody uses these values :-/), and we don't want to do that.
That way at least we don't run into the situation where if "index" changes, then we don't run into some issue since we don't follow on with the chassisNr change (because we cannot be sure who set it).
Well, there are two conflicting situations we want to avoid: 1) having the chassisNr changed in the guest because libvirt internally changed the way it implicitly arrived at the value to use for chassisNr, and 2) having the chassisNr be in conflict with the chassisNr of some other pci controller because the user modified their config by both changing the index of this controller *and* adding a new controller of the same type/model in the old index. The possibility of either is, quite frankly, rather remote. But in case (1) this "something bad" would happen with no conscious action on the part of the user (other than upgrading libvirt), so if it caused problems it would confuse/confound them. In case (2), at least it would be happening after they messed with the config (in a manner that almost nobody will ever do), so they might at least have some kind of clue what caused the problem. Note that what we're doing here follows exactly the same behavior that we follow with network device model types - if no model type is given when the interface is added to the domain, rlt8139 is automatically added. We used to just leave it empty and set it to "whatever was qemu's default", but decided this was too dangerous - we really need to spell out every last detail in the XML and not rely on defaults, otherwise we run into problems when we want to change the defaults.
This is patch 4/13 stuff. I think it's fine as a followup to adjust that logic if necessary - hopefully something that can be done in the 1.2.19 timeframe though. This would change the logic in qemuBuildControllerDevStr in the def->model switch for VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE to perhaps set a local chassisNr value based on opts.pciopts.chassisNr == -1, then use index, else use what was set.
That would be nullifying the entire purpose of storing chassisNr.
Beyond that and the duplicate 'chassis' parse in patch 6, things were find and ready to push with a few adjustments.
Derp. Yeah, that must have snuck in during one of the dozens of rebases. I removed the one that incorrectly checked chassisNr, of course.
On 08/09/2015 01:39 PM, Laine Stump wrote:
On 08/03/2015 08:33 AM, John Ferlan wrote:
Like Martin noted, I've been looking through this series as well. I too haven't had a ton of time to devote to it over the last week or so since I'm in the middle of a move. Also being so close to freeze - it just didn't feel right to shimmy it in there late. This should go in as soon as possible for 1.2.19 to hopefully weed out any weird issues! Some more specific/general comments... The 1.2.18 -> 1.2.19 everywhere will need to be done. The commit message in 2/13 seems to have incurred a disjoint thought or perhaps a forgotten edit in the paragraph that starts "The defaults are set..." - the last sentance just doesn't read well. I had some concern over confusion with 'chassis' and 'chassisNr'...
I've had problems in the past when I made the assumption that two apparently mutually exclusive things could share the same space. One day you get to a place where that isn't true, then you're screwed. Kind of like when you're time traveling and see a former/future version of yourself, and then time-space asplodes and there's no big sloppy mess to clean up because the universe has collapsed into a black hole or something... Kind of.
I also had concern over whether the parser could confuse chassis values depending on order seen - as in was a parse of 'chassis' an exact match or a prefix match...
I don't get this one - the comparison is on the entire string, not on a prefix of the string, so there can be no confusion.
It was my own personal oh sh*! moment. Since chassisNr was first and chassis second, I had some time/space memory thing that popped up and caused me to go look and make sure the comparison algorithm was right. Something from a former job where we had one particularly odd bug where the comparison was not on the length of the constant string we were comparing rather it was on the one read...
I think it's ok especially since one is used for pci-bridge and the other for pcie-root-port...
I'm still not sure if it's "best" to set chassisNr or make it some sort of tristate where if we know it wasn't read in from the XML, then we default or use the 'cont->idx' internally when formulating the qemu command.
The whole point of auto-setting it in the xml is to assure that it will never change "by accident" in the future. If we imply some value when it isn't explicitly set in the xml, then at some later date decide to change how we derive the implicit value, then we will have broken ABI of the guest (albeit in a probably inconsequential way, since my understanding is that essentially nobody uses these values :-/), and we don't want to do that.
I'm OK with the current implementation - just waiting for QA to claim they can cause some sort of failure though ;-)
That way at least we don't run into the situation where if "index" changes, then we don't run into some issue since we don't follow on with the chassisNr change (because we cannot be sure who set it).
Well, there are two conflicting situations we want to avoid: 1) having the chassisNr changed in the guest because libvirt internally changed the way it implicitly arrived at the value to use for chassisNr, and 2) having the chassisNr be in conflict with the chassisNr of some other pci controller because the user modified their config by both changing the index of this controller *and* adding a new controller of the same type/model in the old index.
The possibility of either is, quite frankly, rather remote. But in case (1) this "something bad" would happen with no conscious action on the part of the user (other than upgrading libvirt), so if it caused problems it would confuse/confound them. In case (2), at least it would be happening after they messed with the config (in a manner that almost nobody will ever do), so they might at least have some kind of clue what caused the problem.
Note that what we're doing here follows exactly the same behavior that we follow with network device model types - if no model type is given when the interface is added to the domain, rlt8139 is automatically added. We used to just leave it empty and set it to "whatever was qemu's default", but decided this was too dangerous - we really need to spell out every last detail in the XML and not rely on defaults, otherwise we run into problems when we want to change the defaults.
This is patch 4/13 stuff. I think it's fine as a followup to adjust that logic if necessary - hopefully something that can be done in the 1.2.19 timeframe though. This would change the logic in qemuBuildControllerDevStr in the def->model switch for VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE to perhaps set a local chassisNr value based on opts.pciopts.chassisNr == -1, then use index, else use what was set.
That would be nullifying the entire purpose of storing chassisNr.
OK - I see... That's just me following through on a thought - current impl is fine. I agree people should be messing with these values and IIRC you've documented that (more or less).
Beyond that and the duplicate 'chassis' parse in patch 6, things were find and ready to push with a few adjustments.
Derp. Yeah, that must have snuck in during one of the dozens of rebases. I removed the one that incorrectly checked chassisNr, of course.
I know you've pushed already, but just wanted to be sure to follow-up on my thoughts. John
participants (3)
-
John Ferlan -
Laine Stump -
Martin Kletzander