Hi Adrian, > -----Original Message----- > From: Adrian Moreno <amore...@redhat.com> > Sent: Thursday, July 16, 2020 3:43 PM > To: Xia, Chenbo <chenbo....@intel.com>; dev@dpdk.org > Cc: maxime.coque...@redhat.com; Wang, Zhihong <zhihong.w...@intel.com> > Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio- > user > > > > On 7/16/20 5:15 AM, Xia, Chenbo wrote: > > Hi Adrian, > > > >> -----Original Message----- > >> From: Adrian Moreno <amore...@redhat.com> > >> Sent: Thursday, July 16, 2020 1:18 AM > >> To: dev@dpdk.org > >> Cc: maxime.coque...@redhat.com; Wang, Zhihong > >> <zhihong.w...@intel.com>; amore...@redhat.com; Xia, Chenbo > >> <chenbo....@intel.com> > >> Subject: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to > >> Virtio-user > >> > >> From: Maxime Coquelin <maxime.coque...@redhat.com> > >> > >> This patch adds support for VHOST_USER_SET_STATUS request. It is used > >> to make the backend aware of Virtio devices status update. > >> > >> It is useful for the backend to know when the Virtio driver is done > >> with the Virtio device configuration. > >> > >> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com> > >> Signed-off-by: Adrian Moreno <amore...@redhat.com> > >> --- > >> drivers/net/virtio/virtio_user/vhost.h | 6 +++++ > >> drivers/net/virtio/virtio_user/vhost_user.c | 10 ++++++++ > >> .../net/virtio/virtio_user/virtio_user_dev.c | 23 ++++++++++++++++++- > >> .../net/virtio/virtio_user/virtio_user_dev.h | 1 + > >> drivers/net/virtio/virtio_user_ethdev.c | 1 + > >> 5 files changed, 40 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/virtio/virtio_user/vhost.h > >> b/drivers/net/virtio/virtio_user/vhost.h > >> index 260e1c308..8f49ef457 100644 > >> --- a/drivers/net/virtio/virtio_user/vhost.h > >> +++ b/drivers/net/virtio/virtio_user/vhost.h > >> @@ -57,6 +57,10 @@ struct vhost_vring_addr { #define > >> VHOST_USER_PROTOCOL_F_REPLY_ACK 3 #endif > >> > >> +#ifndef VHOST_USER_PROTOCOL_F_STATUS #define > >> +VHOST_USER_PROTOCOL_F_STATUS 16 #endif > >> + > >> enum vhost_user_request { > >> VHOST_USER_NONE = 0, > >> VHOST_USER_GET_FEATURES = 1, > >> @@ -77,6 +81,8 @@ enum vhost_user_request { > >> VHOST_USER_SET_PROTOCOL_FEATURES = 16, > >> VHOST_USER_GET_QUEUE_NUM = 17, > >> VHOST_USER_SET_VRING_ENABLE = 18, > >> + VHOST_USER_SET_STATUS = 39, > >> + VHOST_USER_GET_STATUS = 40, > >> VHOST_USER_MAX > >> }; > >> > >> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c > >> b/drivers/net/virtio/virtio_user/vhost_user.c > >> index 631d0e353..2332e01d1 100644 > >> --- a/drivers/net/virtio/virtio_user/vhost_user.c > >> +++ b/drivers/net/virtio/virtio_user/vhost_user.c > >> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = { > >> [VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE", > >> [VHOST_USER_GET_PROTOCOL_FEATURES] = > >> "VHOST_USER_GET_PROTOCOL_FEATURES", > >> [VHOST_USER_SET_PROTOCOL_FEATURES] = > >> "VHOST_USER_SET_PROTOCOL_FEATURES", > >> + [VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS", > >> + [VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS", > >> }; > >> > >> static int > >> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev, > >> need_reply = 1; > >> break; > >> > >> + case VHOST_USER_SET_STATUS: > >> + if (!(dev->protocol_features & > >> + (1ULL << > >> VHOST_USER_PROTOCOL_F_STATUS))) > >> + return 0; > >> + > >> + if (has_reply_ack) > >> + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > >> + /* Fallthrough */ > > > > There's a coding style issue here: > > WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough > comment. > > Could you fix this? > > > "fallthrough" is not defined. I think this macro is defined in the linux > kernel > (where checkpatches.pl comes from?). We could define the same macro that > would depend in compiler support for __falthrough__ attribute. > > In any case, this is not the only case: > > $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l > 62 > > So maybe this warning is new? > Should I change all of them together in another patch?
checkpatches.pl is in linux kernel. I think it prefers 'fallthrough' rather than 'Fallthrough'. I think it's a linux style, right? Thanks! Chenbo > > >> case VHOST_USER_SET_FEATURES: > >> case VHOST_USER_SET_PROTOCOL_FEATURES: > >> case VHOST_USER_SET_LOG_BASE: > >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> index 0a6991bcc..2c400a448 100644 > >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev > >> *dev) > >> > >> #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ > >> (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ > >> - 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK) > >> + 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ > >> + 1ULL << VHOST_USER_PROTOCOL_F_STATUS) > >> > >> int > >> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int > >> queues, @@ - > >> 783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, > >> uint16_t > >> queue_idx) > >> __atomic_add_fetch(&vring->used->idx, 1, > __ATOMIC_RELAXED); > >> } > >> } > >> + > >> +int > >> +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t > >> +status) { > >> + int ret; > >> + uint64_t arg = status; > >> + > >> + /* Vhost-user only for now */ > >> + if (!is_vhost_user_by_type(dev->path)) > >> + return 0; > >> + > >> + if (!(dev->protocol_features & (1ULL << > >> VHOST_USER_PROTOCOL_F_STATUS))) > >> + return 0; > >> + > >> + ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg); > >> + if (ret) > >> + return -1; > > > > Do you think we should add a log here to show if SET_STAUTS failed? > > > Good idea! Will do in the next version. Thanks > > > Thanks! > > Chenbo > > > >> + > >> + return 0; > >> +} > >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h > >> b/drivers/net/virtio/virtio_user/virtio_user_dev.h > >> index 10b274d19..a76d7cfaf 100644 > >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h > >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h > >> @@ -74,4 +74,5 @@ void virtio_user_handle_cq(struct virtio_user_dev > >> *dev, uint16_t queue_idx); void virtio_user_handle_cq_packed(struct > >> virtio_user_dev *dev, > >> uint16_t queue_idx); > >> uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t > >> q_pairs); > >> +int virtio_user_send_status_update(struct virtio_user_dev *dev, > >> +uint8_t status); > >> #endif > >> diff --git a/drivers/net/virtio/virtio_user_ethdev.c > >> b/drivers/net/virtio/virtio_user_ethdev.c > >> index 5b06d8e89..e52f11341 100644 > >> --- a/drivers/net/virtio/virtio_user_ethdev.c > >> +++ b/drivers/net/virtio/virtio_user_ethdev.c > >> @@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw, > >> uint8_t > >> status) > >> else if (status == VIRTIO_CONFIG_STATUS_RESET) > >> virtio_user_reset(hw); > >> dev->status = status; > >> + virtio_user_send_status_update(dev, status); > >> } > >> > >> static uint8_t > >> -- > >> 2.26.2 > > > > -- > Adrián Moreno