[25/40] net/virtio: add Virtio-user features ops
Checks
Commit Message
This patch introduce new callbacks for getting
and setting Virtio features, and implements them
for the different backend types.
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
drivers/net/virtio/virtio_user/vhost.h | 2 +
drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
.../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
.../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
.../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
drivers/net/virtio/virtio_user_ethdev.c | 3 +-
8 files changed, 188 insertions(+), 97 deletions(-)
Comments
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, December 21, 2020 5:14 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
> amorenoz@redhat.com; david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 25/40] net/virtio: add Virtio-user features ops
>
> This patch introduce new callbacks for getting
s/introduce/introduces
> and setting Virtio features, and implements them
> for the different backend types.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> drivers/net/virtio/virtio_user/vhost.h | 2 +
> drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
> .../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
> .../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
> drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
> drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
> .../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
> 8 files changed, 188 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> b/drivers/net/virtio/virtio_user/vhost.h
> index 8e819ecfb8..16978e27ed 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -102,6 +102,8 @@ struct virtio_user_dev;
> struct virtio_user_backend_ops {
> int (*setup)(struct virtio_user_dev *dev);
> int (*set_owner)(struct virtio_user_dev *dev);
> + int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
> + int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
> int (*send_request)(struct virtio_user_dev *dev,
> enum vhost_user_request req,
> void *arg);
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
> b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index b79dcad179..f44df8ef1f 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -38,6 +38,28 @@ struct vhost_memory_kernel {
> #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
> #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct
> vhost_vring_file)
>
> +/* with below features, vhost kernel does not need to do the checksum and TSO,
> + * these info will be passed to virtio_user through virtio net header.
> + */
> +#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
> + ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> + (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
> +/* with below features, when flows from virtio_user to vhost kernel
> + * (1) if flows goes up through the kernel networking stack, it does not need
> + * to verify checksum, which can save CPU cycles;
> + * (2) if flows goes through a Linux bridge and outside from an interface
> + * (kernel driver), checksum and TSO will be done by GSO in kernel or even
> + * offloaded into real physical device.
> + */
> +#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
> + ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> + (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> + (1ULL << VIRTIO_NET_F_CSUM))
> +
> static uint64_t max_regions = 64;
>
> static void
> @@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
> return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
> }
>
> +static int
> +vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
> +{
> + int ret;
> + unsigned int tap_features;
> +
> + ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
> + if (ret < 0) {
> + PMD_DRV_LOG(ERR, "Failed to get features");
> + return -1;
> + }
> +
> + ret = tap_support_features(&tap_features);
> + if (ret < 0) {
> + PMD_DRV_LOG(ERR, "Failed to get TAP features)");
should delete ')' after 'features'?
> + return -1;
> + }
> +
> + /* with tap as the backend, all these features are supported
> + * but not claimed by vhost-net, so we add them back when
> + * reporting to upper layer.
> + */
<snip>
> .dma_map = vhost_vdpa_dma_map,
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index f8e4581951..0a85d058a8 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> features &= ~(1ull << VIRTIO_NET_F_STATUS);
> - ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
> + ret = dev->ops->set_features(dev, features);
I noticed that virtio_user_dev_set_features is called by virtio_user_set_status.
The former may fail but the latter will ignore the failure. So this will happen:
setting features already failed but virtio-user still continue to do things. IMHO,
this is not very good (similar things may happen for virtio_user_start_device).
What do you think?
Thanks,
Chenbo
> if (ret < 0)
> goto error;
> PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
> @@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
> return -1;
> }
>
> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> - &dev->device_features) < 0) {
> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
> PMD_INIT_LOG(ERR, "get_features failed: %s",
> strerror(errno));
> return -1;
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 283f5c7a36..4d2635c8aa 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>
> virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>
> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> - &dev->device_features) < 0) {
> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
> PMD_INIT_LOG(ERR, "get_features failed: %s",
> strerror(errno));
> return -1;
> --
> 2.29.2
Hi Chenbo
On 1/6/21 12:54 PM, Xia, Chenbo wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Monday, December 21, 2020 5:14 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
>> amorenoz@redhat.com; david.marchand@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 25/40] net/virtio: add Virtio-user features ops
>>
>> This patch introduce new callbacks for getting
>
> s/introduce/introduces
>
>> and setting Virtio features, and implements them
>> for the different backend types.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> drivers/net/virtio/virtio_user/vhost.h | 2 +
>> drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
>> .../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
>> .../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
>> drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
>> .../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
>> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
>> 8 files changed, 188 insertions(+), 97 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>> b/drivers/net/virtio/virtio_user/vhost.h
>> index 8e819ecfb8..16978e27ed 100644
>> --- a/drivers/net/virtio/virtio_user/vhost.h
>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>> @@ -102,6 +102,8 @@ struct virtio_user_dev;
>> struct virtio_user_backend_ops {
>> int (*setup)(struct virtio_user_dev *dev);
>> int (*set_owner)(struct virtio_user_dev *dev);
>> + int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
>> + int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
>> int (*send_request)(struct virtio_user_dev *dev,
>> enum vhost_user_request req,
>> void *arg);
>> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
>> b/drivers/net/virtio/virtio_user/vhost_kernel.c
>> index b79dcad179..f44df8ef1f 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
>> @@ -38,6 +38,28 @@ struct vhost_memory_kernel {
>> #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
>> #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct
>> vhost_vring_file)
>>
>> +/* with below features, vhost kernel does not need to do the checksum and TSO,
>> + * these info will be passed to virtio_user through virtio net header.
>> + */
>> +#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
>> + ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
>> + (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
>> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
>> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
>> +
>> +/* with below features, when flows from virtio_user to vhost kernel
>> + * (1) if flows goes up through the kernel networking stack, it does not need
>> + * to verify checksum, which can save CPU cycles;
>> + * (2) if flows goes through a Linux bridge and outside from an interface
>> + * (kernel driver), checksum and TSO will be done by GSO in kernel or even
>> + * offloaded into real physical device.
>> + */
>> +#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
>> + ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
>> + (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
>> + (1ULL << VIRTIO_NET_F_CSUM))
>> +
>> static uint64_t max_regions = 64;
>>
>> static void
>> @@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
>> return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
>> }
>>
>> +static int
>> +vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
>> +{
>> + int ret;
>> + unsigned int tap_features;
>> +
>> + ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
>> + if (ret < 0) {
>> + PMD_DRV_LOG(ERR, "Failed to get features");
>> + return -1;
>> + }
>> +
>> + ret = tap_support_features(&tap_features);
>> + if (ret < 0) {
>> + PMD_DRV_LOG(ERR, "Failed to get TAP features)");
>
> should delete ')' after 'features'?
>
>> + return -1;
>> + }
>> +
>> + /* with tap as the backend, all these features are supported
>> + * but not claimed by vhost-net, so we add them back when
>> + * reporting to upper layer.
>> + */
>
> <snip>
>
>> .dma_map = vhost_vdpa_dma_map,
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index f8e4581951..0a85d058a8 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>> features &= ~(1ull << VIRTIO_NET_F_STATUS);
>> - ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
>> + ret = dev->ops->set_features(dev, features);
>
> I noticed that virtio_user_dev_set_features is called by virtio_user_set_status.
> The former may fail but the latter will ignore the failure. So this will happen:
> setting features already failed but virtio-user still continue to do things. IMHO,
> this is not very good (similar things may happen for virtio_user_start_device).
> What do you think?
>
You're right. Although set_status(features_ok) should be followed by
get_status() to check if device has set feature_ok bit, the problem is that
set_status is called on device initialization, which would happen even when the
vhost backed is not yet connected (server-mode). Failing there would tear down
the ethdev which would make the future "hotplug" fail. Hopefully this will all
be fixed by Maxime's refactoring :)
So +1 to reconsider improving error propagation here.
> Thanks,
> Chenbo
>
>> if (ret < 0)
>> goto error;
>> PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
>> @@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>> *path, int queues,
>> return -1;
>> }
>>
>> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>> - &dev->device_features) < 0) {
>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>> PMD_INIT_LOG(ERR, "get_features failed: %s",
>> strerror(errno));
>> return -1;
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index 283f5c7a36..4d2635c8aa 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>>
>> virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>
>> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>> - &dev->device_features) < 0) {
>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>> PMD_INIT_LOG(ERR, "get_features failed: %s",
>> strerror(errno));
>> return -1;
>> --
>> 2.29.2
>
On 1/13/21 2:43 PM, Adrian Moreno wrote:
> Hi Chenbo
>
> On 1/6/21 12:54 PM, Xia, Chenbo wrote:
>> Hi Maxime,
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Monday, December 21, 2020 5:14 AM
>>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
>>> amorenoz@redhat.com; david.marchand@redhat.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [PATCH 25/40] net/virtio: add Virtio-user features ops
>>>
>>> This patch introduce new callbacks for getting
>>
>> s/introduce/introduces
>>
>>> and setting Virtio features, and implements them
>>> for the different backend types.
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>> drivers/net/virtio/virtio_user/vhost.h | 2 +
>>> drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
>>> .../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
>>> .../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
>>> drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
>>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
>>> .../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
>>> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
>>> 8 files changed, 188 insertions(+), 97 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>>> b/drivers/net/virtio/virtio_user/vhost.h
>>> index 8e819ecfb8..16978e27ed 100644
>>> --- a/drivers/net/virtio/virtio_user/vhost.h
>>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>>> @@ -102,6 +102,8 @@ struct virtio_user_dev;
>>> struct virtio_user_backend_ops {
>>> int (*setup)(struct virtio_user_dev *dev);
>>> int (*set_owner)(struct virtio_user_dev *dev);
>>> + int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
>>> + int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
>>> int (*send_request)(struct virtio_user_dev *dev,
>>> enum vhost_user_request req,
>>> void *arg);
>>> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> b/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> index b79dcad179..f44df8ef1f 100644
>>> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> @@ -38,6 +38,28 @@ struct vhost_memory_kernel {
>>> #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
>>> #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct
>>> vhost_vring_file)
>>>
>>> +/* with below features, vhost kernel does not need to do the checksum and TSO,
>>> + * these info will be passed to virtio_user through virtio net header.
>>> + */
>>> +#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
>>> + ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
>>> +
>>> +/* with below features, when flows from virtio_user to vhost kernel
>>> + * (1) if flows goes up through the kernel networking stack, it does not need
>>> + * to verify checksum, which can save CPU cycles;
>>> + * (2) if flows goes through a Linux bridge and outside from an interface
>>> + * (kernel driver), checksum and TSO will be done by GSO in kernel or even
>>> + * offloaded into real physical device.
>>> + */
>>> +#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
>>> + ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
>>> + (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
>>> + (1ULL << VIRTIO_NET_F_CSUM))
>>> +
>>> static uint64_t max_regions = 64;
>>>
>>> static void
>>> @@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
>>> return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
>>> }
>>>
>>> +static int
>>> +vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
>>> +{
>>> + int ret;
>>> + unsigned int tap_features;
>>> +
>>> + ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
>>> + if (ret < 0) {
>>> + PMD_DRV_LOG(ERR, "Failed to get features");
>>> + return -1;
>>> + }
>>> +
>>> + ret = tap_support_features(&tap_features);
>>> + if (ret < 0) {
>>> + PMD_DRV_LOG(ERR, "Failed to get TAP features)");
>>
>> should delete ')' after 'features'?
>>
>>> + return -1;
>>> + }
>>> +
>>> + /* with tap as the backend, all these features are supported
>>> + * but not claimed by vhost-net, so we add them back when
>>> + * reporting to upper layer.
>>> + */
>>
>> <snip>
>>
>>> .dma_map = vhost_vdpa_dma_map,
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index f8e4581951..0a85d058a8 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>> features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>> - ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
>>> + ret = dev->ops->set_features(dev, features);
>>
>> I noticed that virtio_user_dev_set_features is called by virtio_user_set_status.
>> The former may fail but the latter will ignore the failure. So this will happen:
>> setting features already failed but virtio-user still continue to do things. IMHO,
>> this is not very good (similar things may happen for virtio_user_start_device).
>> What do you think?
>>
> You're right. Although set_status(features_ok) should be followed by
> get_status() to check if device has set feature_ok bit, the problem is that
> set_status is called on device initialization, which would happen even when the
> vhost backed is not yet connected (server-mode). Failing there would tear down
> the ethdev which would make the future "hotplug" fail. Hopefully this will all
> be fixed by Maxime's refactoring :)
> So +1 to reconsider improving error propagation here.
Yes, makes sense.
Sorry for not replying earlier, I'm rebasing the series and than will
look at all your comments one by one.
Thanks,
Maxime
>> Thanks,
>> Chenbo
>>
>>> if (ret < 0)
>>> goto error;
>>> PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
>>> @@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>>> *path, int queues,
>>> return -1;
>>> }
>>>
>>> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>> - &dev->device_features) < 0) {
>>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>>> PMD_INIT_LOG(ERR, "get_features failed: %s",
>>> strerror(errno));
>>> return -1;
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>> index 283f5c7a36..4d2635c8aa 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>>>
>>> virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>
>>> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>> - &dev->device_features) < 0) {
>>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>>> PMD_INIT_LOG(ERR, "get_features failed: %s",
>>> strerror(errno));
>>> return -1;
>>> --
>>> 2.29.2
>>
>
On 12/20/20 10:13 PM, Maxime Coquelin wrote:
> This patch introduce new callbacks for getting
> and setting Virtio features, and implements them
> for the different backend types.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> drivers/net/virtio/virtio_user/vhost.h | 2 +
> drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
> .../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
> .../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
> drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
> drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
> .../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
> 8 files changed, 188 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h
> index 8e819ecfb8..16978e27ed 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -102,6 +102,8 @@ struct virtio_user_dev;
> struct virtio_user_backend_ops {
> int (*setup)(struct virtio_user_dev *dev);
> int (*set_owner)(struct virtio_user_dev *dev);
> + int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
> + int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
> int (*send_request)(struct virtio_user_dev *dev,
> enum vhost_user_request req,
> void *arg);
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index b79dcad179..f44df8ef1f 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -38,6 +38,28 @@ struct vhost_memory_kernel {
> #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
> #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_vring_file)
>
> +/* with below features, vhost kernel does not need to do the checksum and TSO,
> + * these info will be passed to virtio_user through virtio net header.
> + */
> +#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
> + ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> + (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
> +/* with below features, when flows from virtio_user to vhost kernel
> + * (1) if flows goes up through the kernel networking stack, it does not need
> + * to verify checksum, which can save CPU cycles;
> + * (2) if flows goes through a Linux bridge and outside from an interface
> + * (kernel driver), checksum and TSO will be done by GSO in kernel or even
> + * offloaded into real physical device.
> + */
> +#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
nit: backslash alignment
> + ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> + (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> + (1ULL << VIRTIO_NET_F_CSUM))
> +
> static uint64_t max_regions = 64;
>
> static void
> @@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
> return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
> }
>
> +static int
> +vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
> +{
> + int ret;
> + unsigned int tap_features;
> +
> + ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
> + if (ret < 0) {
> + PMD_DRV_LOG(ERR, "Failed to get features");
> + return -1;
> + }
> +
> + ret = tap_support_features(&tap_features);
> + if (ret < 0) {
> + PMD_DRV_LOG(ERR, "Failed to get TAP features)");
> + return -1;
> + }
> +
> + /* with tap as the backend, all these features are supported
> + * but not claimed by vhost-net, so we add them back when
> + * reporting to upper layer.
> + */
> + if (tap_features & IFF_VNET_HDR) {
> + *features |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> + *features |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
> + }
> +
> + /* vhost_kernel will not declare this feature, but it does
> + * support multi-queue.
> + */
> + if (tap_features & IFF_MULTI_QUEUE)
> + *features |= (1ull << VIRTIO_NET_F_MQ);
> +
> + return 0;
> +}
> +
> +static int
> +vhost_kernel_set_features(struct virtio_user_dev *dev, uint64_t features)
> +{
> + /* We don't need memory protection here */
> + features &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
> + /* VHOST kernel does not know about below flags */
> + features &= ~VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> + features &= ~VHOST_KERNEL_HOST_OFFLOADS_MASK;
> + features &= ~(1ULL << VIRTIO_NET_F_MQ);
> +
> + return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_FEATURES, &features);
> +}
> +
> static uint64_t vhost_req_user_to_kernel[] = {
> [VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER,
> - [VHOST_USER_SET_FEATURES] = VHOST_SET_FEATURES,
> - [VHOST_USER_GET_FEATURES] = VHOST_GET_FEATURES,
> [VHOST_USER_SET_VRING_CALL] = VHOST_SET_VRING_CALL,
> [VHOST_USER_SET_VRING_NUM] = VHOST_SET_VRING_NUM,
> [VHOST_USER_SET_VRING_BASE] = VHOST_SET_VRING_BASE,
> @@ -150,51 +219,6 @@ prepare_vhost_memory_kernel(void)
> return vm;
> }
>
> -/* with below features, vhost kernel does not need to do the checksum and TSO,
> - * these info will be passed to virtio_user through virtio net header.
> - */
> -#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
> - ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> - (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> - (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> - (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> - (1ULL << VIRTIO_NET_F_GUEST_UFO))
> -
> -/* with below features, when flows from virtio_user to vhost kernel
> - * (1) if flows goes up through the kernel networking stack, it does not need
> - * to verify checksum, which can save CPU cycles;
> - * (2) if flows goes through a Linux bridge and outside from an interface
> - * (kernel driver), checksum and TSO will be done by GSO in kernel or even
> - * offloaded into real physical device.
> - */
> -#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
> - ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> - (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> - (1ULL << VIRTIO_NET_F_CSUM))
> -
> -static unsigned int
> -tap_support_features(void)
> -{
> - int tapfd;
> - unsigned int tap_features;
> -
> - tapfd = open(PATH_NET_TUN, O_RDWR);
> - if (tapfd < 0) {
> - PMD_DRV_LOG(ERR, "fail to open %s: %s",
> - PATH_NET_TUN, strerror(errno));
> - return -1;
> - }
> -
> - if (ioctl(tapfd, TUNGETFEATURES, &tap_features) == -1) {
> - PMD_DRV_LOG(ERR, "TUNGETFEATURES failed: %s", strerror(errno));
> - close(tapfd);
> - return -1;
> - }
> -
> - close(tapfd);
> - return tap_features;
> -}
> -
> static int
> vhost_kernel_send_request(struct virtio_user_dev *dev,
> enum vhost_user_request req,
> @@ -206,7 +230,6 @@ vhost_kernel_send_request(struct virtio_user_dev *dev,
> struct vhost_memory_kernel *vm = NULL;
> int vhostfd;
> unsigned int queue_sel;
> - unsigned int features;
>
> PMD_DRV_LOG(INFO, "%s", vhost_msg_strings[req]);
>
> @@ -219,17 +242,6 @@ vhost_kernel_send_request(struct virtio_user_dev *dev,
> arg = (void *)vm;
> }
>
> - if (req_kernel == VHOST_SET_FEATURES) {
> - /* We don't need memory protection here */
> - *(uint64_t *)arg &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
> -
> - /* VHOST kernel does not know about below flags */
> - *(uint64_t *)arg &= ~VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> - *(uint64_t *)arg &= ~VHOST_KERNEL_HOST_OFFLOADS_MASK;
> -
> - *(uint64_t *)arg &= ~(1ULL << VIRTIO_NET_F_MQ);
> - }
> -
> switch (req_kernel) {
> case VHOST_SET_VRING_NUM:
> case VHOST_SET_VRING_ADDR:
> @@ -259,24 +271,6 @@ vhost_kernel_send_request(struct virtio_user_dev *dev,
> ret = ioctl(vhostfd, req_kernel, arg);
> }
>
> - if (!ret && req_kernel == VHOST_GET_FEATURES) {
> - features = tap_support_features();
> - /* with tap as the backend, all these features are supported
> - * but not claimed by vhost-net, so we add them back when
> - * reporting to upper layer.
> - */
> - if (features & IFF_VNET_HDR) {
> - *((uint64_t *)arg) |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
> - *((uint64_t *)arg) |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
> - }
> -
> - /* vhost_kernel will not declare this feature, but it does
> - * support multi-queue.
> - */
> - if (features & IFF_MULTI_QUEUE)
> - *(uint64_t *)arg |= (1ull << VIRTIO_NET_F_MQ);
> - }
> -
> if (vm)
> free(vm);
>
> @@ -407,6 +401,8 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
> struct virtio_user_backend_ops virtio_ops_kernel = {
> .setup = vhost_kernel_setup,
> .set_owner = vhost_kernel_set_owner,
> + .get_features = vhost_kernel_get_features,
> + .set_features = vhost_kernel_set_features,
> .send_request = vhost_kernel_send_request,
> .enable_qp = vhost_kernel_enable_queue_pair
> };
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> index eade702c5c..99096bdf39 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> @@ -18,6 +18,29 @@
> #include "../virtio_logs.h"
> #include "../virtio.h"
>
> +
> +int
> +tap_support_features(unsigned int *tap_features)
> +{
> + int tapfd;
> +
> + tapfd = open(PATH_NET_TUN, O_RDWR);
> + if (tapfd < 0) {
> + PMD_DRV_LOG(ERR, "fail to open %s: %s",
> + PATH_NET_TUN, strerror(errno));
> + return -1;
> + }
> +
> + if (ioctl(tapfd, TUNGETFEATURES, tap_features) == -1) {
> + PMD_DRV_LOG(ERR, "TUNGETFEATURES failed: %s", strerror(errno));
> + close(tapfd);
> + return -1;
> + }
> +
> + close(tapfd);
> + return 0;
> +}
> +
> int
> vhost_kernel_tap_set_offload(int fd, uint64_t features)
> {
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> index 5c4447b296..ed03fce21e 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> @@ -43,5 +43,6 @@ int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> const char *mac, uint64_t features);
> int vhost_kernel_tap_set_offload(int fd, uint64_t features);
> int vhost_kernel_tap_set_queue(int fd, bool attach);
> +int tap_support_features(unsigned int *tap_features);
>
> #endif
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
> index 5ab15318ac..d204fa1eb0 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -143,6 +143,62 @@ vhost_user_set_owner(struct virtio_user_dev *dev)
> return 0;
> }
>
> +static int
> +vhost_user_get_features(struct virtio_user_dev *dev, uint64_t *features)
> +{
> + int ret;
> + struct vhost_user_msg msg = {
> + .request = VHOST_USER_GET_FEATURES,
> + .flags = VHOST_USER_VERSION,
> + };
> +
> + ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0);
> + if (ret < 0)
> + goto err;
> +
> + ret = vhost_user_read(dev->vhostfd, &msg);
> + if (ret < 0)
> + goto err;
> +
> + if (msg.request != VHOST_USER_GET_FEATURES) {
> + PMD_DRV_LOG(ERR, "Unexpected request type (%d)", msg.request);
> + goto err;
> + }
> +
> + if (msg.size != sizeof(*features)) {
> + PMD_DRV_LOG(ERR, "Unexpected payload size (%d)", msg.size);
> + goto err;
> + }
> +
> + *features = msg.payload.u64;
> +
> + return 0;
> +err:
> + PMD_DRV_LOG(ERR, "Failed to get backend features");
> +
On possibly socket-related errors, it's sometimes useful to print
sterror(errno). WDYT?
> + return -1;
> +}
> +
> +static int
> +vhost_user_set_features(struct virtio_user_dev *dev, uint64_t features)
> +{
> + int ret;
> + struct vhost_user_msg msg = {
> + .request = VHOST_USER_SET_FEATURES,
> + .flags = VHOST_USER_VERSION,
> + .size = sizeof(features),
> + .payload.u64 = features,
> + };
> +
> + ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0);
> + if (ret < 0) {
> + PMD_DRV_LOG(ERR, "Failed to set features");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> struct walk_arg {
> struct vhost_memory *vm;
> int *fds;
> @@ -249,8 +305,6 @@ static struct vhost_user_msg m;
>
> const char * const vhost_msg_strings[] = {
> [VHOST_USER_RESET_OWNER] = "VHOST_RESET_OWNER",
> - [VHOST_USER_SET_FEATURES] = "VHOST_SET_FEATURES",
> - [VHOST_USER_GET_FEATURES] = "VHOST_GET_FEATURES",
> [VHOST_USER_SET_VRING_CALL] = "VHOST_SET_VRING_CALL",
> [VHOST_USER_SET_VRING_NUM] = "VHOST_SET_VRING_NUM",
> [VHOST_USER_SET_VRING_BASE] = "VHOST_SET_VRING_BASE",
> @@ -299,7 +353,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
> (1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
> return -ENOTSUP;
> /* Fallthrough */
> - case VHOST_USER_GET_FEATURES:
> case VHOST_USER_GET_PROTOCOL_FEATURES:
> need_reply = 1;
> break;
> @@ -313,7 +366,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
> if (has_reply_ack)
> msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> /* Fallthrough */
> - case VHOST_USER_SET_FEATURES:
> case VHOST_USER_SET_PROTOCOL_FEATURES:
> case VHOST_USER_SET_LOG_BASE:
> msg.payload.u64 = *((__u64 *)arg);
> @@ -393,7 +445,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
> }
>
> switch (req) {
> - case VHOST_USER_GET_FEATURES:
> case VHOST_USER_GET_STATUS:
> case VHOST_USER_GET_PROTOCOL_FEATURES:
> if (msg.size != sizeof(m.payload.u64)) {
> @@ -531,6 +582,8 @@ vhost_user_enable_queue_pair(struct virtio_user_dev *dev,
> struct virtio_user_backend_ops virtio_ops_user = {
> .setup = vhost_user_setup,
> .set_owner = vhost_user_set_owner,
> + .get_features = vhost_user_get_features,
> + .set_features = vhost_user_set_features,
> .send_request = vhost_user_sock,
> .enable_qp = vhost_user_enable_queue_pair
> };
> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c b/drivers/net/virtio/virtio_user/vhost_vdpa.c
> index f405b6b86a..c0a9b5b767 100644
> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c
> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c
> @@ -38,8 +38,6 @@
>
> static uint64_t vhost_req_user_to_vdpa[] = {
> [VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER,
> - [VHOST_USER_SET_FEATURES] = VHOST_SET_FEATURES,
> - [VHOST_USER_GET_FEATURES] = VHOST_GET_FEATURES,
> [VHOST_USER_SET_VRING_CALL] = VHOST_SET_VRING_CALL,
> [VHOST_USER_SET_VRING_NUM] = VHOST_SET_VRING_NUM,
> [VHOST_USER_SET_VRING_BASE] = VHOST_SET_VRING_BASE,
> @@ -101,6 +99,32 @@ vhost_vdpa_set_owner(struct virtio_user_dev *dev)
> return vhost_vdpa_ioctl(dev->vhostfd, VHOST_SET_OWNER, NULL);
> }
>
> +static int
> +vhost_vdpa_get_features(struct virtio_user_dev *dev, uint64_t *features)
> +{
> + int ret;
> +
> + ret = vhost_vdpa_ioctl(dev->vhostfd, VHOST_GET_FEATURES, features);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Failed to get features");
> + return -1;
> + }
> +
> + /* Multiqueue not supported for now */
> + *features &= ~(1ULL << VIRTIO_NET_F_MQ);
> +
> + return 0;
> +}
> +
> +static int
> +vhost_vdpa_set_features(struct virtio_user_dev *dev, uint64_t features)
> +{
> + /* WORKAROUND */
> + features |= 1ULL << VIRTIO_F_IOMMU_PLATFORM;
> +
> + return vhost_vdpa_ioctl(dev->vhostfd, VHOST_SET_FEATURES, &features);
> +}
> +
> static int
> vhost_vdpa_dma_map(struct virtio_user_dev *dev, void *addr,
> uint64_t iova, size_t len)
> @@ -227,14 +251,6 @@ vhost_vdpa_send_request(struct virtio_user_dev *dev,
> if (req_vdpa == VHOST_SET_MEM_TABLE)
> return vhost_vdpa_dma_map_all(dev);
>
> - if (req_vdpa == VHOST_SET_FEATURES) {
> - /* WORKAROUND */
> - *(uint64_t *)arg |= 1ULL << VIRTIO_F_IOMMU_PLATFORM;
> -
> - /* Multiqueue not supported for now */
> - *(uint64_t *)arg &= ~(1ULL << VIRTIO_NET_F_MQ);
> - }
> -
> switch (req_vdpa) {
> case VHOST_SET_VRING_NUM:
> case VHOST_SET_VRING_ADDR:
> @@ -313,6 +329,8 @@ vhost_vdpa_enable_queue_pair(struct virtio_user_dev *dev,
> struct virtio_user_backend_ops virtio_ops_vdpa = {
> .setup = vhost_vdpa_setup,
> .set_owner = vhost_vdpa_set_owner,
> + .get_features = vhost_vdpa_get_features,
> + .set_features = vhost_vdpa_set_features,
> .send_request = vhost_vdpa_send_request,
> .enable_qp = vhost_vdpa_enable_queue_pair,
> .dma_map = vhost_vdpa_dma_map,
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index f8e4581951..0a85d058a8 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> features &= ~(1ull << VIRTIO_NET_F_STATUS);
> - ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
> + ret = dev->ops->set_features(dev, features);
> if (ret < 0)
> goto error;
> PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
> @@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> return -1;
> }
>
> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> - &dev->device_features) < 0) {
> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
> PMD_INIT_LOG(ERR, "get_features failed: %s",
> strerror(errno));
> return -1;
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index 283f5c7a36..4d2635c8aa 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>
> virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>
> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> - &dev->device_features) < 0) {
> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
> PMD_INIT_LOG(ERR, "get_features failed: %s",
> strerror(errno));
> return -1;
>
On 1/13/21 2:43 PM, Adrian Moreno wrote:
> Hi Chenbo
>
> On 1/6/21 12:54 PM, Xia, Chenbo wrote:
>> Hi Maxime,
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Monday, December 21, 2020 5:14 AM
>>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
>>> amorenoz@redhat.com; david.marchand@redhat.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [PATCH 25/40] net/virtio: add Virtio-user features ops
>>>
>>> This patch introduce new callbacks for getting
>>
>> s/introduce/introduces
>>
>>> and setting Virtio features, and implements them
>>> for the different backend types.
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>> drivers/net/virtio/virtio_user/vhost.h | 2 +
>>> drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
>>> .../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
>>> .../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
>>> drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
>>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
>>> .../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
>>> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
>>> 8 files changed, 188 insertions(+), 97 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>>> b/drivers/net/virtio/virtio_user/vhost.h
>>> index 8e819ecfb8..16978e27ed 100644
>>> --- a/drivers/net/virtio/virtio_user/vhost.h
>>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>>> @@ -102,6 +102,8 @@ struct virtio_user_dev;
>>> struct virtio_user_backend_ops {
>>> int (*setup)(struct virtio_user_dev *dev);
>>> int (*set_owner)(struct virtio_user_dev *dev);
>>> + int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
>>> + int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
>>> int (*send_request)(struct virtio_user_dev *dev,
>>> enum vhost_user_request req,
>>> void *arg);
>>> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> b/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> index b79dcad179..f44df8ef1f 100644
>>> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> @@ -38,6 +38,28 @@ struct vhost_memory_kernel {
>>> #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
>>> #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct
>>> vhost_vring_file)
>>>
>>> +/* with below features, vhost kernel does not need to do the checksum and TSO,
>>> + * these info will be passed to virtio_user through virtio net header.
>>> + */
>>> +#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
>>> + ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
>>> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
>>> +
>>> +/* with below features, when flows from virtio_user to vhost kernel
>>> + * (1) if flows goes up through the kernel networking stack, it does not need
>>> + * to verify checksum, which can save CPU cycles;
>>> + * (2) if flows goes through a Linux bridge and outside from an interface
>>> + * (kernel driver), checksum and TSO will be done by GSO in kernel or even
>>> + * offloaded into real physical device.
>>> + */
>>> +#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
>>> + ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
>>> + (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
>>> + (1ULL << VIRTIO_NET_F_CSUM))
>>> +
>>> static uint64_t max_regions = 64;
>>>
>>> static void
>>> @@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
>>> return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
>>> }
>>>
>>> +static int
>>> +vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
>>> +{
>>> + int ret;
>>> + unsigned int tap_features;
>>> +
>>> + ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
>>> + if (ret < 0) {
>>> + PMD_DRV_LOG(ERR, "Failed to get features");
>>> + return -1;
>>> + }
>>> +
>>> + ret = tap_support_features(&tap_features);
>>> + if (ret < 0) {
>>> + PMD_DRV_LOG(ERR, "Failed to get TAP features)");
>>
>> should delete ')' after 'features'?
>>
>>> + return -1;
>>> + }
>>> +
>>> + /* with tap as the backend, all these features are supported
>>> + * but not claimed by vhost-net, so we add them back when
>>> + * reporting to upper layer.
>>> + */
>>
>> <snip>
>>
>>> .dma_map = vhost_vdpa_dma_map,
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index f8e4581951..0a85d058a8 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>> features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>> - ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
>>> + ret = dev->ops->set_features(dev, features);
>>
>> I noticed that virtio_user_dev_set_features is called by virtio_user_set_status.
>> The former may fail but the latter will ignore the failure. So this will happen:
>> setting features already failed but virtio-user still continue to do things. IMHO,
>> this is not very good (similar things may happen for virtio_user_start_device).
>> What do you think?
>>
> You're right. Although set_status(features_ok) should be followed by
> get_status() to check if device has set feature_ok bit, the problem is that
> set_status is called on device initialization, which would happen even when the
> vhost backed is not yet connected (server-mode). Failing there would tear down
> the ethdev which would make the future "hotplug" fail. Hopefully this will all
> be fixed by Maxime's refactoring :)
> So +1 to reconsider improving error propagation here.
I agree with the idea of improving the error propagation, maybe it can
be done after the series is merged as further improvements.
With making server mode made blocking, it will be easier to handle.
But there is still the reconnection case that could be problematic, I
prefer not risking introducing more regressions for this series :).
Is that OK for you?
Cheers,
Maxime
>
>> Thanks,
>> Chenbo
>>
>>> if (ret < 0)
>>> goto error;
>>> PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
>>> @@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>>> *path, int queues,
>>> return -1;
>>> }
>>>
>>> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>> - &dev->device_features) < 0) {
>>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>>> PMD_INIT_LOG(ERR, "get_features failed: %s",
>>> strerror(errno));
>>> return -1;
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>> index 283f5c7a36..4d2635c8aa 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>>>
>>> virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>
>>> - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>> - &dev->device_features) < 0) {
>>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>>> PMD_INIT_LOG(ERR, "get_features failed: %s",
>>> strerror(errno));
>>> return -1;
>>> --
>>> 2.29.2
>>
>
On 1/13/21 2:57 PM, Adrian Moreno wrote:
>
>
> On 12/20/20 10:13 PM, Maxime Coquelin wrote:
>> This patch introduce new callbacks for getting
>> and setting Virtio features, and implements them
>> for the different backend types.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> drivers/net/virtio/virtio_user/vhost.h | 2 +
>> drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
>> .../net/virtio/virtio_user/vhost_kernel_tap.c | 23 +++
>> .../net/virtio/virtio_user/vhost_kernel_tap.h | 1 +
>> drivers/net/virtio/virtio_user/vhost_user.c | 63 +++++++-
>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 38 +++--
>> .../net/virtio/virtio_user/virtio_user_dev.c | 5 +-
>> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
>> 8 files changed, 188 insertions(+), 97 deletions(-)
>>
...
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
>> index 5ab15318ac..d204fa1eb0 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -143,6 +143,62 @@ vhost_user_set_owner(struct virtio_user_dev *dev)
>> return 0;
>> }
>>
>> +static int
>> +vhost_user_get_features(struct virtio_user_dev *dev, uint64_t *features)
>> +{
>> + int ret;
>> + struct vhost_user_msg msg = {
>> + .request = VHOST_USER_GET_FEATURES,
>> + .flags = VHOST_USER_VERSION,
>> + };
>> +
>> + ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0);
>> + if (ret < 0)
>> + goto err;
>> +
>> + ret = vhost_user_read(dev->vhostfd, &msg);
>> + if (ret < 0)
>> + goto err;
>> +
>> + if (msg.request != VHOST_USER_GET_FEATURES) {
>> + PMD_DRV_LOG(ERR, "Unexpected request type (%d)", msg.request);
>> + goto err;
>> + }
>> +
>> + if (msg.size != sizeof(*features)) {
>> + PMD_DRV_LOG(ERR, "Unexpected payload size (%d)", msg.size);
>> + goto err;
>> + }
>> +
>> + *features = msg.payload.u64;
>> +
>> + return 0;
>> +err:
>> + PMD_DRV_LOG(ERR, "Failed to get backend features");
>> +
> On possibly socket-related errors, it's sometimes useful to print
> sterror(errno). WDYT?
>> + return -1;
>> +}
I agree, I will do it in vhost_user_read/vhost_user_write.
Actually I have a already done it in vhost_user_write after earlier
comment from Chenbo.
Thanks,
Maxime
@@ -102,6 +102,8 @@ struct virtio_user_dev;
struct virtio_user_backend_ops {
int (*setup)(struct virtio_user_dev *dev);
int (*set_owner)(struct virtio_user_dev *dev);
+ int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
+ int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
int (*send_request)(struct virtio_user_dev *dev,
enum vhost_user_request req,
void *arg);
@@ -38,6 +38,28 @@ struct vhost_memory_kernel {
#define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
#define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_vring_file)
+/* with below features, vhost kernel does not need to do the checksum and TSO,
+ * these info will be passed to virtio_user through virtio net header.
+ */
+#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
+ ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
+ (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
+ (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
+ (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
+ (1ULL << VIRTIO_NET_F_GUEST_UFO))
+
+/* with below features, when flows from virtio_user to vhost kernel
+ * (1) if flows goes up through the kernel networking stack, it does not need
+ * to verify checksum, which can save CPU cycles;
+ * (2) if flows goes through a Linux bridge and outside from an interface
+ * (kernel driver), checksum and TSO will be done by GSO in kernel or even
+ * offloaded into real physical device.
+ */
+#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
+ ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
+ (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
+ (1ULL << VIRTIO_NET_F_CSUM))
+
static uint64_t max_regions = 64;
static void
@@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
}
+static int
+vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
+{
+ int ret;
+ unsigned int tap_features;
+
+ ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
+ if (ret < 0) {
+ PMD_DRV_LOG(ERR, "Failed to get features");
+ return -1;
+ }
+
+ ret = tap_support_features(&tap_features);
+ if (ret < 0) {
+ PMD_DRV_LOG(ERR, "Failed to get TAP features)");
+ return -1;
+ }
+
+ /* with tap as the backend, all these features are supported
+ * but not claimed by vhost-net, so we add them back when
+ * reporting to upper layer.
+ */
+ if (tap_features & IFF_VNET_HDR) {
+ *features |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
+ *features |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
+ }
+
+ /* vhost_kernel will not declare this feature, but it does
+ * support multi-queue.
+ */
+ if (tap_features & IFF_MULTI_QUEUE)
+ *features |= (1ull << VIRTIO_NET_F_MQ);
+
+ return 0;
+}
+
+static int
+vhost_kernel_set_features(struct virtio_user_dev *dev, uint64_t features)
+{
+ /* We don't need memory protection here */
+ features &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
+ /* VHOST kernel does not know about below flags */
+ features &= ~VHOST_KERNEL_GUEST_OFFLOADS_MASK;
+ features &= ~VHOST_KERNEL_HOST_OFFLOADS_MASK;
+ features &= ~(1ULL << VIRTIO_NET_F_MQ);
+
+ return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_FEATURES, &features);
+}
+
static uint64_t vhost_req_user_to_kernel[] = {
[VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER,
- [VHOST_USER_SET_FEATURES] = VHOST_SET_FEATURES,
- [VHOST_USER_GET_FEATURES] = VHOST_GET_FEATURES,
[VHOST_USER_SET_VRING_CALL] = VHOST_SET_VRING_CALL,
[VHOST_USER_SET_VRING_NUM] = VHOST_SET_VRING_NUM,
[VHOST_USER_SET_VRING_BASE] = VHOST_SET_VRING_BASE,
@@ -150,51 +219,6 @@ prepare_vhost_memory_kernel(void)
return vm;
}
-/* with below features, vhost kernel does not need to do the checksum and TSO,
- * these info will be passed to virtio_user through virtio net header.
- */
-#define VHOST_KERNEL_GUEST_OFFLOADS_MASK \
- ((1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
- (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
- (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
- (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
- (1ULL << VIRTIO_NET_F_GUEST_UFO))
-
-/* with below features, when flows from virtio_user to vhost kernel
- * (1) if flows goes up through the kernel networking stack, it does not need
- * to verify checksum, which can save CPU cycles;
- * (2) if flows goes through a Linux bridge and outside from an interface
- * (kernel driver), checksum and TSO will be done by GSO in kernel or even
- * offloaded into real physical device.
- */
-#define VHOST_KERNEL_HOST_OFFLOADS_MASK \
- ((1ULL << VIRTIO_NET_F_HOST_TSO4) | \
- (1ULL << VIRTIO_NET_F_HOST_TSO6) | \
- (1ULL << VIRTIO_NET_F_CSUM))
-
-static unsigned int
-tap_support_features(void)
-{
- int tapfd;
- unsigned int tap_features;
-
- tapfd = open(PATH_NET_TUN, O_RDWR);
- if (tapfd < 0) {
- PMD_DRV_LOG(ERR, "fail to open %s: %s",
- PATH_NET_TUN, strerror(errno));
- return -1;
- }
-
- if (ioctl(tapfd, TUNGETFEATURES, &tap_features) == -1) {
- PMD_DRV_LOG(ERR, "TUNGETFEATURES failed: %s", strerror(errno));
- close(tapfd);
- return -1;
- }
-
- close(tapfd);
- return tap_features;
-}
-
static int
vhost_kernel_send_request(struct virtio_user_dev *dev,
enum vhost_user_request req,
@@ -206,7 +230,6 @@ vhost_kernel_send_request(struct virtio_user_dev *dev,
struct vhost_memory_kernel *vm = NULL;
int vhostfd;
unsigned int queue_sel;
- unsigned int features;
PMD_DRV_LOG(INFO, "%s", vhost_msg_strings[req]);
@@ -219,17 +242,6 @@ vhost_kernel_send_request(struct virtio_user_dev *dev,
arg = (void *)vm;
}
- if (req_kernel == VHOST_SET_FEATURES) {
- /* We don't need memory protection here */
- *(uint64_t *)arg &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
-
- /* VHOST kernel does not know about below flags */
- *(uint64_t *)arg &= ~VHOST_KERNEL_GUEST_OFFLOADS_MASK;
- *(uint64_t *)arg &= ~VHOST_KERNEL_HOST_OFFLOADS_MASK;
-
- *(uint64_t *)arg &= ~(1ULL << VIRTIO_NET_F_MQ);
- }
-
switch (req_kernel) {
case VHOST_SET_VRING_NUM:
case VHOST_SET_VRING_ADDR:
@@ -259,24 +271,6 @@ vhost_kernel_send_request(struct virtio_user_dev *dev,
ret = ioctl(vhostfd, req_kernel, arg);
}
- if (!ret && req_kernel == VHOST_GET_FEATURES) {
- features = tap_support_features();
- /* with tap as the backend, all these features are supported
- * but not claimed by vhost-net, so we add them back when
- * reporting to upper layer.
- */
- if (features & IFF_VNET_HDR) {
- *((uint64_t *)arg) |= VHOST_KERNEL_GUEST_OFFLOADS_MASK;
- *((uint64_t *)arg) |= VHOST_KERNEL_HOST_OFFLOADS_MASK;
- }
-
- /* vhost_kernel will not declare this feature, but it does
- * support multi-queue.
- */
- if (features & IFF_MULTI_QUEUE)
- *(uint64_t *)arg |= (1ull << VIRTIO_NET_F_MQ);
- }
-
if (vm)
free(vm);
@@ -407,6 +401,8 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
struct virtio_user_backend_ops virtio_ops_kernel = {
.setup = vhost_kernel_setup,
.set_owner = vhost_kernel_set_owner,
+ .get_features = vhost_kernel_get_features,
+ .set_features = vhost_kernel_set_features,
.send_request = vhost_kernel_send_request,
.enable_qp = vhost_kernel_enable_queue_pair
};
@@ -18,6 +18,29 @@
#include "../virtio_logs.h"
#include "../virtio.h"
+
+int
+tap_support_features(unsigned int *tap_features)
+{
+ int tapfd;
+
+ tapfd = open(PATH_NET_TUN, O_RDWR);
+ if (tapfd < 0) {
+ PMD_DRV_LOG(ERR, "fail to open %s: %s",
+ PATH_NET_TUN, strerror(errno));
+ return -1;
+ }
+
+ if (ioctl(tapfd, TUNGETFEATURES, tap_features) == -1) {
+ PMD_DRV_LOG(ERR, "TUNGETFEATURES failed: %s", strerror(errno));
+ close(tapfd);
+ return -1;
+ }
+
+ close(tapfd);
+ return 0;
+}
+
int
vhost_kernel_tap_set_offload(int fd, uint64_t features)
{
@@ -43,5 +43,6 @@ int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
const char *mac, uint64_t features);
int vhost_kernel_tap_set_offload(int fd, uint64_t features);
int vhost_kernel_tap_set_queue(int fd, bool attach);
+int tap_support_features(unsigned int *tap_features);
#endif
@@ -143,6 +143,62 @@ vhost_user_set_owner(struct virtio_user_dev *dev)
return 0;
}
+static int
+vhost_user_get_features(struct virtio_user_dev *dev, uint64_t *features)
+{
+ int ret;
+ struct vhost_user_msg msg = {
+ .request = VHOST_USER_GET_FEATURES,
+ .flags = VHOST_USER_VERSION,
+ };
+
+ ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0);
+ if (ret < 0)
+ goto err;
+
+ ret = vhost_user_read(dev->vhostfd, &msg);
+ if (ret < 0)
+ goto err;
+
+ if (msg.request != VHOST_USER_GET_FEATURES) {
+ PMD_DRV_LOG(ERR, "Unexpected request type (%d)", msg.request);
+ goto err;
+ }
+
+ if (msg.size != sizeof(*features)) {
+ PMD_DRV_LOG(ERR, "Unexpected payload size (%d)", msg.size);
+ goto err;
+ }
+
+ *features = msg.payload.u64;
+
+ return 0;
+err:
+ PMD_DRV_LOG(ERR, "Failed to get backend features");
+
+ return -1;
+}
+
+static int
+vhost_user_set_features(struct virtio_user_dev *dev, uint64_t features)
+{
+ int ret;
+ struct vhost_user_msg msg = {
+ .request = VHOST_USER_SET_FEATURES,
+ .flags = VHOST_USER_VERSION,
+ .size = sizeof(features),
+ .payload.u64 = features,
+ };
+
+ ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0);
+ if (ret < 0) {
+ PMD_DRV_LOG(ERR, "Failed to set features");
+ return -1;
+ }
+
+ return 0;
+}
+
struct walk_arg {
struct vhost_memory *vm;
int *fds;
@@ -249,8 +305,6 @@ static struct vhost_user_msg m;
const char * const vhost_msg_strings[] = {
[VHOST_USER_RESET_OWNER] = "VHOST_RESET_OWNER",
- [VHOST_USER_SET_FEATURES] = "VHOST_SET_FEATURES",
- [VHOST_USER_GET_FEATURES] = "VHOST_GET_FEATURES",
[VHOST_USER_SET_VRING_CALL] = "VHOST_SET_VRING_CALL",
[VHOST_USER_SET_VRING_NUM] = "VHOST_SET_VRING_NUM",
[VHOST_USER_SET_VRING_BASE] = "VHOST_SET_VRING_BASE",
@@ -299,7 +353,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
return -ENOTSUP;
/* Fallthrough */
- case VHOST_USER_GET_FEATURES:
case VHOST_USER_GET_PROTOCOL_FEATURES:
need_reply = 1;
break;
@@ -313,7 +366,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
if (has_reply_ack)
msg.flags |= VHOST_USER_NEED_REPLY_MASK;
/* Fallthrough */
- case VHOST_USER_SET_FEATURES:
case VHOST_USER_SET_PROTOCOL_FEATURES:
case VHOST_USER_SET_LOG_BASE:
msg.payload.u64 = *((__u64 *)arg);
@@ -393,7 +445,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
}
switch (req) {
- case VHOST_USER_GET_FEATURES:
case VHOST_USER_GET_STATUS:
case VHOST_USER_GET_PROTOCOL_FEATURES:
if (msg.size != sizeof(m.payload.u64)) {
@@ -531,6 +582,8 @@ vhost_user_enable_queue_pair(struct virtio_user_dev *dev,
struct virtio_user_backend_ops virtio_ops_user = {
.setup = vhost_user_setup,
.set_owner = vhost_user_set_owner,
+ .get_features = vhost_user_get_features,
+ .set_features = vhost_user_set_features,
.send_request = vhost_user_sock,
.enable_qp = vhost_user_enable_queue_pair
};
@@ -38,8 +38,6 @@
static uint64_t vhost_req_user_to_vdpa[] = {
[VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER,
- [VHOST_USER_SET_FEATURES] = VHOST_SET_FEATURES,
- [VHOST_USER_GET_FEATURES] = VHOST_GET_FEATURES,
[VHOST_USER_SET_VRING_CALL] = VHOST_SET_VRING_CALL,
[VHOST_USER_SET_VRING_NUM] = VHOST_SET_VRING_NUM,
[VHOST_USER_SET_VRING_BASE] = VHOST_SET_VRING_BASE,
@@ -101,6 +99,32 @@ vhost_vdpa_set_owner(struct virtio_user_dev *dev)
return vhost_vdpa_ioctl(dev->vhostfd, VHOST_SET_OWNER, NULL);
}
+static int
+vhost_vdpa_get_features(struct virtio_user_dev *dev, uint64_t *features)
+{
+ int ret;
+
+ ret = vhost_vdpa_ioctl(dev->vhostfd, VHOST_GET_FEATURES, features);
+ if (ret) {
+ PMD_DRV_LOG(ERR, "Failed to get features");
+ return -1;
+ }
+
+ /* Multiqueue not supported for now */
+ *features &= ~(1ULL << VIRTIO_NET_F_MQ);
+
+ return 0;
+}
+
+static int
+vhost_vdpa_set_features(struct virtio_user_dev *dev, uint64_t features)
+{
+ /* WORKAROUND */
+ features |= 1ULL << VIRTIO_F_IOMMU_PLATFORM;
+
+ return vhost_vdpa_ioctl(dev->vhostfd, VHOST_SET_FEATURES, &features);
+}
+
static int
vhost_vdpa_dma_map(struct virtio_user_dev *dev, void *addr,
uint64_t iova, size_t len)
@@ -227,14 +251,6 @@ vhost_vdpa_send_request(struct virtio_user_dev *dev,
if (req_vdpa == VHOST_SET_MEM_TABLE)
return vhost_vdpa_dma_map_all(dev);
- if (req_vdpa == VHOST_SET_FEATURES) {
- /* WORKAROUND */
- *(uint64_t *)arg |= 1ULL << VIRTIO_F_IOMMU_PLATFORM;
-
- /* Multiqueue not supported for now */
- *(uint64_t *)arg &= ~(1ULL << VIRTIO_NET_F_MQ);
- }
-
switch (req_vdpa) {
case VHOST_SET_VRING_NUM:
case VHOST_SET_VRING_ADDR:
@@ -313,6 +329,8 @@ vhost_vdpa_enable_queue_pair(struct virtio_user_dev *dev,
struct virtio_user_backend_ops virtio_ops_vdpa = {
.setup = vhost_vdpa_setup,
.set_owner = vhost_vdpa_set_owner,
+ .get_features = vhost_vdpa_get_features,
+ .set_features = vhost_vdpa_set_features,
.send_request = vhost_vdpa_send_request,
.enable_qp = vhost_vdpa_enable_queue_pair,
.dma_map = vhost_vdpa_dma_map,
@@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
/* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
features &= ~(1ull << VIRTIO_NET_F_STATUS);
- ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
+ ret = dev->ops->set_features(dev, features);
if (ret < 0)
goto error;
PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
@@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
return -1;
}
- if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
- &dev->device_features) < 0) {
+ if (dev->ops->get_features(dev, &dev->device_features) < 0) {
PMD_INIT_LOG(ERR, "get_features failed: %s",
strerror(errno));
return -1;
@@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
- if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
- &dev->device_features) < 0) {
+ if (dev->ops->get_features(dev, &dev->device_features) < 0) {
PMD_INIT_LOG(ERR, "get_features failed: %s",
strerror(errno));
return -1;