[ovs-dev] [PATCH 2/2] netdev-dpdk: add support for vhost IOMMU feature
Stokes, Ian
ian.stokes at intel.com
Fri Nov 24 09:01:38 UTC 2017
> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> bounces at openvswitch.org] On Behalf Of Stokes, Ian
> Sent: Friday, November 24, 2017 8:19 AM
> To: Ilya Maximets <i.maximets at samsung.com>; ovs-dev at openvswitch.org;
> Kavanagh, Mark B <mark.b.kavanagh at intel.com>
> Cc: Maxime Coquelin <maxime.coquelin at redhat.com>
> Subject: Re: [ovs-dev] [PATCH 2/2] netdev-dpdk: add support for vhost
> IOMMU feature
>
> > Hello, Mark.
> >
> > Thanks for the patch. Few comments:
> >
>
> Thanks for the feedback Ilya, great minds :). The feedback is quite
> timely, from my point of view I'd like to see these issues resolved as
> quickly as possible. I don't think there's anything 'show stopper' wise in
> what you've raise, we should be able to correct these adjustments.
>
>
> @Mark, what do you think?
>
> Ian
>
>
> > 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
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list