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

Re: [libvirt] [PATCHv3 05/14] Add virtio-related options to interfaces



On Wed, Jun 07, 2017 at 09:24:55PM +0200, Ján Tomko wrote:
> <interface type='user'>
>   <mac address='52:54:56:5a:5c:5e'/>
>   <model type='virtio'/>
>   <driver iommu='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                             | 121 +++++++++++++++++++++
>  src/conf/domain_conf.h                             |  10 ++
>  .../qemuxml2argv-virtio-options.xml                |   1 +
>  5 files changed, 163 insertions(+)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fa0edd1..e9ff73b 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</code> attribute enables the use of emulated IOMMU
> +      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 5c542c7..15fd72f 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">
> +        <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 710048c..22b8653 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1112,6 +1112,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)", ctxt))) {
> +        if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("invalid iommu value"));
> +            goto cleanup;
> +        }
> +        res->iommu = 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;
> +}
> +
>  
>  virSaveCookieCallbacksPtr
>  virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt)
> @@ -1966,6 +2006,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);
> @@ -4340,6 +4381,28 @@ virDomainHostdevDefPostParse(virDomainHostdevDefPtr dev,
>  
>  
>  static int
> +virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
> +{
> +    if (!virtio)
> +        return 0;
> +
> +    if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("iommu driver option is only supported "
> +                         "for virtio devices"));
> +        return -1;
> +    }
> +    if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("ats driver option is only supported "
> +                         "for virtio devices"));
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +
> +static int
>  virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>                                      const virDomainDef *def,
>                                      virCapsPtr caps ATTRIBUTE_UNUSED,
> @@ -4437,6 +4500,13 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>          }
>      }
>  
> +    if (dev->type == VIR_DOMAIN_DEVICE_NET) {
> +        virDomainNetDefPtr net = dev->data.net;
> +        if (STRNEQ_NULLABLE(net->model, "virtio") &&
> +            virDomainCheckVirtioOptions(net->virtio) < 0)
> +            return -1;
> +    }
> +
>      return 0;
>  }
>  
> @@ -5235,6 +5305,24 @@ virDomainDefValidate(virDomainDefPtr def,
>  }
>  
>  
> +static void
> +virDomainVirtioOptionsFormat(virBufferPtr buf,
> +                             virDomainVirtioOptionsPtr virtio)
> +{
> +    if (!virtio)
> +        return;
> +
> +    if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
> +        virBufferAsprintf(buf, " iommu='%s'",
> +                          virTristateSwitchTypeToString(virtio->iommu));
> +    }
> +    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
>   */
> @@ -10381,6 +10469,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>              goto error;
>      }
>  
> +    if (virDomainVirtioOptionsParseXML(ctxt, &def->virtio) < 0)
> +        goto error;
> +
>   cleanup:
>      ctxt->node = oldnode;
>      VIR_FREE(macaddr);
> @@ -19005,6 +19096,30 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src,
>  
>  
>  static bool
> +virDomainVirtioOptionsCheckABIStability(virDomainVirtioOptionsPtr src,
> +                                        virDomainVirtioOptionsPtr dst)
> +{
> +    if (src->iommu != dst->iommu) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Target device iommu option '%s' does not "
> +                         "match source '%s'"),
> +                       virTristateSwitchTypeToString(dst->iommu),
> +                       virTristateSwitchTypeToString(src->iommu));
> +        return false;
> +    }
> +    if (src->ats != dst->ats) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Target device ats option '%s' does not "
> +                         "match source '%s'"),
> +                       virTristateSwitchTypeToString(dst->ats),
> +                       virTristateSwitchTypeToString(src->ats));
> +        return false;
> +    }
> +    return true;
> +}
> +
> +
> +static bool
>  virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src,
>                                    virDomainDiskDefPtr dst)
>  {
> @@ -19163,6 +19278,10 @@ virDomainNetDefCheckABIStability(virDomainNetDefPtr src,
>          return false;
>      }
>  
> +    if (src->virtio && dst->virtio &&
> +        !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
> +        return false;

Shouldn't we also check !!src->virtion == !!dst->virtio?  The parser
for virtio options always allocates @virtio so technically it shouldn't
happen but throughout the code we check whether @virtio is allocated.

I've missed the ABI stability check in the previous review, good catch.

Pavel

> +
>      if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
>          return false;
>  
> @@ -22118,6 +22237,8 @@ virDomainVirtioNetDriverFormat(char **outstr,
>          virBufferAsprintf(&buf, " rx_queue_size='%u'",
>                            def->driver.virtio.rx_queue_size);
>  
> +    virDomainVirtioOptionsFormat(&buf, def->virtio);
> +
>      if (virBufferCheckError(&buf) < 0)
>          return -1;
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 597fb22..b3e607b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -156,6 +156,9 @@ typedef virDomainTPMDef *virDomainTPMDefPtr;
>  typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
>  typedef virDomainIOMMUDef *virDomainIOMMUDefPtr;
>  
> +typedef struct _virDomainVirtioOptions virDomainVirtioOptions;
> +typedef virDomainVirtioOptions *virDomainVirtioOptionsPtr;
> +
>  /* Flags for the 'type' field in virDomainDeviceDef */
>  typedef enum {
>      VIR_DOMAIN_DEVICE_NONE = 0,
> @@ -1040,6 +1043,7 @@ struct _virDomainNetDef {
>      int linkstate;
>      unsigned int mtu;
>      virNetDevCoalescePtr coalesce;
> +    virDomainVirtioOptionsPtr virtio;
>  };
>  
>  typedef enum {
> @@ -2215,6 +2219,12 @@ struct _virDomainIOMMUDef {
>      virTristateSwitch eim;
>      virTristateSwitch iotlb;
>  };
> +
> +struct _virDomainVirtioOptions {
> +    virTristateSwitch iommu;
> +    virTristateSwitch ats;
> +};
> +
>  /*
>   * Guest VM main configuration
>   *
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-options.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-options.xml
> index c88cf64..3357bc6 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-options.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-options.xml
> @@ -47,6 +47,7 @@
>      <interface type='user'>
>        <mac address='52:54:56:58:5a:5c'/>
>        <model type='virtio'/>
> +      <driver iommu='on' ats='on'/>
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
>      </interface>
>      <input type='mouse' bus='virtio'>
> -- 
> 2.10.2
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


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