On 07/17/2015 02:43 PM, Laine Stump wrote:
> This new subelement is used in PCI controllers: the toplevel
> *attribute* "model" of a controller denotes what kind of PCI
> controller is being described, e.g. a "dmi-to-pci-bridge",
> "pci-bridge", or "pci-root". But in the future there will be different
> implementations of some of those types of PCI controllers, which
> behave similarly from libvirt's point of view (and so should have the
> same model), but use a different device in qemu (and present
> themselves as a different piece of hardware in the guest). In an ideal
> world we (i.e. "I") would have thought of that back when the pci
> controllers were added, and used some sort of type/class/model
> notation (where class was used in the way we are now using model, and
> model was used for the actual manufacturer's model number of a
> particular family of PCI controller), but that opportunity is long
> past, so as an alternative, this patch allows selecting a particular
> implementation of a pci controller with the "type" attribute of the
> <model> subelement, e.g.:
>
> <controller type='pci' model='dmi-to-pci-bridge' index='1'>
> <model type='i82801b11-bridge'/>
> </controller>
>
> In this case, "dmi-to-pci-bridge" is the kind of controller (one that
> has a single PCIe port upstream, and 32 standard PCI ports downstream,
> which are not hotpluggable), and the qemu device to be used to
> implement this kind of controller is named "i82801b11-bridge".
>
> Implementing the above now will allow us in the future to add a new
> kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge
> device, but instead uses something else (which doesn't yet exist, but
> qemu people have been discussing it), all without breaking existing
> configs.
>
> (note that for the existing "pci-bridge" type of PCI controller, both
> the model attribute and <model> type are 'pci-bridge'. This is just a
> coincidence, since it turns out that in this case the device name in
> qemu really is a generic 'pci-bridge' rather than being the name of
> some real-world chip)
> ---
> new in V2 (previously was a part of the patch to add pcie-root-port)
>
> docs/formatdomain.html.in | 12 ++++++++++++
> docs/schemas/domaincommon.rng | 13 +++++++++++++
> src/conf/domain_conf.c | 23 +++++++++++++++++++++--
> src/conf/domain_conf.h | 8 ++++++++
> tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++--
> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++--
> 6 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8cd8d09..fa46276 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3037,6 +3037,18 @@
> (set to 0). <span class="since">Since 1.1.2 (QEMU only)</span>
> </p>
> <p>
> + PCI controllers also have an optional
> + subelement <code><model></code> with an attribute named
> + "type". The type attribute holds the name of the specific device
The <code>type</code> attribute...
> + that qemu is emulating (e.g. "i82801b11-bridge") rather than
> + simply the class of device ("dmi-to-pci-bridge", "pci-bridge"),
> + which is set in the controller element's model <b>attribute</b>.
> + In almost all cases, you should not manually add
> + a <code><model></code> subelement to a controller, nor
> + should you modify one that is automatically generated by
> + libvirt. <span class="since">Since 1.3.0 (QEMU only).</span>
1.2.18 (at least for now)
NB: As I read the code, only the *first* <model type='%s'> listed will
be used, as virDomainControllerDefParseXML not parse a second entry nor
does a second entry cause an error
> + </p>
> + <p>
> For machine types which provide an implicit PCI bus, the pci-root
> controller with index=0 is auto-added and required to use PCI devices.
> pci-root has no address.
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 1120003..66518f9 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1731,6 +1731,19 @@
> <attribute name="type">
> <value>pci</value>
> </attribute>
> + <optional>
> + <element name="model">
> + <attribute name="type">
> + <choice>
> + <!-- implementations of 'pci-bridge' -->
> + <value>pci-bridge</value>
> + <!-- implementations of 'dmi-to-pci-bridge' -->
> + <value>i82801b11-bridge</value>
> + </choice>
> + </attribute>
> + <empty/>
> + </element>
> + </optional>
> <!-- *-root controllers have an optional element "pcihole64"-->
> <choice>
> <group>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8dd4bf0..380b758 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7637,6 +7637,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> char *queues = NULL;
> char *cmd_per_lun = NULL;
> char *max_sectors = NULL;
> + char *guestModel = NULL;
> xmlNodePtr saved = ctxt->node;
> int rc;
>
> @@ -7682,6 +7683,9 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> queues = virXMLPropString(cur, "queues");
> cmd_per_lun = virXMLPropString(cur, "cmd_per_lun");
> max_sectors = virXMLPropString(cur, "max_sectors");
> + } else if (xmlStrEqual(cur->name, BAD_CAST "model")) {
> + if (!guestModel)
> + guestModel = virXMLPropString(cur, "type");
So subsequent "<model type='%s'>" are gleefully ignored? Should there be
an error? IDC either way, as long as it's described/noted because you
know there's someone from QA looking to add two <model...> entries and
expecting the second one to be used or an error to be generated.
Of course scrolling back to the RNG - syntactically there can only be
one it seems.
> }
> }
> cur = cur->next;
> @@ -7790,6 +7794,11 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> def->opts.pciopts.pcihole64size = VIR_DIV_UP(bytes, 1024);
> }
> }
> + if (guestModel) {
> + def->opts.pciopts.type = guestModel;
> + guestModel = 0;
s/0/NULL/
> + }
> + break;
>
> default:
> break;
> @@ -7814,6 +7823,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> VIR_FREE(queues);
> VIR_FREE(cmd_per_lun);
> VIR_FREE(max_sectors);
> + VIR_FREE(guestModel);
>
> return def;
>
> @@ -18823,7 +18833,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
> {
> const char *type = virDomainControllerTypeToString(def->type);
> const char *model = NULL;
> - bool pcihole64 = false;
> + bool pcihole64 = false, pciModel = false;
>
> if (!type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -18863,17 +18873,26 @@ virDomainControllerDefFormat(virBufferPtr buf,
> case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> if (def->opts.pciopts.pcihole64)
> pcihole64 = true;
> + if (def->opts.pciopts.type)
> + pciModel = true;
> break;
>
> default:
> break;
> }
>
> - if (def->queues || def->cmd_per_lun || def->max_sectors ||
> + if (pciModel ||
> + def->queues || def->cmd_per_lun || def->max_sectors ||
> virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) {
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
>
> + if (pciModel) {
> + virBufferAddLit(buf, "<model");
> + virBufferEscapeString(buf, " type='%s'", def->opts.pciopts.type);
> + virBufferAddLit(buf, "/>\n");
> + }
> +
> if (def->queues || def->cmd_per_lun || def->max_sectors) {
> virBufferAddLit(buf, "<driver");
> if (def->queues)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 50750c1..09fe3c0 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr;
> struct _virDomainPCIControllerOpts {
> bool pcihole64;
> unsigned long pcihole64size;
> +
> + /* the type is in the "model" subelement, e.g.:
> + * <controller type='pci' model='pcie-root-port'>
> + * <model type='ioh3420''/>
> + * ...
> + * similar to the model of <interface> devices.
> + */
> + char *type; /* the exact name of the device in hypervisor */
> };
>
> /* Stores the virtual disk controller configuration */
Since examples still exist that do not have the <model type='%s'> I
suppose it's OK to hijack an existing test, but having a "new" test
probably would have been better
ACK - with the adjustments - whether you update/add a new test is your call.
John
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> index 05967a4..4623a5c 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 type='i82801b11-bridge'/>
> + </controller>
> + <controller type='pci' index='2' model='pci-bridge'>
> + <model type='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..760830a 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 type='i82801b11-bridge'/>
> + </controller>
> + <controller type='pci' index='2' model='pci-bridge'>
> + <model type='pci-bridge'/>
> + </controller>
> <controller type='sata' index='0'/>
> <video>
> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list