> 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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev