From: "Liu, Changpeng" <changpeng.liu@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
"Kulasek, TomaszX" <tomaszx.kulasek@intel.com>,
"yliu@fridaylinux.org" <yliu@fridaylinux.org>
Cc: "Verkamp, Daniel" <daniel.verkamp@intel.com>,
"Harris, James R" <james.r.harris@intel.com>,
"Wodkowski, PawelX" <pawelx.wodkowski@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Tan, Jianfeng" <jianfeng.tan@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space messages
Date: Wed, 28 Mar 2018 09:50:20 +0000 [thread overview]
Message-ID: <FF7FC980937D6342B9D289F5F3C7C2625B6252DD@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <6c556086-bccc-1e55-d490-c21bcc8a6c4b@redhat.com>
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, March 28, 2018 5:12 PM
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; yliu@fridaylinux.org
> Cc: Verkamp, Daniel <daniel.verkamp@intel.com>; Harris, James R
> <james.r.harris@intel.com>; Wodkowski, PawelX
> <pawelx.wodkowski@intel.com>; dev@dpdk.org; Liu, Changpeng
> <changpeng.liu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space
> messages
>
>
>
> On 03/27/2018 05:35 PM, Tomasz Kulasek wrote:
> > This patch adds new vhost user messages GET_CONFIG and SET_CONFIG used
> > for get/set virtio device's configuration space.
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> > ---
> > Changes in v2:
> > - code cleanup
> >
> > lib/librte_vhost/rte_vhost.h | 4 ++++
> > lib/librte_vhost/vhost_user.c | 22 ++++++++++++++++++++++
> > lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index d332069..fe30518 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -84,6 +84,10 @@ struct vhost_device_ops {
> > int (*new_connection)(int vid);
> > void (*destroy_connection)(int vid);
> >
> > + int (*get_config)(int vid, uint8_t *config, uint32_t config_len);
> > + int (*set_config)(int vid, uint8_t *config, uint32_t offset,
> > + uint32_t len, uint32_t flags);
> > +
> > void *reserved[2]; /**< Reserved for future extension */
>
> You are breaking the ABI, as you grow the size of the ops struct.
>
> Also, I'm wondering if we shouldn't have a different ops for external
> backends. Here these ops are more intended to the application, we could
> have a specific ops struct for external backends IMHO.
>
> > };
> >
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 90ed211..0ed6a5a 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -50,6 +50,8 @@ static const char *vhost_message_str[VHOST_USER_MAX]
> = {
> > [VHOST_USER_NET_SET_MTU] = "VHOST_USER_NET_SET_MTU",
> > [VHOST_USER_SET_SLAVE_REQ_FD] =
> "VHOST_USER_SET_SLAVE_REQ_FD",
> > [VHOST_USER_IOTLB_MSG] = "VHOST_USER_IOTLB_MSG",
> > + [VHOST_USER_GET_CONFIG] = "VHOST_USER_GET_CONFIG",
> > + [VHOST_USER_SET_CONFIG] = "VHOST_USER_SET_CONFIG",
> > };
> >
> > static uint64_t
> > @@ -1355,6 +1357,7 @@ vhost_user_msg_handler(int vid, int fd)
> > * would cause a dead lock.
> > */
> > switch (msg.request.master) {
> > + case VHOST_USER_SET_CONFIG:
>
> It seems VHOST_USER_GET_CONFIG is missing here.
>
> > case VHOST_USER_SET_FEATURES:
> > case VHOST_USER_SET_PROTOCOL_FEATURES:
> > case VHOST_USER_SET_OWNER:
> > @@ -1380,6 +1383,25 @@ vhost_user_msg_handler(int vid, int fd)
> > }
> >
> > switch (msg.request.master) {
> > + case VHOST_USER_GET_CONFIG:
> > + if (dev->notify_ops->get_config(dev->vid,
> Please check ->get_config is set before calling it.
>
> > + msg.payload.config.region,
> > + msg.payload.config.size) != 0) {
> > + msg.size = sizeof(uint64_t);
> > + }
> > + send_vhost_reply(fd, &msg);
> > + break;
> > + case VHOST_USER_SET_CONFIG:
> > + if ((dev->notify_ops->set_config(dev->vid,
> Ditto.
>
> > + msg.payload.config.region,
> > + msg.payload.config.offset,
> > + msg.payload.config.size,
> > + msg.payload.config.flags)) != 0) {
> > + ret = 1;
> > + } else {
> > + ret = 0;
> > + }
>
> ret = dev->notify_ops->set_config instead?
> > + break;
> > case VHOST_USER_GET_FEATURES:
> > msg.payload.u64 = vhost_user_get_features(dev);
> > msg.size = sizeof(msg.payload.u64);
> > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> > index d4bd604..25cc026 100644
> > --- a/lib/librte_vhost/vhost_user.h
> > +++ b/lib/librte_vhost/vhost_user.h
> > @@ -14,6 +14,11 @@
> >
> > #define VHOST_MEMORY_MAX_NREGIONS 8
> >
> > +/*
> > + * Maximum size of virtio device config space
> > + */
> > +#define VHOST_USER_MAX_CONFIG_SIZE 256
> > +
> > #define VHOST_USER_PROTOCOL_F_MQ 0
> > #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
> > #define VHOST_USER_PROTOCOL_F_RARP 2
>
> Shouldn't there be a protocol feature associated to these new messages?
> Else how QEMU knows the backend supports it or not?
>
> I looked at QEMU code and indeed no protocol feature associated, that's
> strange...
Nice to have, for now not all the QEMU host driver need to get this configuration space from slave backend
when getting start. This message can be used for migration of vhost-user devices.
>
> > @@ -52,12 +57,15 @@ typedef enum VhostUserRequest {
> > VHOST_USER_NET_SET_MTU = 20,
> > VHOST_USER_SET_SLAVE_REQ_FD = 21,
> > VHOST_USER_IOTLB_MSG = 22,
> > + VHOST_USER_GET_CONFIG = 24,
> > + VHOST_USER_SET_CONFIG = 25,
> > VHOST_USER_MAX
> > } VhostUserRequest;
> >
> > typedef enum VhostUserSlaveRequest {
> > VHOST_USER_SLAVE_NONE = 0,
> > VHOST_USER_SLAVE_IOTLB_MSG = 1,
> > + VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
> > VHOST_USER_SLAVE_MAX
> > } VhostUserSlaveRequest;
> >
> > @@ -79,6 +87,13 @@ typedef struct VhostUserLog {
> > uint64_t mmap_offset;
> > } VhostUserLog;
> >
> > +typedef struct VhostUserConfig {
> > + uint32_t offset;
> > + uint32_t size;
> > + uint32_t flags;
> > + uint8_t region[VHOST_USER_MAX_CONFIG_SIZE];
> > +} VhostUserConfig;
> > +
> > typedef struct VhostUserMsg {
> > union {
> > VhostUserRequest master;
> > @@ -98,6 +113,7 @@ typedef struct VhostUserMsg {
> > struct vhost_vring_addr addr;
> > VhostUserMemory memory;
> > VhostUserLog log;
> > + VhostUserConfig config;
> > struct vhost_iotlb_msg iotlb;
> > } payload;
> > int fds[VHOST_MEMORY_MAX_NREGIONS];
> >
next prev parent reply other threads:[~2018-03-28 9:50 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-27 15:17 [dpdk-dev] [PATCH] " Tomasz Kulasek
2018-03-27 15:35 ` [dpdk-dev] [PATCH v2] " Tomasz Kulasek
2018-03-28 9:11 ` Maxime Coquelin
2018-03-28 9:19 ` Wodkowski, PawelX
2018-03-28 9:33 ` Maxime Coquelin
2018-03-28 9:48 ` Maxime Coquelin
2018-03-28 9:50 ` Liu, Changpeng [this message]
2018-03-28 9:57 ` Maxime Coquelin
2018-03-28 10:03 ` Liu, Changpeng
2018-03-28 10:11 ` Maxime Coquelin
2018-03-28 10:23 ` Liu, Changpeng
2018-03-28 10:56 ` Maxime Coquelin
2018-04-19 14:39 ` Maxime Coquelin
2018-04-20 0:32 ` Liu, Changpeng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=FF7FC980937D6342B9D289F5F3C7C2625B6252DD@SHSMSX103.ccr.corp.intel.com \
--to=changpeng.liu@intel.com \
--cc=daniel.verkamp@intel.com \
--cc=dev@dpdk.org \
--cc=james.r.harris@intel.com \
--cc=jianfeng.tan@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=pawelx.wodkowski@intel.com \
--cc=tomaszx.kulasek@intel.com \
--cc=yliu@fridaylinux.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).