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

Re: [libvirt] [PATCHv2 06/15] Add virtio-related options to interfaces



On Tue, Jun 06, 2017 at 01:36:20PM +0200, Ján Tomko wrote:
> <interface type='user'>
>   <mac address='52:54:56:5a:5c:5e'/>
>   <model type='virtio'/>
>   <driver iommu_platform='on' ats='on'/>
> </interface>
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1283251
> ---
>  docs/formatdomain.html.in                          | 19 +++++++
>  docs/schemas/domaincommon.rng                      | 12 +++++
>  src/conf/domain_conf.c                             | 63 ++++++++++++++++++++++
>  src/conf/domain_conf.h                             | 19 +++++++
>  .../qemuxml2argv-virtio-options.xml                |  2 +
>  .../qemuxml2xmlout-virtio-options.xml              |  2 +
>  6 files changed, 117 insertions(+)

I would spit this patch into two patches, one that introduces the virtio
related options and second one that introduces this element to the
interfaces.

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 2f1e030..dcc2e5e 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3451,6 +3451,19 @@
>        </dd>
>      </dl>
>  
> +    <h4><a name="elementsVirtio">Virtio-related options</a></h4>
> +
> +    <p>
> +      QEMU's virtio devices have some attributes related to the virtio transport under
> +      the <code>driver</code> element:
> +      The <code>iommu_platform</code> attribute enables the use of emulated IOMMU

I think that we tend to use camelCase for attributes and elements.
Isn't "iommuEnabled" or just "iommu", we don't need to follow QEMU
naming, it should be something generic.

> +      by the device. The attribute <code>ats</code> controls the Address
> +      Translation Service support for PCIe devices. This is needed to make use
> +      of IOTLB support (see <a href="#elementsIommu">IOMMU device</a>).
> +      Possible values are <code>on</code> or <code>off</code>.
> +      <span class="since">Since 3.5.0</span>
> +    </p>
> +
>      <h4><a name="elementsControllers">Controllers</a></h4>
>  
>      <p>
> @@ -5142,6 +5155,12 @@ qemu-kvm -net nic,model=? /dev/null
>          <b>In general you should leave this option alone, unless you
>          are very certain you know what you are doing.</b>
>        </dd>
> +      <dt>virtio options</dt>
> +      <dd>
> +        For virtio interfaces,
> +        <a href="#elementsVirtio">Virtio-specific options</a> can also be
> +        set. (<span class="since">Since 3.5.0</span>)
> +      </dd>
>      </dl>
>      <p>
>        Offloading options for the host and guest can be configured using
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6c3e885..d7f3b02 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2686,6 +2686,7 @@
>                </optional>
>              </group>
>            </choice>
> +          <ref name="virtioOptions"/>
>            <interleave>
>              <optional>
>                <element name='host'>
> @@ -5006,6 +5007,17 @@
>      </element>
>    </define>
>  
> +  <define name="virtioOptions">
> +    <optional>
> +      <attribute name="iommu_platform">
> +        <ref name="virOnOff"/>
> +      </attribute>
> +      <attribute name="ats">
> +        <ref name="virOnOff"/>
> +      </attribute>
> +    </optional>
> +  </define>
> +
>    <define name="usbmaster">
>      <element name="master">
>        <attribute name="startport">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 89c8917..ef3383d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1105,6 +1105,46 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt)
>      return &xmlopt->ns;
>  }
>  
> +static int
> +virDomainVirtioOptionsParseXML(xmlXPathContextPtr ctxt,
> +                               virDomainVirtioOptionsPtr *virtio)
> +{
> +    char *str = NULL;
> +    int ret = -1;
> +    int val;
> +    virDomainVirtioOptionsPtr res;
> +
> +    if (VIR_ALLOC(*virtio) < 0)
> +        return -1;
> +
> +    res = *virtio;
> +
> +    if ((str = virXPathString("string(./driver/@iommu_platform)", ctxt))) {
> +        if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("invalid iommu_platform value"));
> +            goto cleanup;
> +        }
> +        res->iommu_platform = val;
> +    }
> +    VIR_FREE(str);
> +
> +    if ((str = virXPathString("string(./driver/@ats)", ctxt))) {
> +        if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("invalid ats value"));
> +            goto cleanup;
> +        }
> +        res->ats = val;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(str);
> +    return ret;
> +}
> +
>  
>  void
>  virBlkioDeviceArrayClear(virBlkioDevicePtr devices,
> @@ -1952,6 +1992,7 @@ virDomainNetDefClear(virDomainNetDefPtr def)
>      VIR_FREE(def->ifname);
>      VIR_FREE(def->ifname_guest);
>      VIR_FREE(def->ifname_guest_actual);
> +    VIR_FREE(def->virtio);
>  
>      virNetDevIPInfoClear(&def->guestIP);
>      virNetDevIPInfoClear(&def->hostIP);
> @@ -5221,6 +5262,24 @@ virDomainDefValidate(virDomainDefPtr def,
>  }
>  
>  
> +static void
> +virDomainVirtioOptionsFormat(virBufferPtr buf,
> +                             virDomainVirtioOptionsPtr virtio)
> +{
> +    if (!virtio)
> +        return;
> +
> +    if (virtio->iommu_platform != VIR_TRISTATE_SWITCH_ABSENT) {
> +        virBufferAsprintf(buf, "iommu_platform='%s' ",
> +                          virTristateSwitchTypeToString(virtio->iommu_platform));
> +    }
> +    if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
> +        virBufferAsprintf(buf, "ats='%s' ",
> +                          virTristateSwitchTypeToString(virtio->ats));
> +    }
> +}
> +
> +
>  /* Generate a string representation of a device address
>   * @info address Device address to stringify
>   */
> @@ -10367,6 +10426,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>              goto error;
>      }
>  
> +    if (virDomainVirtioOptionsParseXML(ctxt, &def->virtio) < 0)
> +        goto error;
> +
>   cleanup:
>      ctxt->node = oldnode;
>      VIR_FREE(macaddr);
> @@ -22104,6 +22166,7 @@ virDomainVirtioNetDriverFormat(char **outstr,
>          virBufferAsprintf(&buf, "rx_queue_size='%u' ",
>                            def->driver.virtio.rx_queue_size);
>  
> +    virDomainVirtioOptionsFormat(&buf, def->virtio);
>      virBufferTrim(&buf, " ", -1);
>  
>      if (virBufferCheckError(&buf) < 0)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 7d1f05c..a17f217 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -155,6 +155,17 @@ typedef virDomainTPMDef *virDomainTPMDefPtr;
>  typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
>  typedef virDomainIOMMUDef *virDomainIOMMUDefPtr;
>  
> +typedef struct _virDomainVirtioOptions virDomainVirtioOptions;
> +typedef virDomainVirtioOptions *virDomainVirtioOptionsPtr;
> +
> +typedef enum {
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_DEFAULT,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_LEGACY,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_TRANSITIONAL,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_MODERN,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST,
> +} virDomainDriverCompatibility;

This isn't used anywhere in the code and in the remaining patches,
isn't it just some leftover?

Pavel

Attachment: signature.asc
Description: Digital signature


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