[ovs-dev] [PATCH 2/2] netdev-dpdk: add support for vhost IOMMU feature

Ilya Maximets i.maximets at samsung.com
Fri Nov 24 08:06:11 UTC 2017


Hello, Mark.

Thanks for the patch. Few comments:

1. We can not change the RTE_VHOST_USER_IOMMU_SUPPORT flag if device already
   registered, i.e. if (dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT) != 0.
   IMHO, we should print a warning message in that case and should not update
   the device flags and request reconfiguration.

   This should be clearly described in commit message and documentation that
   vhost-iommu-support can't be changed if device already fully initialized.
   It should be clear that this option should be set before or simultaneously
   with vhost-server-path.

2. General style comment for the patch: According to coding style, you need to
   use braces for if statements even if there is only one statement in it.

3. Few more comments inline.

> DPDK v17.11 introduces support for the vHost IOMMU feature.
> This is a security feature, that restricts the vhost memory
> that a virtio device may access.
> 
> This feature also enables the vhost REPLY_ACK protocol, the
> implementation of which is known to work in newer versions of
> QEMU (i.e. v2.10.0), but is buggy in older versions (v2.7.0 -
> v2.9.0, inclusive). As such, the feature is disabled by default
> in (and should remain so, for the aforementioned older QEMU
> verions). Starting with QEMU v2.9.1, vhost-iommu-support can
> safely be enabled, even without having an IOMMU device, with
> no performance penalty.
> 
> This patch adds a new vhost port option, vhost-iommu-support,
> to allow enablement of the vhost IOMMU feature:
> 
>     $ ovs-vsctl add-port br0 vhost-client-1 \
>         -- set Interface vhost-client-1 type=dpdkvhostuserclient \
>              options:vhost-server-path=$VHOST_USER_SOCKET_PATH   \
>              options:vhost-iommu-support=true
> 
> Note that support for this feature is only implemented for vhost
> user client ports (since vhost user ports are considered deprecated).
> 
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> Acked-by: Maxime Coquelin <maxime.coquelin at redhat.com>
> Acked-by: Ciara Loftus <ciara.loftus at intel.com>
> ---
>  Documentation/topics/dpdk/vhost-user.rst | 21 +++++++++++++++++++++
>  NEWS                                     |  1 +
>  lib/netdev-dpdk.c                        | 29 ++++++++++++++++++++++++++---
>  vswitchd/vswitch.xml                     | 10 ++++++++++
>  4 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst
> index 5347995..8dff901 100644
> --- a/Documentation/topics/dpdk/vhost-user.rst
> +++ b/Documentation/topics/dpdk/vhost-user.rst
> @@ -250,6 +250,27 @@ Once the vhost-user-client ports have been added to the switch, they must be
>  added to the guest. Like vhost-user ports, there are two ways to do this: using
>  QEMU directly, or using libvirt. Only the QEMU case is covered here.
>  
> +vhost-user client IOMMU
> +~~~~~~~~~~~~~~~~~~~~~~~
> +It is possible to enable IOMMU support for vHost User client ports. This is
> +a feature which restricts the vhost memory that a virtio device can access, and
> +as such is useful in deployments in which security is a concern. IOMMU mode may
> +be enabled on the command line::
> +
> +    $ ovs-vsctl add-port br0 vhost-client-1 \
> +        -- set Interface vhost-client-1 type=dpdkvhostuserclient \
> +             options:vhost-server-path=$VHOST_USER_SOCKET_PATH   \
> +             options:vhost-iommu-support=true
> +
> +.. important::
> +
> +    Enabling the IOMMU feature also enables the vhost user reply-ack protocol;
> +    this is known to work on QEMU v2.10.0, but is buggy on older versions
> +    (2.7.0 - 2.9.0, inclusive). Consequently, the IOMMU feaure is disabled by
> +    default (and should remain so if using the aforementioned versions of QEMU).
> +    Starting with QEMU v2.9.1, vhost-iommu-support can safely be enabled, even
> +    without having an IOMMU device, with no performance penalty.
> +
>  Adding vhost-user-client ports to the guest (QEMU)
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/NEWS b/NEWS
> index 74e59bf..c15dc24 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,7 @@ Post-v2.8.0
>       * Add support for compiling OVS with the latest Linux 4.13 kernel
>     - DPDK:
>       * Add support for DPDK v17.11
> +     * Add support for vHost IOMMU feature
>  
>  v2.8.0 - 31 Aug 2017
>  --------------------
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index ed5bf62..2e9633a 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1424,15 +1424,29 @@ netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
>  {
>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>      const char *path;
> +    bool iommu_enable;
> +    bool request_reconfigure = false;
> +    uint64_t vhost_driver_flags_prev = dev->vhost_driver_flags;

This should be done under the mutex.

>  
>      ovs_mutex_lock(&dev->mutex);
>      if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
>          path = smap_get(args, "vhost-server-path");
>          if (path && strcmp(path, dev->vhost_id)) {
>              strcpy(dev->vhost_id, path);
> -            netdev_request_reconfigure(netdev);
> +            request_reconfigure = true;
>          }
>      }
> +
> +    iommu_enable = smap_get_bool(args, "vhost-iommu-support", false);
> +    if (iommu_enable)
> +        dev->vhost_driver_flags |= RTE_VHOST_USER_IOMMU_SUPPORT;
> +    else
> +        dev->vhost_driver_flags &= ~RTE_VHOST_USER_IOMMU_SUPPORT;
> +    if (vhost_driver_flags_prev != dev->vhost_driver_flags)
> +        request_reconfigure = true;
> +
> +    if (request_reconfigure)
> +        netdev_request_reconfigure(netdev);
>      ovs_mutex_unlock(&dev->mutex);
>  
>      return 0;
> @@ -3326,9 +3340,18 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev)
>       */
>      if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)
>              && strlen(dev->vhost_id)) {
> +        /* Enable vhost IOMMU, if it was requested.

Above comment connected to 'if', but not the 'flags'. I'm suggesting to
split the comment by moving above line between 'flags' declaration and the 'if'.

> +         * XXX: the 'flags' variable is required, as not all vhost backend
> +         * features are currently supported by OvS; in time, it should be
> +         * possible to invoke rte_vhost_driver_register(), passing
> +         * dev->vhost_driver_flags directly as a parameter to same.
> +         */
> +        uint64_t flags = RTE_VHOST_USER_CLIENT
Anyway, there should be blank line between variable declaration and the other code.

> +        if (dev->vhost_driver_flags & RTE_VHOST_USER_IOMMU_SUPPORT)
> +            flags |= RTE_VHOST_USER_IOMMU_SUPPORT;
> +
>          /* Register client-mode device */
> -        err = rte_vhost_driver_register(dev->vhost_id,
> -                                        RTE_VHOST_USER_CLIENT);
> +        err = rte_vhost_driver_register(dev->vhost_id, flags);
>          if (err) {
>              VLOG_ERR("vhost-user device setup failure for device %s\n",
>                       dev->vhost_id);
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index c145e1a..a633226 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2654,6 +2654,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>          </p>
>        </column>
>  
> +      <column name="options" key="vhost-iommu-support"
> +              type='{"type": "boolean"}'>
> +        <p>
> +          The value specifies whether IOMMU support is enabled for a vHost User
> +          client mode device that has been or will be created by QEMU.
> +          Only supported by dpdkvhostuserclient interfaces. If not specified or
> +          an incorrect value is specified, defaults to 'false'.
> +        </p>
> +      </column>
> +
>        <column name="options" key="n_rxq_desc"
>                type='{"type": "integer", "minInteger": 1, "maxInteger": 4096}'>
>          <p>
> -- 
> 1.9.3



More information about the dev mailing list