>From: Ilya Maximets [mailto:i.maxim...@samsung.com] >Sent: Thursday, December 7, 2017 9:49 AM >To: Kavanagh, Mark B <mark.b.kavan...@intel.com>; d...@openvswitch.org >Cc: ktray...@redhat.com; maxime.coque...@redhat.com; >jan.scheur...@ericsson.com; Mooney, Sean K <sean.k.moo...@intel.com>; Stokes, >Ian <ian.sto...@intel.com> >Subject: Re: [ovs-dev][PATCH V4 2/2] netdev-dpdk: vHost IOMMU support > >Looks good to me in general, but I prefer if you'll fix following >checkpatch warnings:
Thanks for your comments Ilya - I'll address same in the next version of this patchset, depending on the outcome of this: https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341701.html Best regards, Mark > >== Checking 2e9587f27add ("netdev-dpdk: vHost IOMMU support") == >WARNING: Line length is >79-characters long >#52 FILE: Documentation/topics/dpdk/vhost-user.rst:280: >can access, and as such is useful in deployments in which security is a >concern. > >WARNING: Line length is >79-characters long >#54 FILE: Documentation/topics/dpdk/vhost-user.rst:282: >IOMMU support may be enabled via a global config value, ```vhost-iommu- >support```. > >WARNING: Line length is >79-characters long >#71 FILE: Documentation/topics/dpdk/vhost-user.rst:299: > default (and should remain so if using the aforementioned versions of >QEMU). > >One more comment inline. > >Best regards, Ilya Maximets. > >On 04.12.2017 14:15, Mark Kavanagh wrote: >> DPDK v17.11 introduces support for the vHost IOMMU feature. >> This is a security feature, which 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 global config option, vhost-iommu-support, >> that controls enablement of the vhost IOMMU feature: >> >> ovs-vsctl set Open_vSwitch . other_config:vhost-iommu-support=true >> >> This value defaults to false; to enable IOMMU support, this field >> should be set to true when setting other global parameters on init >> (such as "dpdk-socket-mem", for example). Changing the value at >> runtime is not supported, and requires restarting the vswitch daemon. >> >> Signed-off-by: Mark Kavanagh <mark.b.kavan...@intel.com> >> >> --- >> >> v4: - rebase to HEAD of master >> - clarify that IOMMU support applies exclusively to vhost user >> client ports. >> - reword caveats regarding modifying vhost-iommu-support at runtime >> - use smap_get_bool() to parse vhost-iommu-support, instead of >> process_vhost_flags(). >> - add stub implementation of dpdk_vhost_iommu_enabled(). >> - move stdbool.h #include outside of DPDK compiler guards. >> - remove mention of vhost IOMMU mode from >> netdev_dpdk_vhost_client_configure() log. >> >> v3: - erroneous; disregard. >> >> v2: - rebase to HEAD of master >> - refactor vHost IOMMU enablement mechanism (use a global >> config option, instead of the previous per-port approach). >> --- >> Documentation/topics/dpdk/vhost-user.rst | 27 +++++++++++++++++++++++++++ >> NEWS | 1 + >> lib/dpdk-stub.c | 6 ++++++ >> lib/dpdk.c | 12 ++++++++++++ >> lib/dpdk.h | 3 +++ >> lib/netdev-dpdk.c | 14 ++++++++++---- >> vswitchd/vswitch.xml | 15 +++++++++++++++ >> 7 files changed, 74 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst >b/Documentation/topics/dpdk/vhost-user.rst >> index 5347995..ffef653 100644 >> --- a/Documentation/topics/dpdk/vhost-user.rst >> +++ b/Documentation/topics/dpdk/vhost-user.rst >> @@ -273,6 +273,33 @@ One benefit of using this mode is the ability for vHost >ports to 'reconnect' in >> event of the switch crashing or being brought down. Once it is brought back >up, >> the vHost ports will reconnect automatically and normal service will >resume. >> >> +vhost-user-client IOMMU Support >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + >> +vhost IOMMU 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 support may be enabled via a global config value, ```vhost-iommu- >support```. >> +Setting this to true enables vhost IOMMU support for all vhost ports >when/where >> +available:: >> + >> + $ ovs-vsctl set Open_vSwitch.other_config:vhost-iommu-support=true > >Few spaces missing in above command. > >> + >> +The default value is false. >> + >> +.. important:: >> + >> + Changing this value requires restarting the daemon. >> + >> +.. 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. >> + >> .. _dpdk-testpmd: >> >> DPDK in the Guest >> diff --git a/NEWS b/NEWS >> index d4a1c9a..99c47d8 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -15,6 +15,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 >> >> v2.8.0 - 31 Aug 2017 >> -------------------- >> diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c >> index daef729..3602180 100644 >> --- a/lib/dpdk-stub.c >> +++ b/lib/dpdk-stub.c >> @@ -48,3 +48,9 @@ dpdk_get_vhost_sock_dir(void) >> { >> return NULL; >> } >> + >> +bool >> +dpdk_vhost_iommu_enabled(void) >> +{ >> + return false; >> +} >> diff --git a/lib/dpdk.c b/lib/dpdk.c >> index 8da6c32..6710d10 100644 >> --- a/lib/dpdk.c >> +++ b/lib/dpdk.c >> @@ -41,6 +41,7 @@ VLOG_DEFINE_THIS_MODULE(dpdk); >> static FILE *log_stream = NULL; /* Stream for DPDK log redirection */ >> >> static char *vhost_sock_dir = NULL; /* Location of vhost-user sockets */ >> +static bool vhost_iommu_enabled = false; /* Status of vHost IOMMU support >*/ >> >> static int >> process_vhost_flags(char *flag, const char *default_val, int size, >> @@ -345,6 +346,11 @@ dpdk_init__(const struct smap *ovs_other_config) >> vhost_sock_dir = sock_dir_subcomponent; >> } >> >> + vhost_iommu_enabled = smap_get_bool(ovs_other_config, >> + "vhost-iommu-support", false); >> + VLOG_INFO("IOMMU support for vhost-user-client %s.", >> + vhost_iommu_enabled ? "enabled" : "disabled"); >> + >> argv = grow_argv(&argv, 0, 1); >> argc = 1; >> argv[0] = xstrdup(ovs_get_program_name()); >> @@ -482,6 +488,12 @@ dpdk_get_vhost_sock_dir(void) >> return vhost_sock_dir; >> } >> >> +bool >> +dpdk_vhost_iommu_enabled(void) >> +{ >> + return vhost_iommu_enabled; >> +} >> + >> void >> dpdk_set_lcore_id(unsigned cpu) >> { >> diff --git a/lib/dpdk.h b/lib/dpdk.h >> index 673a1f1..dc58d96 100644 >> --- a/lib/dpdk.h >> +++ b/lib/dpdk.h >> @@ -17,6 +17,8 @@ >> #ifndef DPDK_H >> #define DPDK_H >> >> +#include <stdbool.h> >> + >> #ifdef DPDK_NETDEV >> >> #include <rte_config.h> >> @@ -35,5 +37,6 @@ struct smap; >> void dpdk_init(const struct smap *ovs_other_config); >> void dpdk_set_lcore_id(unsigned cpu); >> const char *dpdk_get_vhost_sock_dir(void); >> +bool dpdk_vhost_iommu_enabled(void); >> >> #endif /* dpdk.h */ >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index f552444..9715c39 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -3253,6 +3253,7 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev >*netdev) >> { >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> int err; >> + uint64_t vhost_flags = 0; >> >> ovs_mutex_lock(&dev->mutex); >> >> @@ -3263,16 +3264,21 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev >*netdev) >> */ >> if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT) >> && strlen(dev->vhost_id)) { >> - /* Register client-mode device */ >> - err = rte_vhost_driver_register(dev->vhost_id, >> - RTE_VHOST_USER_CLIENT); >> + /* Register client-mode device. */ >> + vhost_flags |= RTE_VHOST_USER_CLIENT; >> + >> + /* Enable IOMMU support, if explicitly requested. */ >> + if (dpdk_vhost_iommu_enabled()) { >> + vhost_flags |= RTE_VHOST_USER_IOMMU_SUPPORT; >> + } >> + err = rte_vhost_driver_register(dev->vhost_id, vhost_flags); >> if (err) { >> VLOG_ERR("vhost-user device setup failure for device %s\n", >> dev->vhost_id); >> goto unlock; >> } else { >> /* Configuration successful */ >> - dev->vhost_driver_flags |= RTE_VHOST_USER_CLIENT; >> + dev->vhost_driver_flags |= vhost_flags; >> VLOG_INFO("vHost User device '%s' created in 'client' mode, " >> "using client socket '%s'", >> dev->up.name, dev->vhost_id); >> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >> index c145e1a..6c6df50 100644 >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -344,6 +344,21 @@ >> </p> >> </column> >> >> + <column name="other_config" key="vhost-iommu-support" >> + type='{"type": "boolean"}'> >> + <p> >> + vHost IOMMU is a security feature, which restricts the vhost >memory >> + that a virtio device may access. vHost IOMMU support is disabled >by >> + default, due to a bug in QEMU implementations of the vhost >REPLY_ACK >> + protocol, (on which vHost IOMMU relies) prior to v2.9.1. Setting >this >> + value to <code>true</code> enables vHost IOMMU support for vHost >User >> + Client ports in OvS-DPDK, starting from DPDK v17.11. >> + </p> >> + <p> >> + Changing this value requires restarting the daemon. >> + </p> >> + </column> >> + >> <column name="other_config" key="n-handler-threads" >> type='{"type": "integer", "minInteger": 1}'> >> <p> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev