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/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

> +    </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



[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]