Thanks for the patches. Comments inline.
Best regards, Ilya Maximets.
On 30.11.2017 13:03, 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
>
> Note that changing this value after guest devices have already been
> initialized will not toggle IOMMU support. To that end, if IOMMU
> support is required, this field should be set to true when setting
> other global parameters on init (such as "dpdk-socket-mem", for
> example).
>
> Signed-off-by: Mark Kavanagh <mark.b.kavan...@intel.com>
>
> ---
>
> v2->v1:
> - 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 | 29 +++++++++++++++++++++++++++++
> NEWS | 1 +
> lib/dpdk.c | 16 ++++++++++++++++
> lib/dpdk.h | 3 +++
> lib/netdev-dpdk.c | 19 +++++++++++++------
> vswitchd/vswitch.xml | 19 +++++++++++++++++++
> 6 files changed, 81 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/topics/dpdk/vhost-user.rst
> b/Documentation/topics/dpdk/vhost-user.rst
> index 5347995..814c50b 100644
> --- a/Documentation/topics/dpdk/vhost-user.rst
> +++ b/Documentation/topics/dpdk/vhost-user.rst
> @@ -273,6 +273,35 @@ 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 IOMMU Support
> +-------------------
We need to state here that this supported only by vhost-user-client ports.
Maybe you need to change section name to "vhost-user-client IOMMU support"?
And add some clues to the text below.
Also, as soon as we're describing how to start QEMU with vhost-user* ports
in this document it'll be nice to have section about how to add IOMMU device
to QEMU cmdline/libvirt xml.
> +
> +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
> +
> +.. important::
> +
> + Changing this value after guest devices have already been initialized
> + will not toggle vHost IOMMU support. To that end, if vHost IOMMU
> + support is required, this field should be set to ```true```
> + when setting other global parameters on init (such as
> ```dpdk-socket-mem```,
> + for example).
As we can't actually configure this in runtime, we could just state that restart
is required on change. How about just this:
"The default is 'false'. 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 74e59bf..3e1a073 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
>
> v2.8.0 - 31 Aug 2017
> --------------------
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 8da6c32..c5120f7 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,
> @@ -312,6 +313,7 @@ dpdk_init__(const struct smap *ovs_other_config)
> int err = 0;
> cpu_set_t cpuset;
> char *sock_dir_subcomponent;
> + char *enable_vhost_iommu;
>
> log_stream = fopencookie(NULL, "w+", dpdk_log_func);
> if (log_stream == NULL) {
> @@ -345,6 +347,14 @@ dpdk_init__(const struct smap *ovs_other_config)
> vhost_sock_dir = sock_dir_subcomponent;
> }
>
> + if (process_vhost_flags("vhost-iommu-support", "false",
> + strlen("vhost-iommu-support"), ovs_other_config,
> + &enable_vhost_iommu)) {
> + vhost_iommu_enabled = (strncmp(enable_vhost_iommu, "true",
> + strlen("true")) == 0) ?
> + true : false;
> + }
> +
We may replace above code with just something like this:
----------------------------------------------------------------
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");
----------------------------------------------------------------
IHMO, it looks much better and properly handles upper/lowercase characters.
Beside that,
I guess, we should rename 'process_vhost_flags' somehow and use it only for
string parameters for which it was implemented. But we can do this in a separate
change someday.
> argv = grow_argv(&argv, 0, 1);
> argc = 1;
> argv[0] = xstrdup(ovs_get_program_name());
> @@ -482,6 +492,12 @@ dpdk_get_vhost_sock_dir(void)
> return vhost_sock_dir;
> }
>
> +bool
> +dpdk_vhost_iommu_enabled(void)
> +{
> + return vhost_iommu_enabled;
> +}
> +
You also need to add dummy implementation of this function to lib/dpdk-stub.c
> void
> dpdk_set_lcore_id(unsigned cpu)
> {
> diff --git a/lib/dpdk.h b/lib/dpdk.h
> index 673a1f1..83f0fac 100644
> --- a/lib/dpdk.h
> +++ b/lib/dpdk.h
> @@ -19,6 +19,8 @@
>
> #ifdef DPDK_NETDEV
>
> +#include <stdbool.h>
> +
Looks like you need to include this above the '#ifdef DPDK_NETDEV'.
> #include <rte_config.h>
> #include <rte_lcore.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..e190e0c 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,19 +3264,25 @@ 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);
> + "using client socket '%s'. vHost IOMMU support is %s.",
> + dev->up.name, dev->vhost_id,
> dpdk_vhost_iommu_enabled() ?
> + "enabled" : "disabled");
We don't need to print IOMMU status here because we already printed it on
sartup.
Also, this may confuse users, because they may start thinking that IOMMU is
really
in use while QEMU is not configured for that.
> }
>
> err = rte_vhost_driver_callback_register(dev->vhost_id,
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index c145e1a..d8e767b 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -344,6 +344,25 @@
> </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 after guest devices have already been
> initialized
> + does not toggle vHost IOMMU support. To that end, if vHost IOMMU
> + support is required, this field should be set to <code>true</code>
> + when setting other global parameters on init (such as
> + <ref column="other_config" key="dpdk-socket-mem"/>, for example).
Same as above. "Changing this value requires restarting the daemon." should be
enough.
> + </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
