| Message ID | 20201125152120.183691-2-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | Vhost-vDPA fixes | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, November 25, 2020 11:21 PM > To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; > jasowang@redhat.com; david.marchand@redhat.com > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org > Subject: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features > negotiation > > This patch adds missing backend features negotiation for > in Vhost-vDPA. Without it, IOTLB messages v2 could be sent > by Virtio-user PMD while not supported by the backend. > > Fixes: 6b901437056e ("net/virtio: introduce vhost-vDPA backend") > Cc: stable@dpdk.org > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > drivers/net/virtio/virtio_user/vhost.h | 4 ++++ > drivers/net/virtio/virtio_user/vhost_vdpa.c | 14 ++++++++++++++ > drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 ++++++++++---- > 3 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/virtio/virtio_user/vhost.h > b/drivers/net/virtio/virtio_user/vhost.h > index 210a3704e7..c1dcc50b58 100644 > --- a/drivers/net/virtio/virtio_user/vhost.h > +++ b/drivers/net/virtio/virtio_user/vhost.h > @@ -86,6 +86,10 @@ enum vhost_user_request { > VHOST_USER_MAX > }; > > +#ifndef VHOST_BACKEND_F_IOTLB_MSG_V2 > +#define VHOST_BACKEND_F_IOTLB_MSG_V2 1 > +#endif > + > extern const char * const vhost_msg_strings[VHOST_USER_MAX]; > > struct vhost_memory_region { > diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c > b/drivers/net/virtio/virtio_user/vhost_vdpa.c > index c7b9349fc8..b6c81d6f17 100644 > --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c > +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c > @@ -35,6 +35,8 @@ > #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) > #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \ > struct vhost_vring_state) > +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) > +#define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) > > static uint64_t vhost_req_user_to_vdpa[] = { > [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER, > @@ -51,6 +53,8 @@ static uint64_t vhost_req_user_to_vdpa[] = { > [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, > [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, > [VHOST_USER_SET_VRING_ENABLE] = VHOST_VDPA_SET_VRING_ENABLE, > + [VHOST_USER_GET_PROTOCOL_FEATURES] = VHOST_GET_BACKEND_FEATURES, > + [VHOST_USER_SET_PROTOCOL_FEATURES] = VHOST_SET_BACKEND_FEATURES, > }; > > /* no alignment requirement */ > @@ -86,6 +90,11 @@ vhost_vdpa_dma_map(struct virtio_user_dev *dev, void *addr, > { > struct vhost_msg msg = {}; > > + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) > { > + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); > + return -1; > + } > + > msg.type = VHOST_IOTLB_MSG_V2; > msg.iotlb.type = VHOST_IOTLB_UPDATE; > msg.iotlb.iova = iova; > @@ -108,6 +117,11 @@ vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, > __rte_unused void *addr, > { > struct vhost_msg msg = {}; > > + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) > { > + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); > + return -1; > + } > + > msg.type = VHOST_IOTLB_MSG_V2; > msg.iotlb.type = VHOST_IOTLB_INVALIDATE; > msg.iotlb.iova = iova; > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > b/drivers/net/virtio/virtio_user/virtio_user_dev.c > index 053f0267ca..96bc6b232d 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > @@ -439,11 +439,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) > 1ULL << VIRTIO_F_RING_PACKED | \ > 1ULL << VHOST_USER_F_PROTOCOL_FEATURES) > > -#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ > +#define VHOST_USER_SUPPORTED_PROTOCOL_FEATURES \ > (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ > 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ > 1ULL << VHOST_USER_PROTOCOL_F_STATUS) > > +#define VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES \ > + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) > int > virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > int cq, int queue_size, const char *mac, char **ifname, > @@ -462,9 +464,13 @@ 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; > dev->backend_type = backend_type; > > + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) > + dev->protocol_features = VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; > + else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) > + dev->protocol_features = VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES; > + > parse_mac(dev, mac); > > if (*ifname) { > @@ -497,8 +503,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char > *path, int queues, > } > > > - if (dev->device_features & > - (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) { > + if ((dev->device_features & (1ULL << > VHOST_USER_F_PROTOCOL_FEATURES) || > + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) Do you mean: if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) { here? Besides, I think it's better to update here as vhost-vdpa also uses protocol_features. (http://code.dpdk.org/dpdk/v20.08/source/drivers/net/virtio/virtio_user/virtio_user_dev.h#L44) Thanks! Chenbo > { > if (dev->ops->send_request(dev, > VHOST_USER_GET_PROTOCOL_FEATURES, > &protocol_features)) > -- > 2.26.2
On 12/21/20 7:23 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Wednesday, November 25, 2020 11:21 PM >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; >> jasowang@redhat.com; david.marchand@redhat.com >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org >> Subject: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features >> negotiation >> >> This patch adds missing backend features negotiation for >> in Vhost-vDPA. Without it, IOTLB messages v2 could be sent >> by Virtio-user PMD while not supported by the backend. >> >> Fixes: 6b901437056e ("net/virtio: introduce vhost-vDPA backend") >> Cc: stable@dpdk.org >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> drivers/net/virtio/virtio_user/vhost.h | 4 ++++ >> drivers/net/virtio/virtio_user/vhost_vdpa.c | 14 ++++++++++++++ >> drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 ++++++++++---- >> 3 files changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_user/vhost.h >> b/drivers/net/virtio/virtio_user/vhost.h >> index 210a3704e7..c1dcc50b58 100644 >> --- a/drivers/net/virtio/virtio_user/vhost.h >> +++ b/drivers/net/virtio/virtio_user/vhost.h >> @@ -86,6 +86,10 @@ enum vhost_user_request { >> VHOST_USER_MAX >> }; >> >> +#ifndef VHOST_BACKEND_F_IOTLB_MSG_V2 >> +#define VHOST_BACKEND_F_IOTLB_MSG_V2 1 >> +#endif >> + >> extern const char * const vhost_msg_strings[VHOST_USER_MAX]; >> >> struct vhost_memory_region { >> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c >> b/drivers/net/virtio/virtio_user/vhost_vdpa.c >> index c7b9349fc8..b6c81d6f17 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c >> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c >> @@ -35,6 +35,8 @@ >> #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) >> #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \ >> struct vhost_vring_state) >> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) >> +#define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) >> >> static uint64_t vhost_req_user_to_vdpa[] = { >> [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER, >> @@ -51,6 +53,8 @@ static uint64_t vhost_req_user_to_vdpa[] = { >> [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, >> [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, >> [VHOST_USER_SET_VRING_ENABLE] = VHOST_VDPA_SET_VRING_ENABLE, >> + [VHOST_USER_GET_PROTOCOL_FEATURES] = VHOST_GET_BACKEND_FEATURES, >> + [VHOST_USER_SET_PROTOCOL_FEATURES] = VHOST_SET_BACKEND_FEATURES, >> }; >> >> /* no alignment requirement */ >> @@ -86,6 +90,11 @@ vhost_vdpa_dma_map(struct virtio_user_dev *dev, void *addr, >> { >> struct vhost_msg msg = {}; >> >> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) >> { >> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); >> + return -1; >> + } >> + >> msg.type = VHOST_IOTLB_MSG_V2; >> msg.iotlb.type = VHOST_IOTLB_UPDATE; >> msg.iotlb.iova = iova; >> @@ -108,6 +117,11 @@ vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, >> __rte_unused void *addr, >> { >> struct vhost_msg msg = {}; >> >> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) >> { >> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); >> + return -1; >> + } >> + >> msg.type = VHOST_IOTLB_MSG_V2; >> msg.iotlb.type = VHOST_IOTLB_INVALIDATE; >> msg.iotlb.iova = iova; >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index 053f0267ca..96bc6b232d 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -439,11 +439,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) >> 1ULL << VIRTIO_F_RING_PACKED | \ >> 1ULL << VHOST_USER_F_PROTOCOL_FEATURES) >> >> -#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ >> +#define VHOST_USER_SUPPORTED_PROTOCOL_FEATURES \ >> (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ >> 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ >> 1ULL << VHOST_USER_PROTOCOL_F_STATUS) >> >> +#define VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES \ >> + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) >> int >> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, >> int cq, int queue_size, const char *mac, char **ifname, >> @@ -462,9 +464,13 @@ 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; >> dev->backend_type = backend_type; >> >> + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) >> + dev->protocol_features = VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; >> + else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) >> + dev->protocol_features = VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES; >> + >> parse_mac(dev, mac); >> >> if (*ifname) { >> @@ -497,8 +503,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char >> *path, int queues, >> } >> >> >> - if (dev->device_features & >> - (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) { >> + if ((dev->device_features & (1ULL << >> VHOST_USER_F_PROTOCOL_FEATURES) || >> + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) > > Do you mean: > > if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || > (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) { > > here? Indeed! > Besides, I think it's better to update here as vhost-vdpa also uses protocol_features. > (http://code.dpdk.org/dpdk/v20.08/source/drivers/net/virtio/virtio_user/virtio_user_dev.h#L44) Not sure what you mean here? Can you please elaborate? Thanks! Maxime > Thanks! > Chenbo > >> { >> if (dev->ops->send_request(dev, >> VHOST_USER_GET_PROTOCOL_FEATURES, >> &protocol_features)) >> -- >> 2.26.2 >
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Tuesday, December 22, 2020 6:56 PM > To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; amorenoz@redhat.com; > jasowang@redhat.com; david.marchand@redhat.com > Cc: stable@dpdk.org > Subject: Re: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features > negotiation > > > > On 12/21/20 7:23 AM, Xia, Chenbo wrote: > > Hi Maxime, > > > >> -----Original Message----- > >> From: Maxime Coquelin <maxime.coquelin@redhat.com> > >> Sent: Wednesday, November 25, 2020 11:21 PM > >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; > >> jasowang@redhat.com; david.marchand@redhat.com > >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org > >> Subject: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features > >> negotiation > >> > >> This patch adds missing backend features negotiation for > >> in Vhost-vDPA. Without it, IOTLB messages v2 could be sent > >> by Virtio-user PMD while not supported by the backend. > >> > >> Fixes: 6b901437056e ("net/virtio: introduce vhost-vDPA backend") > >> Cc: stable@dpdk.org > >> > >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >> --- > >> drivers/net/virtio/virtio_user/vhost.h | 4 ++++ > >> drivers/net/virtio/virtio_user/vhost_vdpa.c | 14 ++++++++++++++ > >> drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 ++++++++++---- > >> 3 files changed, 28 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/net/virtio/virtio_user/vhost.h > >> b/drivers/net/virtio/virtio_user/vhost.h > >> index 210a3704e7..c1dcc50b58 100644 > >> --- a/drivers/net/virtio/virtio_user/vhost.h > >> +++ b/drivers/net/virtio/virtio_user/vhost.h > >> @@ -86,6 +86,10 @@ enum vhost_user_request { > >> VHOST_USER_MAX > >> }; > >> > >> +#ifndef VHOST_BACKEND_F_IOTLB_MSG_V2 > >> +#define VHOST_BACKEND_F_IOTLB_MSG_V2 1 > >> +#endif > >> + > >> extern const char * const vhost_msg_strings[VHOST_USER_MAX]; > >> > >> struct vhost_memory_region { > >> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c > >> b/drivers/net/virtio/virtio_user/vhost_vdpa.c > >> index c7b9349fc8..b6c81d6f17 100644 > >> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c > >> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c > >> @@ -35,6 +35,8 @@ > >> #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) > >> #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \ > >> struct vhost_vring_state) > >> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) > >> +#define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) > >> > >> static uint64_t vhost_req_user_to_vdpa[] = { > >> [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER, > >> @@ -51,6 +53,8 @@ static uint64_t vhost_req_user_to_vdpa[] = { > >> [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, > >> [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, > >> [VHOST_USER_SET_VRING_ENABLE] = VHOST_VDPA_SET_VRING_ENABLE, > >> + [VHOST_USER_GET_PROTOCOL_FEATURES] = VHOST_GET_BACKEND_FEATURES, > >> + [VHOST_USER_SET_PROTOCOL_FEATURES] = VHOST_SET_BACKEND_FEATURES, > >> }; > >> > >> /* no alignment requirement */ > >> @@ -86,6 +90,11 @@ vhost_vdpa_dma_map(struct virtio_user_dev *dev, void > *addr, > >> { > >> struct vhost_msg msg = {}; > >> > >> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) > >> { > >> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); > >> + return -1; > >> + } > >> + > >> msg.type = VHOST_IOTLB_MSG_V2; > >> msg.iotlb.type = VHOST_IOTLB_UPDATE; > >> msg.iotlb.iova = iova; > >> @@ -108,6 +117,11 @@ vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, > >> __rte_unused void *addr, > >> { > >> struct vhost_msg msg = {}; > >> > >> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) > >> { > >> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); > >> + return -1; > >> + } > >> + > >> msg.type = VHOST_IOTLB_MSG_V2; > >> msg.iotlb.type = VHOST_IOTLB_INVALIDATE; > >> msg.iotlb.iova = iova; > >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> index 053f0267ca..96bc6b232d 100644 > >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> @@ -439,11 +439,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) > >> 1ULL << VIRTIO_F_RING_PACKED | \ > >> 1ULL << VHOST_USER_F_PROTOCOL_FEATURES) > >> > >> -#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ > >> +#define VHOST_USER_SUPPORTED_PROTOCOL_FEATURES \ > >> (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ > >> 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ > >> 1ULL << VHOST_USER_PROTOCOL_F_STATUS) > >> > >> +#define VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES \ > >> + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) > >> int > >> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > >> int cq, int queue_size, const char *mac, char **ifname, > >> @@ -462,9 +464,13 @@ 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; > >> dev->backend_type = backend_type; > >> > >> + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) > >> + dev->protocol_features = VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; > >> + else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) > >> + dev->protocol_features = VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES; > >> + > >> parse_mac(dev, mac); > >> > >> if (*ifname) { > >> @@ -497,8 +503,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char > >> *path, int queues, > >> } > >> > >> > >> - if (dev->device_features & > >> - (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) { > >> + if ((dev->device_features & (1ULL << > >> VHOST_USER_F_PROTOCOL_FEATURES) || > >> + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) > > > > Do you mean: > > > > if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || > > (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) > { > > > > here? > > Indeed! > > > Besides, I think it's better to update here as vhost-vdpa also uses > protocol_features. > > > (http://code.dpdk.org/dpdk/v20.08/source/drivers/net/virtio/virtio_user/virtio > _user_dev.h#L44) > > Not sure what you mean here? Can you please elaborate? Sorry, I'm not clear on this. It's just a small point about code comment. In http://code.dpdk.org/dpdk/v20.11/source/drivers/net/virtio/virtio_user/virtio_user_dev.h#L52, It says 'negotiated protocol features(vhost-user only)'. Since it's also used by vhost-vdpa now, maybe it's better to improve it. What do you think? Thanks! Chenbo > > Thanks! > Maxime > > > Thanks! > > Chenbo > > > >> { > >> if (dev->ops->send_request(dev, > >> VHOST_USER_GET_PROTOCOL_FEATURES, > >> &protocol_features)) > >> -- > >> 2.26.2 > >
On 12/23/20 6:37 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Tuesday, December 22, 2020 6:56 PM >> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; amorenoz@redhat.com; >> jasowang@redhat.com; david.marchand@redhat.com >> Cc: stable@dpdk.org >> Subject: Re: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features >> negotiation >> >> >> >> On 12/21/20 7:23 AM, Xia, Chenbo wrote: >>> Hi Maxime, >>> >>>> -----Original Message----- >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> Sent: Wednesday, November 25, 2020 11:21 PM >>>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; >>>> jasowang@redhat.com; david.marchand@redhat.com >>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org >>>> Subject: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features >>>> negotiation >>>> >>>> This patch adds missing backend features negotiation for >>>> in Vhost-vDPA. Without it, IOTLB messages v2 could be sent >>>> by Virtio-user PMD while not supported by the backend. >>>> >>>> Fixes: 6b901437056e ("net/virtio: introduce vhost-vDPA backend") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> --- >>>> drivers/net/virtio/virtio_user/vhost.h | 4 ++++ >>>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 14 ++++++++++++++ >>>> drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 ++++++++++---- >>>> 3 files changed, 28 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h >>>> b/drivers/net/virtio/virtio_user/vhost.h >>>> index 210a3704e7..c1dcc50b58 100644 >>>> --- a/drivers/net/virtio/virtio_user/vhost.h >>>> +++ b/drivers/net/virtio/virtio_user/vhost.h >>>> @@ -86,6 +86,10 @@ enum vhost_user_request { >>>> VHOST_USER_MAX >>>> }; >>>> >>>> +#ifndef VHOST_BACKEND_F_IOTLB_MSG_V2 >>>> +#define VHOST_BACKEND_F_IOTLB_MSG_V2 1 >>>> +#endif >>>> + >>>> extern const char * const vhost_msg_strings[VHOST_USER_MAX]; >>>> >>>> struct vhost_memory_region { >>>> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> b/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> index c7b9349fc8..b6c81d6f17 100644 >>>> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> @@ -35,6 +35,8 @@ >>>> #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) >>>> #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \ >>>> struct vhost_vring_state) >>>> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) >>>> +#define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) >>>> >>>> static uint64_t vhost_req_user_to_vdpa[] = { >>>> [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER, >>>> @@ -51,6 +53,8 @@ static uint64_t vhost_req_user_to_vdpa[] = { >>>> [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, >>>> [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, >>>> [VHOST_USER_SET_VRING_ENABLE] = VHOST_VDPA_SET_VRING_ENABLE, >>>> + [VHOST_USER_GET_PROTOCOL_FEATURES] = VHOST_GET_BACKEND_FEATURES, >>>> + [VHOST_USER_SET_PROTOCOL_FEATURES] = VHOST_SET_BACKEND_FEATURES, >>>> }; >>>> >>>> /* no alignment requirement */ >>>> @@ -86,6 +90,11 @@ vhost_vdpa_dma_map(struct virtio_user_dev *dev, void >> *addr, >>>> { >>>> struct vhost_msg msg = {}; >>>> >>>> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) >>>> { >>>> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); >>>> + return -1; >>>> + } >>>> + >>>> msg.type = VHOST_IOTLB_MSG_V2; >>>> msg.iotlb.type = VHOST_IOTLB_UPDATE; >>>> msg.iotlb.iova = iova; >>>> @@ -108,6 +117,11 @@ vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, >>>> __rte_unused void *addr, >>>> { >>>> struct vhost_msg msg = {}; >>>> >>>> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) >>>> { >>>> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); >>>> + return -1; >>>> + } >>>> + >>>> msg.type = VHOST_IOTLB_MSG_V2; >>>> msg.iotlb.type = VHOST_IOTLB_INVALIDATE; >>>> msg.iotlb.iova = iova; >>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> index 053f0267ca..96bc6b232d 100644 >>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> @@ -439,11 +439,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) >>>> 1ULL << VIRTIO_F_RING_PACKED | \ >>>> 1ULL << VHOST_USER_F_PROTOCOL_FEATURES) >>>> >>>> -#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ >>>> +#define VHOST_USER_SUPPORTED_PROTOCOL_FEATURES \ >>>> (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ >>>> 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ >>>> 1ULL << VHOST_USER_PROTOCOL_F_STATUS) >>>> >>>> +#define VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES \ >>>> + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) >>>> int >>>> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, >>>> int cq, int queue_size, const char *mac, char **ifname, >>>> @@ -462,9 +464,13 @@ 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; >>>> dev->backend_type = backend_type; >>>> >>>> + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) >>>> + dev->protocol_features = VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; >>>> + else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) >>>> + dev->protocol_features = VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES; >>>> + >>>> parse_mac(dev, mac); >>>> >>>> if (*ifname) { >>>> @@ -497,8 +503,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char >>>> *path, int queues, >>>> } >>>> >>>> >>>> - if (dev->device_features & >>>> - (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) { >>>> + if ((dev->device_features & (1ULL << >>>> VHOST_USER_F_PROTOCOL_FEATURES) || >>>> + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) >>> >>> Do you mean: >>> >>> if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || >>> (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) >> { >>> >>> here? >> >> Indeed! >> >>> Besides, I think it's better to update here as vhost-vdpa also uses >> protocol_features. >>> >> (http://code.dpdk.org/dpdk/v20.08/source/drivers/net/virtio/virtio_user/virtio >> _user_dev.h#L44) >> >> Not sure what you mean here? Can you please elaborate? > > Sorry, I'm not clear on this. It's just a small point about code comment. > > In http://code.dpdk.org/dpdk/v20.11/source/drivers/net/virtio/virtio_user/virtio_user_dev.h#L52, > It says 'negotiated protocol features(vhost-user only)'. Since it's also used by vhost-vdpa now, > maybe it's better to improve it. What do you think? I think it makes sense! Note that in my Virtio PMD rework series, this field disappears, meaning that once this series applied, I will need to rebase my rework on top of it so that vhost-vdpa backend features is moved into vhost_vdpa.c. In the mean time, I will remove the "(vhost-user only)" comment. Thanks, Maxime > Thanks! > Chenbo > >> >> Thanks! >> Maxime >> >>> Thanks! >>> Chenbo >>> >>>> { >>>> if (dev->ops->send_request(dev, >>>> VHOST_USER_GET_PROTOCOL_FEATURES, >>>> &protocol_features)) >>>> -- >>>> 2.26.2 >>> >
diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h index 210a3704e7..c1dcc50b58 100644 --- a/drivers/net/virtio/virtio_user/vhost.h +++ b/drivers/net/virtio/virtio_user/vhost.h @@ -86,6 +86,10 @@ enum vhost_user_request { VHOST_USER_MAX }; +#ifndef VHOST_BACKEND_F_IOTLB_MSG_V2 +#define VHOST_BACKEND_F_IOTLB_MSG_V2 1 +#endif + extern const char * const vhost_msg_strings[VHOST_USER_MAX]; struct vhost_memory_region { diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c b/drivers/net/virtio/virtio_user/vhost_vdpa.c index c7b9349fc8..b6c81d6f17 100644 --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c @@ -35,6 +35,8 @@ #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \ struct vhost_vring_state) +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) +#define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) static uint64_t vhost_req_user_to_vdpa[] = { [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER, @@ -51,6 +53,8 @@ static uint64_t vhost_req_user_to_vdpa[] = { [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, [VHOST_USER_SET_VRING_ENABLE] = VHOST_VDPA_SET_VRING_ENABLE, + [VHOST_USER_GET_PROTOCOL_FEATURES] = VHOST_GET_BACKEND_FEATURES, + [VHOST_USER_SET_PROTOCOL_FEATURES] = VHOST_SET_BACKEND_FEATURES, }; /* no alignment requirement */ @@ -86,6 +90,11 @@ vhost_vdpa_dma_map(struct virtio_user_dev *dev, void *addr, { struct vhost_msg msg = {}; + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) { + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); + return -1; + } + msg.type = VHOST_IOTLB_MSG_V2; msg.iotlb.type = VHOST_IOTLB_UPDATE; msg.iotlb.iova = iova; @@ -108,6 +117,11 @@ vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, __rte_unused void *addr, { struct vhost_msg msg = {}; + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) { + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); + return -1; + } + msg.type = VHOST_IOTLB_MSG_V2; msg.iotlb.type = VHOST_IOTLB_INVALIDATE; msg.iotlb.iova = iova; diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 053f0267ca..96bc6b232d 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -439,11 +439,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) 1ULL << VIRTIO_F_RING_PACKED | \ 1ULL << VHOST_USER_F_PROTOCOL_FEATURES) -#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ +#define VHOST_USER_SUPPORTED_PROTOCOL_FEATURES \ (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ 1ULL << VHOST_USER_PROTOCOL_F_STATUS) +#define VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES \ + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, int cq, int queue_size, const char *mac, char **ifname, @@ -462,9 +464,13 @@ 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; dev->backend_type = backend_type; + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) + dev->protocol_features = VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; + else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) + dev->protocol_features = VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES; + parse_mac(dev, mac); if (*ifname) { @@ -497,8 +503,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, } - if (dev->device_features & - (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) { + if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES) || + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) { if (dev->ops->send_request(dev, VHOST_USER_GET_PROTOCOL_FEATURES, &protocol_features))
This patch adds missing backend features negotiation for in Vhost-vDPA. Without it, IOTLB messages v2 could be sent by Virtio-user PMD while not supported by the backend. Fixes: 6b901437056e ("net/virtio: introduce vhost-vDPA backend") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- drivers/net/virtio/virtio_user/vhost.h | 4 ++++ drivers/net/virtio/virtio_user/vhost_vdpa.c | 14 ++++++++++++++ drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 ++++++++++---- 3 files changed, 28 insertions(+), 4 deletions(-)