Re: [PATCHv2 05/17] conf: add new <model> subelement with type attribute to <controller>

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>&lt;model&gt;</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>&lt;model&gt;</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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]