Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space messages

["Liu, Changpeng" <changpeng.liu@intel.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];
> >