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