On 6/17/20 1:57 PM, Xia, Chenbo wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: dev <dev-boun...@dpdk.org> On Behalf Of Maxime Coquelin
>> Sent: Thursday, May 28, 2020 3:46 PM
>> To: dev@dpdk.org; amore...@redhat.com; Ye, Xiaolong
>> <xiaolong...@intel.com>; shah...@mellanox.com; ma...@mellanox.com
>> Cc: Maxime Coquelin <maxime.coque...@redhat.com>
>> Subject: [dpdk-dev] [PATCH 1/3] net/virtio: add vhost-user protocol features
>> support
>>
>> This patch adds support for Vhost-user protocol features.
>> It is required to support protocol features that were not in initial
>> Vhost-user
>> specification, such as reply-ack, MTU...
>>
>> Also, this patch prevents Virtio multiqueue feature negotiation if the slave
>> does
>> not support MQ protocol feature as stated in Vhost-user specification:
>> "The multiple queues feature is supported only when the protocol feature
>> ``VHOST_USER_PROTOCOL_F_MQ`` (bit 0) is set."
>>
>> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
>> ---
>> drivers/net/virtio/virtio_user/vhost.h | 9 +++++
>> drivers/net/virtio/virtio_user/vhost_user.c | 3 ++
>> .../net/virtio/virtio_user/virtio_user_dev.c | 39 ++++++++++++++++++-
>> .../net/virtio/virtio_user/virtio_user_dev.h | 3 ++
>> drivers/net/virtio/virtio_user_ethdev.c | 19 +++++++++
>> 5 files changed, 71 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>> b/drivers/net/virtio/virtio_user/vhost.h
>> index 1e784e58ef..9ace1a90c3 100644
>> --- a/drivers/net/virtio/virtio_user/vhost.h
>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>> @@ -44,6 +44,15 @@ struct vhost_vring_addr {
>> uint64_t log_guest_addr;
>> };
>>
>> +#ifndef VHOST_USER_F_PROTOCOL_FEATURES
>> +#define VHOST_USER_F_PROTOCOL_FEATURES 30 #endif
>> +
>> +/** Protocol features. */
>> +#ifndef VHOST_USER_PROTOCOL_F_MQ
>> +#define VHOST_USER_PROTOCOL_F_MQ 0
>> +#endif
>> +
>> enum vhost_user_request {
>> VHOST_USER_NONE = 0,
>> VHOST_USER_GET_FEATURES = 1,
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index 74b82e56e4..b687665042 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -269,10 +269,12 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>
>> switch (req) {
>> case VHOST_USER_GET_FEATURES:
>> + case VHOST_USER_GET_PROTOCOL_FEATURES:
>> need_reply = 1;
>> break;
>>
>> case VHOST_USER_SET_FEATURES:
>> + case VHOST_USER_SET_PROTOCOL_FEATURES:
>> case VHOST_USER_SET_LOG_BASE:
>> msg.payload.u64 = *((__u64 *)arg);
>> msg.size = sizeof(m.payload.u64);
>> @@ -351,6 +353,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>
>> switch (req) {
>> case VHOST_USER_GET_FEATURES:
>> + case VHOST_USER_GET_PROTOCOL_FEATURES:
>> if (msg.size != sizeof(m.payload.u64)) {
>> PMD_DRV_LOG(ERR, "Received bad msg size");
>> return -1;
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index 7fb135f49a..3afb09df2d 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -151,8 +151,10 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>> if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
>> goto error;
>>
>> - /* Step 1: set features */
>> + /* Step 1: negotiate protocol features & set features */
>> features = dev->features;
>> +
>> +
>> /* Strip VIRTIO_NET_F_MAC, as MAC address is handled in vdev init */
>> features &= ~(1ull << VIRTIO_NET_F_MAC);
>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know
>> */ @@ -417,13 +419,19 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>> 1ULL << VIRTIO_NET_F_GUEST_TSO6 | \
>> 1ULL << VIRTIO_F_IN_ORDER | \
>> 1ULL << VIRTIO_F_VERSION_1 | \
>> - 1ULL << VIRTIO_F_RING_PACKED)
>> + 1ULL << VIRTIO_F_RING_PACKED | \
>> + 1ULL << VHOST_USER_F_PROTOCOL_FEATURES)
>> +
>> +#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \
>> + (1ULL << VHOST_USER_PROTOCOL_F_MQ)
>>
>> int
>> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>> int cq, int queue_size, const char *mac, char **ifname,
>> int server, int mrg_rxbuf, int in_order, int packed_vq) {
>> + uint64_t protocol_features = 0;
>> +
>> pthread_mutex_init(&dev->mutex, NULL);
>> strlcpy(dev->path, path, PATH_MAX);
>> dev->started = 0;
>> @@ -434,6 +442,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>> dev->mac_specified = 0;
>> dev->frontend_features = 0;
>> dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES;
>> + dev->protocol_features =
>> VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES;
>> parse_mac(dev, mac);
>>
>> if (*ifname) {
>> @@ -446,6 +455,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>> return -1;
>> }
>>
>> + if (!is_vhost_user_by_type(dev->path))
>> + dev->unsupported_features |=
>> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
>> +
>> if (!dev->is_server) {
>> if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
>> NULL) < 0) {
>> @@ -460,6 +473,26 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>> strerror(errno));
>> return -1;
>> }
>> +
>> +
>> + if (dev->device_features &
>> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
>> + if (dev->ops->send_request(dev,
>> + VHOST_USER_GET_PROTOCOL_FEATURES,
>> + &protocol_features))
>> + return -1;
>> + }
>
> Should we put '}' after sending VHOST_USER_SET_PROTOCOL_FEATURES like in
> virtio_user_server_reconnect?
Good catch, we indeed don't want to send a Vhost-user request if it is
not supported by the Vhost-user backend.
>> +
>> + dev->protocol_features &= protocol_features;
>> +
>> + if (dev->ops->send_request(dev,
>> + VHOST_USER_SET_PROTOCOL_FEATURES,
>> + &dev->protocol_features))
>> + return -1;
>> +
>> + if (!(dev->protocol_features &
>> + (1ULL <<
>> VHOST_USER_PROTOCOL_F_MQ)))
>> + dev->unsupported_features |= (1ull <<
>> VIRTIO_NET_F_MQ);
>> } else {
>> /* We just pretend vhost-user can support all these features.
>> * Note that this could be problematic that if some feature is
>> @@ -469,6 +502,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>> dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
>> }
>>
>> +
>> +
>> if (!mrg_rxbuf)
>> dev->unsupported_features |= (1ull <<
>> VIRTIO_NET_F_MRG_RXBUF);
>>
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> index 3b6b6065a5..56e638f8a6 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -40,6 +40,9 @@ struct virtio_user_dev {
>> uint64_t device_features; /* supported features by device */
>> uint64_t frontend_features; /* enabled frontend features */
>> uint64_t unsupported_features; /* unsupported features mask
>> */
>> + uint64_t protocol_features; /* negotiated protocol features
>> + * (Vhost-user only)
>> + */
>> uint8_t status;
>> uint16_t port_id;
>> uint8_t mac_addr[RTE_ETHER_ADDR_LEN];
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index 798f191c32..ccb5a18e25 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -68,6 +68,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>> int connectfd;
>> struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
>> struct virtio_hw *hw = eth_dev->data->dev_private;
>> + uint64_t protocol_features;
>>
>> connectfd = accept(dev->listenfd, NULL, NULL);
>> if (connectfd < 0)
>> @@ -81,6 +82,24 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>> return -1;
>> }
>>
>> + if (dev->device_features &
>> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) {
>> + if (dev->ops->send_request(dev,
>> +
>> VHOST_USER_GET_PROTOCOL_FEATURES,
>> + &protocol_features))
>> + return -1;
>> +
>> + dev->protocol_features &= protocol_features;
>> +
>> + if (dev->ops->send_request(dev,
>> +
>> VHOST_USER_SET_PROTOCOL_FEATURES,
>> + &dev->protocol_features))
>> + return -1;
>> + }
>> +
>> + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
>> + dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
>
> Should we consider the case that vhost-user does not support
> VHOST_USER_F_PROTOCOL_FEATURES but support
> VIRTIO_NET_F_MQ? Because if the device negotiated feature does not include
> that, we should not use above logic to decide
> whether MQ is supported. If the case should be considered, the above two
> lines should be moved into last '{}' and
> same thing should be done in virtio_user_dev_init.
The Vhost-user specification says:
"
Masters must not rely on the ``VHOST_USER_PROTOCOL_F_MQ`` protocol
feature for
devices with a fixed number of virtqueues. Only true multiqueue devices
require this protocol feature.
"
Virtio-net device being a true multiqueue device it should require this.
But for now Virtio-user PMD does not use VHOST_USER_GET_QUEUE_NUM, so
it might not be mandatory.
I can drop it for now, but we should consider adding support for sending
VHOST_USER_GET_QUEUE_NUM, as the container may want to know how many
queues are supported by the Vhost-user backend or the vDPA device.
Thanks!
Maxime
> Thanks!
> Chenbo
>
>> +
>> dev->device_features |= dev->frontend_features;
>>
>> /* umask vhost-user unsupported features */
>> --
>> 2.26.2
>
