On 07/22/2015 04:20 PM, John Ferlan wrote:
>
> 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.
As far as I understand from Alex, it is 0-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 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.
"-1" is used to indicate "not specified" (of course when it isn't
specified, it is auto-filled later on).
>
>
>> + </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?
Yes. I'll put that in.
>
>
>> + 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]
Currently true. But as more things are added to <target> (in another
couple patches) that will no longer be true, so I opted to put in from
the beginning the code that would later be needed, rather than changing
it later.
>
>> + virBufferAsprintf(buf, " chassisNr='%d'",
>> + def->opts.pciopts.chassisNr);
>> + virBufferAddLit(buf, "/>\n");
> and of course means the whole thing can be formatted in one line
Not after patch
>
>> + }
>> +
>> 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.
I see this code as just making an adjustment to the existing
controllers, so it makes sense to me to adjust the test (while assuring
there are other tests around to make sure that it continues to work with
the new attributes not set).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list