diff mbox

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

Message ID 20180327153500.10464-1-tomaszx.kulasek@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Maxime Coquelin
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Tomasz Kulasek March 27, 2018, 3:35 p.m. UTC 
  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(+)

Comments

Maxime Coquelin March 28, 2018, 9:11 a.m. UTC | #1 
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...

> @@ -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];
>
Wodkowski, PawelX March 28, 2018, 9:19 a.m. UTC | #2 
> -----Original Message-----

> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> Sent: Wednesday, March 28, 2018 11:12 AM

> 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.


What do mean by "external backends" ?
> 

> >   };

> >

> > 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...

> 

> > @@ -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];

> >
Maxime Coquelin March 28, 2018, 9:33 a.m. UTC | #3 
On 03/28/2018 11:19 AM, Wodkowski, PawelX wrote:
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, March 28, 2018 11:12 AM
>> 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.
> 
> What do mean by "external backends" ?

Libs like SPDK or Crypto that implements their own ring processing,
comparing to an application like DPDK that doesn't care of rings
details.
Maxime Coquelin March 28, 2018, 9:48 a.m. UTC | #4 
On 03/28/2018 11:33 AM, Maxime Coquelin wrote:
> 
> 
> On 03/28/2018 11:19 AM, Wodkowski, PawelX wrote:
>>> -----Original Message-----
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Wednesday, March 28, 2018 11:12 AM
>>> 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.
>>
>> What do mean by "external backends" ?
> 
> Libs like SPDK or Crypto that implements their own ring processing,
> comparing to an application like DPDK that doesn't care of rings
> details.

Sorry, I meant comparing to an application like OVS*
Liu, Changpeng March 28, 2018, 9:50 a.m. UTC | #5 
> -----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];

> >
Maxime Coquelin March 28, 2018, 9:57 a.m. UTC | #6 
On 03/28/2018 11:50 AM, Liu, Changpeng wrote:
> 
> 
>> -----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.

