>From: Ilya Maximets [mailto:[email protected]] >Sent: Thursday, December 7, 2017 12:19 PM >To: Kavanagh, Mark B <[email protected]>; Stokes, Ian ><[email protected]>; [email protected] >Cc: [email protected]; [email protected]; Mooney, Sean K ><[email protected]>; Fischetti, Antonio <[email protected]>; >Bie, Tiwei <[email protected]>; Mcnamara, John <[email protected]>; >Guoshuai Li <[email protected]>; [email protected]; Loftus, Ciara ><[email protected]>; Yuanhan Liu <[email protected]> >Subject: Re: [ovs-dev] [PATCH V4 2/2] netdev-dpdk: vHost IOMMU support > >On 07.12.2017 14:32, Kavanagh, Mark B wrote: >> Yuanhan's old email address was used in the previous mail - updated >correctly here. >> -Mark >> >>> -----Original Message----- >>> From: [email protected] [mailto:ovs-dev- >[email protected]] >>> On Behalf Of Kavanagh, Mark B >>> Sent: Thursday, December 7, 2017 11:24 AM >>> To: Stokes, Ian <[email protected]>; [email protected] >>> Cc: Liu, Yuanhan <[email protected]>; Bie, Tiwei <[email protected]>; >>> Mcnamara, John <[email protected]>; [email protected]; >>> [email protected] >>> Subject: Re: [ovs-dev] [PATCH V4 2/2] netdev-dpdk: vHost IOMMU support >>> >>> Hi folks, >>> >>> Thanks for all of your respective reviews and testing of this patchset. >>> >>> It seems, however, that an issue in DPDK v17.11 has come to light that >affects >>> guests which use testpmd. >>> Specifically, traffic does not reach a guest when traffic is live prior to >>> kicking off the testpmd app within said guest, and at least two forwarding >>> cores are used. [1] >>> >>> This is explained in additional detail is [2], an extract of which is >>> reproduced below: >>> >>> "the vector Rx could be broken if backend has consumed all the avail >>> descs before the >>> device is started. Because in current implementation, the vector Rx >>> will >>> return immediately >>> without refilling the avail ring if the used ring is empty. So we have >>> to refill >>> the avail ring after flushing the elements in the used ring." >>> >>> This issue was initially uncovered by Antonio Fischetti, as part of the >17.11 >>> patchset validation, and has since been localized to DPDK, rather than OvS. >>> As a result, it seems now that we should not move to 17.11, but seek an >out- >>> of-tree 17.11.1 stable/bugfix release. I'm interested in opinions on this - >>> should we: >>> a) move to 17.11 now, note the issue above in the 'errata' section of >>> the >>> documentation, and move to 17.11.1 when it becomes available in February of >>> next year >>> b) request the early release of the 17.11.1 stable branch, which >>> incorporates a fix for this issue (the possibility, and availability, of >which >>> are both TBD). >>> >>> Thanks in advance, >>> Mark > >Hmm. Isn't it a guest driver issue? Do we need to care so much about running >OVS inside the VM? If I assumed right, if we're running OVS not inside the VM, >there will be no issues on the OVS side. The issue happens if guest >application >uses DPDK 17.11, but it will happen regardless of the DPDK version on the host >side. > >So, the only bad scenario is running OVS inside the VM with virtio-net PMD >driver. >My question is: Do we need to care about this scenario so much? >If the answer is "not really" then we can use variant a). >But if it's very important thing we need to support than b) will be >preferable. > >Am I missing something?
Hey Ilya, I've just had the exact same conversation with Sean Mooney locally. I think the point that both yourself and Sean has made is completely valid, which puts option a) back on the table. Antonio has agreed to test that this works; once he confirms I can push v5 of the 17.11 patchet, and simply document the guest DPDK issue. Thanks again, Mark > >Best regards, Ilya Maximets. > > >>> >>> [1] http://dpdk.org/ml/archives/dev/2017-December/082801.html >>> [2] http://dpdk.org/ml/archives/dev/2017-December/082874.html >>> >>> >>>> -----Original Message----- >>>> From: Stokes, Ian >>>> Sent: Thursday, December 7, 2017 8:43 AM >>>> To: Kavanagh, Mark B <[email protected]>; [email protected] >>>> Cc: [email protected]; [email protected]; >[email protected]; >>>> [email protected]; Mooney, Sean K <[email protected]> >>>> Subject: RE: [ovs-dev][PATCH V4 2/2] netdev-dpdk: vHost IOMMU support >>>> >>>>> 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. >>>> >>>> Hi all, >>>> >>>> There were a few requests for changes in the v4 of this patch and it's an >>>> important aspect of the DPDK 17.11 support for OVS. >>>> >>>> I'd like to get this series in to the pull request this week. If people >have >>>> flagged issues for the latest revision I'd appreciate if they could review >>> the >>>> latest revision and flag any new issues that need to be raised. >>>> >>>> Ian >>>> >>>>> >>>>> 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 <[email protected]> >>>>> >>>>> --- >>>>> >>>>> 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 >>>>> + >>>>> +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> >>>>> -- >>>>> 2.7.4 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
