On 07/17/2015 02:43 PM, Laine Stump wrote:
> There are some configuration options to some types of pci
> controllers that are currently automatically derived from other parts
> of the controller's configuration. For example, a pci-bridge
> controller has an option that is called "chassis_nr" in qemu; libvirt
> always sets chassis_nr to the index of the pci-bridge. So this:
>
> <controller type='pci' model='pci-bridge' index='2'/>
>
> will always result in:
>
> -device pci-bridge,chassis_nr=2,...
>
> on the qemu commandline. In the future we may decide there is a better
> way to derive that option, but even in that case we will need for
> existing domains to retain the same chassis_nr they were using in the
> past - that is something that is visible to the guest so it is part of
> the guest ABI and changing it would lead to problems for migrating
> guests (or just guests with very picky OSes).
>
> The <target> subelement has been added as a place to put the new
> "chassisNr" attribute that will be filled in by libvirt when it
> auto-generates the chassisNr; it will be saved in the config, then
> reused any time the domain is started:
>
> <controller type='pci' model='pci-bridge' index='2'>
> <model type='pci-bridge'/>
> <target chassisNr='2'/>
> </controller>
>
> The one oddity of all this is that if the controller configuration
> is changed (for example to change the index or the pci address
> where the controller is plugged in), the items in <target> will
> *not* be re-generated, which might lead to conflict. I can't
> really see any way around this, but fortunately if there is a
> material conflict qemu will let us know and we will pass that on
> to the user.
> ---
>
> new in V2 (previously was a part of the patch to add pcie-root-port,
> but adding the attributes under <model> instead of <target>)
>
> docs/formatdomain.html.in | 23 ++++++++++++++++++++
> docs/schemas/domaincommon.rng | 10 +++++++++
> src/conf/domain_conf.c | 28 +++++++++++++++++++++++--
> src/conf/domain_conf.h | 10 ++++++++-
> tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 1 +
> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 1 +
> 6 files changed, 70 insertions(+), 3 deletions(-)
>
Again assuming this is the generally acceptable mechanism. It seems fine
to me though
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fa46276..ae0d66a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3049,6 +3049,29 @@
> libvirt. <span class="since">Since 1.3.0 (QEMU only).</span>
> </p>
> <p>
> + PCI controllers also have an optional
s/also//
> + subelement <code><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.3.0 (QEMU only).</span>
1.2.18
> + </p>
> + <dl>
> + <dt><code>chassisNr</code></dt>
> + <dd>
> + PCI controllers that have attribute model="pci-bridge", can
> + also have a <code>chassisNr</code> attribute in
> + the <code><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).
Is there a valid range that needs to be documented? See comments for
rng and parse.
> + </dd>
> + </dl>
> + <p>
> For machine types which provide an implicit PCI bus, the pci-root
> controller with index=0 is auto-added and required to use PCI devices.
> pci-root has no address.
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 66518f9..1b1f592 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1744,6 +1744,16 @@
> <empty/>
> </element>
> </optional>
> + <optional>
> + <element name="target">
> + <optional>
> + <attribute name='chassisNr'>
> + <ref name='uint8range'/>
This is an int in domain_conf.h and is set to -1 in
virDomainControllerDefNew.
So one or the other seems to need adjustment since by this rule we can
only be between 0 and 255 inclusive.
> + </attribute>
> + </optional>
> + <empty/>
> + </element>
> + </optional>
> <!-- *-root controllers have an optional element "pcihole64"-->
> <choice>
> <group>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 380b758..17526d4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1544,6 +1544,8 @@ virDomainControllerDefNew(virDomainControllerType type)
> def->opts.vioserial.vectors = -1;
> break;
> case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> + def->opts.pciopts.chassisNr = -1;
> + break;
> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
> case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> @@ -7638,6 +7640,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> char *cmd_per_lun = NULL;
> char *max_sectors = NULL;
> char *guestModel = NULL;
> + char *chassisNr = NULL;
> xmlNodePtr saved = ctxt->node;
> int rc;
>
> @@ -7686,6 +7689,9 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> } else if (xmlStrEqual(cur->name, BAD_CAST "model")) {
> if (!guestModel)
> guestModel = virXMLPropString(cur, "type");
> + } else if (xmlStrEqual(cur->name, BAD_CAST "target")) {
> + if (!chassisNr)
> + chassisNr = virXMLPropString(cur, "chassisNr");
Similar to note about guestModel - multiple chassisNr aren't allowed by
RNG rules...
> }
> }
> cur = cur->next;
> @@ -7798,6 +7804,13 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> def->opts.pciopts.type = guestModel;
> guestModel = 0;
> }
> + if (chassisNr && virStrToLong_i(chassisNr, NULL, 0,
> + &def->opts.pciopts.chassisNr) < 0) {
^
one extra space too many (just had a shot of coffee so I saw it!)
Also if the range is truly 0 to 255 inclusive, then don't we need to
test for that here as well?
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid chassisNr '%s' in PCI controller"),
> + chassisNr);
> + goto error;
> + }
> break;
>
> default:
> @@ -7824,6 +7837,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> VIR_FREE(cmd_per_lun);
> VIR_FREE(max_sectors);
> VIR_FREE(guestModel);
> + VIR_FREE(chassisNr);
>
> return def;
>
> @@ -18833,7 +18847,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
> {
> const char *type = virDomainControllerTypeToString(def->type);
> const char *model = NULL;
> - bool pcihole64 = false, pciModel = false;
> + bool pcihole64 = false, pciModel = false, pciTarget = false;
>
> if (!type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -18875,13 +18889,15 @@ virDomainControllerDefFormat(virBufferPtr buf,
> pcihole64 = true;
> if (def->opts.pciopts.type)
> pciModel = true;
> + if (def->opts.pciopts.chassisNr != -1)
> + pciTarget = true;
[this]
> break;
>
> default:
> break;
> }
>
> - if (pciModel ||
> + if (pciModel || pciTarget ||
> def->queues || def->cmd_per_lun || def->max_sectors ||
> virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) {
> virBufferAddLit(buf, ">\n");
> @@ -18893,6 +18909,14 @@ virDomainControllerDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, "/>\n");
> }
>
> + if (pciTarget) {
This can only be true if chassisNr != -1
> + virBufferAddLit(buf, "<target");
> + if (def->opts.pciopts.chassisNr != -1)
[which means this if isn't necessary]
> + virBufferAsprintf(buf, " chassisNr='%d'",
> + def->opts.pciopts.chassisNr);
> + virBufferAddLit(buf, "/>\n");
and of course means the whole thing can be formatted in one line
> + }
> +
> if (def->queues || def->cmd_per_lun || def->max_sectors) {
> virBufferAddLit(buf, "<driver");
> if (def->queues)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 09fe3c0..32d1073 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -805,7 +805,15 @@ struct _virDomainPCIControllerOpts {
> * similar to the model of <interface> devices.
> */
> char *type; /* the exact name of the device in hypervisor */
> -};
> +
> + /* the following items are attributes of the "target" subelement
> + * of controller type='pci'. They are bits of configuration that
> + * are specified on the qemu commandline and are visible to the
> + * guest OS, so they must be preserved to ensure ABI
> + * compatibility.
> + */
> + int chassisNr; /* used by pci-bridge, -1 == unspecified */
> + };
>
> /* Stores the virtual disk controller configuration */
> struct _virDomainControllerDef {
Similar to previous comment about "reusing" a test - IDC, but perhaps
this should have been a "new" test which we're building upon rather than
stealing an existing one.
ACK with the appropriate adjustments
John
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> index 4623a5c..815eb08 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 type='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 760830a..13e4734 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 type='pci-bridge'/>
> + <target chassisNr='56'/>
> </controller>
> <controller type='sata' index='0'/>
> <video>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list