So if QEMU sends this message but the DPDK version does not support it
yet, vhost_user_msg_handler() will return an error ("vhost read
incorrect message") and the socket will be closed.

How do we overcome this? I think we really need a spec update ASAP, 
before QEMU v2.12 is out (-rc1 already).

Do you have time to take care of this?

Thanks,
Maxime
Liu, Changpeng March 28, 2018, 10:03 a.m. UTC | #7 
> -----Original Message-----

> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> Sent: Wednesday, March 28, 2018 5:58 PM

> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng

> <jianfeng.tan@intel.com>

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

> messages

> 

> 

> 

> On 03/28/2018 11:50 AM, Liu, Changpeng wrote:

> >

> >

> >> -----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.

> 

> So if QEMU sends this message but the DPDK version does not support it

> yet, vhost_user_msg_handler() will return an error ("vhost read

> incorrect message") and the socket will be closed.

> 

> How do we overcome this? I think we really need a spec update ASAP,

> before QEMU v2.12 is out (-rc1 already).

> 

> Do you have time to take care of this?

For now there are no other users except us care about this message, :), it's no hurry.
I can take this after QEMU 2.12 release adding a new protocol feature bit.
> 

> Thanks,

> Maxime
Maxime Coquelin March 28, 2018, 10:11 a.m. UTC | #8 
On 03/28/2018 12:03 PM, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, March 28, 2018 5:58 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng
>> <jianfeng.tan@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space
>> messages
>>
>>
>>
>> On 03/28/2018 11:50 AM, Liu, Changpeng wrote:
>>>
>>>
>>>> -----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.
>>
>> So if QEMU sends this message but the DPDK version does not support it
>> yet, vhost_user_msg_handler() will return an error ("vhost read
>> incorrect message") and the socket will be closed.
>>
>> How do we overcome this? I think we really need a spec update ASAP,
>> before QEMU v2.12 is out (-rc1 already).
>>
>> Do you have time to take care of this?
> For now there are no other users except us care about this message, :), it's no hurry.
> I can take this after QEMU 2.12 release adding a new protocol feature bit.

Are you sure?
If I understand the code correctly, as the guest writes in config regs
of a virtio-blk device, .set_config callback will be called.

If you have a vhost-user backend, it will receive the SET_CONFIG
request, no?

Cheers,
Maxime

>>
>> Thanks,
>> Maxime
Liu, Changpeng March 28, 2018, 10:23 a.m. UTC | #9 
> -----Original Message-----

> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> Sent: Wednesday, March 28, 2018 6:11 PM

> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng

> <jianfeng.tan@intel.com>

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

> messages

> 

> 

> 

> On 03/28/2018 12:03 PM, Liu, Changpeng wrote:

> >

> >

> >> -----Original Message-----

> >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> >> Sent: Wednesday, March 28, 2018 5:58 PM

> >> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng

> >> <jianfeng.tan@intel.com>

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

> >> messages

> >>

> >>

> >>

> >> On 03/28/2018 11:50 AM, Liu, Changpeng wrote:

> >>>

> >>>

> >>>> -----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.

> >>

> >> So if QEMU sends this message but the DPDK version does not support it

> >> yet, vhost_user_msg_handler() will return an error ("vhost read

> >> incorrect message") and the socket will be closed.

> >>

> >> How do we overcome this? I think we really need a spec update ASAP,

> >> before QEMU v2.12 is out (-rc1 already).

> >>

> >> Do you have time to take care of this?

> > For now there are no other users except us care about this message, :), it's no

> hurry.

> > I can take this after QEMU 2.12 release adding a new protocol feature bit.

> 

> Are you sure?

> If I understand the code correctly, as the guest writes in config regs

> of a virtio-blk device, .set_config callback will be called.

Exactly.
> 

> If you have a vhost-user backend, it will receive the SET_CONFIG

> request, no?

For now this only enabled for QEMU vhost-user-blk driver, QEMU virtio-blk driver didn't have such issue.
> 

> Cheers,

> Maxime

> 

> >>

> >> Thanks,

> >> Maxime
Maxime Coquelin March 28, 2018, 10:56 a.m. UTC | #10 
On 03/28/2018 12:23 PM, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, March 28, 2018 6:11 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng
>> <jianfeng.tan@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space
>> messages
>>
>>
>>
>> On 03/28/2018 12:03 PM, Liu, Changpeng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>> Sent: Wednesday, March 28, 2018 5:58 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng
>>>> <jianfeng.tan@intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space
>>>> messages
>>>>
>>>>
>>>>
>>>> On 03/28/2018 11:50 AM, Liu, Changpeng wrote:
>>>>>
>>>>>
>>>>>> -----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.
>>>>
>>>> So if QEMU sends this message but the DPDK version does not support it
>>>> yet, vhost_user_msg_handler() will return an error ("vhost read
>>>> incorrect message") and the socket will be closed.
>>>>
>>>> How do we overcome this? I think we really need a spec update ASAP,
>>>> before QEMU v2.12 is out (-rc1 already).
>>>>
>>>> Do you have time to take care of this?
>>> For now there are no other users except us care about this message, :), it's no
>> hurry.
>>> I can take this after QEMU 2.12 release adding a new protocol feature bit.
>>
>> Are you sure?
>> If I understand the code correctly, as the guest writes in config regs
>> of a virtio-blk device, .set_config callback will be called.
> Exactly.
>>
>> If you have a vhost-user backend, it will receive the SET_CONFIG
>> request, no?
> For now this only enabled for QEMU vhost-user-blk driver, QEMU virtio-blk driver didn't have such issue.

Right.
But it will be really painful to manage for example for cross-version
live migration. Or when you'll want to use QEMU v2.13+ with a DPDK
v18.05 backend, the protocol feature won't be negotiated.

Really, this is important to get it right at the beginning.

Thanks,
Maxime
>>
>> Cheers,
>> Maxime
>>
>>>>
>>>> Thanks,
>>>> Maxime
Maxime Coquelin April 19, 2018, 2:39 p.m. UTC | #11 
Hi Changpeng, Tomasz,

Any chance that you resubmit the series now that the Qemu changes adding
a protocol feature flag has been accepted?

Cheers,
Maxime
On 03/28/2018 12:56 PM, Maxime Coquelin wrote:
> 
> 
> On 03/28/2018 12:23 PM, Liu, Changpeng wrote:
>>
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Wednesday, March 28, 2018 6:11 PM
>>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng
>>> <jianfeng.tan@intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration space
>>> messages
>>>
>>>
>>>
>>> On 03/28/2018 12:03 PM, Liu, Changpeng wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>>> Sent: Wednesday, March 28, 2018 5:58 PM
>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng
>>>>> <jianfeng.tan@intel.com>
>>>>> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration 
>>>>> space
>>>>> messages
>>>>>
>>>>>
>>>>>
>>>>> On 03/28/2018 11:50 AM, Liu, Changpeng wrote:
>>>>>>
>>>>>>
>>>>>>> -----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.
>>>>>
>>>>> So if QEMU sends this message but the DPDK version does not support it
>>>>> yet, vhost_user_msg_handler() will return an error ("vhost read
>>>>> incorrect message") and the socket will be closed.
>>>>>
>>>>> How do we overcome this? I think we really need a spec update ASAP,
>>>>> before QEMU v2.12 is out (-rc1 already).
>>>>>
>>>>> Do you have time to take care of this?
>>>> For now there are no other users except us care about this message, 
>>>> :), it's no
>>> hurry.
>>>> I can take this after QEMU 2.12 release adding a new protocol 
>>>> feature bit.
>>>
>>> Are you sure?
>>> If I understand the code correctly, as the guest writes in config regs
>>> of a virtio-blk device, .set_config callback will be called.
>> Exactly.
>>>
>>> If you have a vhost-user backend, it will receive the SET_CONFIG
>>> request, no?
>> For now this only enabled for QEMU vhost-user-blk driver, QEMU 
>> virtio-blk driver didn't have such issue.
> 
> Right.
> But it will be really painful to manage for example for cross-version
> live migration. Or when you'll want to use QEMU v2.13+ with a DPDK
> v18.05 backend, the protocol feature won't be negotiated.
> 
> Really, this is important to get it right at the beginning.
> 
> Thanks,
> Maxime
>>>
>>> Cheers,
>>> Maxime
>>>
>>>>>
>>>>> Thanks,
>>>>> Maxime
Liu, Changpeng April 20, 2018, 12:32 a.m. UTC | #12 
Hi Maxime,

We'll submit a new v3 version soon after tested with QEMU 2.12 release.

> -----Original Message-----

> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> Sent: Thursday, April 19, 2018 10:40 PM

> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng <jianfeng.tan@intel.com>

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

> 

> Hi Changpeng, Tomasz,

> 

> Any chance that you resubmit the series now that the Qemu changes adding

> a protocol feature flag has been accepted?

> 

> Cheers,

> Maxime

> On 03/28/2018 12:56 PM, Maxime Coquelin wrote:

> >

> >

> > On 03/28/2018 12:23 PM, Liu, Changpeng wrote:

> >>

> >>

> >>> -----Original Message-----

> >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> >>> Sent: Wednesday, March 28, 2018 6:11 PM

> >>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng

> >>> <jianfeng.tan@intel.com>

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

> >>> messages

> >>>

> >>>

> >>>

> >>> On 03/28/2018 12:03 PM, Liu, Changpeng wrote:

> >>>>

> >>>>

> >>>>> -----Original Message-----

> >>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> >>>>> Sent: Wednesday, March 28, 2018 5:58 PM

> >>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; 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; Tan, Jianfeng

> >>>>> <jianfeng.tan@intel.com>

> >>>>> Subject: Re: [dpdk-dev] [PATCH v2] vhost: add virtio configuration

> >>>>> space

> >>>>> messages

> >>>>>

> >>>>>

> >>>>>

> >>>>> On 03/28/2018 11:50 AM, Liu, Changpeng wrote:

> >>>>>>

> >>>>>>

> >>>>>>> -----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.

> >>>>>

> >>>>> So if QEMU sends this message but the DPDK version does not support it

> >>>>> yet, vhost_user_msg_handler() will return an error ("vhost read

> >>>>> incorrect message") and the socket will be closed.

> >>>>>

> >>>>> How do we overcome this? I think we really need a spec update ASAP,

> >>>>> before QEMU v2.12 is out (-rc1 already).

> >>>>>

> >>>>> Do you have time to take care of this?

> >>>> For now there are no other users except us care about this message,

> >>>> :), it's no

> >>> hurry.

> >>>> I can take this after QEMU 2.12 release adding a new protocol

> >>>> feature bit.

> >>>

> >>> Are you sure?

> >>> If I understand the code correctly, as the guest writes in config regs

> >>> of a virtio-blk device, .set_config callback will be called.

> >> Exactly.

> >>>

> >>> If you have a vhost-user backend, it will receive the SET_CONFIG

> >>> request, no?

> >> For now this only enabled for QEMU vhost-user-blk driver, QEMU

> >> virtio-blk driver didn't have such issue.

> >

> > Right.

> > But it will be really painful to manage for example for cross-version

> > live migration. Or when you'll want to use QEMU v2.13+ with a DPDK

> > v18.05 backend, the protocol feature won't be negotiated.

> >

> > Really, this is important to get it right at the beginning.

> >

> > Thanks,

> > Maxime

> >>>

> >>> Cheers,

> >>> Maxime

> >>>

> >>>>>

> >>>>> Thanks,

> >>>>> Maxime
diff mbox

Patch

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 */
 };
 
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:
 	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,
+				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,
+				msg.payload.config.region,
+				msg.payload.config.offset,
+				msg.payload.config.size,
+				msg.payload.config.flags)) != 0) {
+			ret = 1;
+		} else {
+			ret = 0;
+		}
+		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
@@ -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];