diff mbox series

vhost: add virtio configuration space access socket messages

Message ID 1551081095-14286-1-git-send-email-changpeng.liu@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: add virtio configuration space access socket messages |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Liu, Changpeng Feb. 25, 2019, 7:51 a.m. UTC 
  This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
used to get/set virtio device's PCI configuration space.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 lib/librte_vhost/rte_vhost.h  |  8 ++++++++
 lib/librte_vhost/vhost_user.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
 3 files changed, 68 insertions(+)

Comments

Jason Wang Feb. 25, 2019, 7:48 a.m. UTC | #1 
On 2019/2/25 下午3:51, Changpeng Liu wrote:
> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> used to get/set virtio device's PCI configuration space.


I think it's better to describe the reason for doing this. I believe 
vhost is transport independent.

Thanks


>
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>   lib/librte_vhost/rte_vhost.h  |  8 ++++++++
>   lib/librte_vhost/vhost_user.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>   lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
>   3 files changed, 68 insertions(+)
>
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 2753670..ab710ce 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -63,6 +63,10 @@
>   #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
>   #endif
>   
> +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> +#endif
> +
>   #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>   #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
>   #endif
> @@ -173,6 +177,10 @@ struct vhost_device_ops {
>   
>   	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);	/**< triggered when a vring is enabled or disabled */
>   
> +	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);
> +
>   	/**
>   	 * Features could be changed after the feature negotiation.
>   	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index b086ad9..8e42734 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -73,6 +73,8 @@
>   	[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",
>   	[VHOST_USER_CRYPTO_CREATE_SESS] = "VHOST_USER_CRYPTO_CREATE_SESS",
>   	[VHOST_USER_CRYPTO_CLOSE_SESS] = "VHOST_USER_CRYPTO_CLOSE_SESS",
>   	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
> @@ -359,6 +361,46 @@
>   	return RTE_VHOST_MSG_RESULT_OK;
>   }
>   
> +static int
> +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> +		      int main_fd __rte_unused)
> +{
> +	struct virtio_net *dev = *pdev;
> +
> +	if (!dev->notify_ops->get_config) {
> +		msg->size = sizeof(uint64_t);
> +		return RTE_VHOST_MSG_RESULT_REPLY;
> +	}
> +
> +	if (dev->notify_ops->get_config(dev->vid,
> +					msg->payload.config.region,
> +					msg->payload.config.size) != 0) {
> +		msg->size = sizeof(uint64_t);
> +	}
> +
> +	return RTE_VHOST_MSG_RESULT_REPLY;
> +}
> +
> +static int
> +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> +		      int main_fd __rte_unused)
> +{
> +	struct virtio_net *dev = *pdev;
> +
> +	if (!dev->notify_ops->set_config)
> +		return RTE_VHOST_MSG_RESULT_ERR;
> +
> +	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) {
> +		return RTE_VHOST_MSG_RESULT_ERR;
> +	}
> +
> +	return RTE_VHOST_MSG_RESULT_OK;
> +}
> +
>   /*
>    * Reallocate virtio_dev and vhost_virtqueue data structure to make them on the
>    * same numa node as the memory of vring descriptor.
> @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
>   	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>   	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
>   	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
>   	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
>   	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index 2a650fe..057cdec 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -12,6 +12,11 @@
>   
>   /* refer to hw/virtio/vhost-user.c */
>   
> +/*
> + * Maximum size of virtio device config space
> + */
> +#define VHOST_USER_MAX_CONFIG_SIZE 256
> +
>   #define VHOST_MEMORY_MAX_NREGIONS 8
>   
>   #define VHOST_USER_PROTOCOL_FEATURES	((1ULL << VHOST_USER_PROTOCOL_F_MQ) | \
> @@ -49,6 +54,8 @@
>   	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_CRYPTO_CREATE_SESS = 26,
>   	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
>   	VHOST_USER_POSTCOPY_ADVISE = 28,
> @@ -60,6 +67,7 @@
>   typedef enum VhostUserSlaveRequest {
>   	VHOST_USER_SLAVE_NONE = 0,
>   	VHOST_USER_SLAVE_IOTLB_MSG = 1,
> +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
>   	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
>   	VHOST_USER_SLAVE_MAX
>   } VhostUserSlaveRequest;
> @@ -112,6 +120,13 @@
>   	uint64_t offset;
>   } VhostUserVringArea;
>   
> +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 {
>   		uint32_t master; /* a VhostUserRequest value */
> @@ -131,6 +146,7 @@
>   		struct vhost_vring_addr addr;
>   		VhostUserMemory memory;
>   		VhostUserLog    log;
> +		VhostUserConfig config;
>   		struct vhost_iotlb_msg iotlb;
>   		VhostUserCryptoSessionParam crypto_session;
>   		VhostUserVringArea area;
Stojaczyk, Dariusz Feb. 25, 2019, 11:49 a.m. UTC | #2 
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Monday, February 25, 2019 8:49 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong <zhihong.wang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] vhost: add virtio configuration space access
> socket messages
> 
> 
> On 2019/2/25 下午3:51, Changpeng Liu wrote:
> > This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> > used to get/set virtio device's PCI configuration space.
> 
> 
> I think it's better to describe the reason for doing this. I believe
> vhost is transport independent.

Virtio does define a generic configuration space [Virtio 1.0, 2.3 Device
Configuration Space]. It's not PCI specific at all - I believe Changpeng
mentioned PCI just to clearly lay things out.

Besides, GET/SET_CONFIG is already defined in the vhost-user spec:

https://github.com/qemu/qemu/blob/master/docs/interop/vhost-user.txt#L678

Still, I agree this commit could use some more description.

D.

> 
> Thanks
> 
> 
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >   lib/librte_vhost/rte_vhost.h  |  8 ++++++++
> >   lib/librte_vhost/vhost_user.c | 44
> +++++++++++++++++++++++++++++++++++++++++++
> >   lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
> >   3 files changed, 68 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index 2753670..ab710ce 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -63,6 +63,10 @@
> >   #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
> >   #endif
> >
> > +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> > +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> > +#endif
> > +
> >   #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
> >   #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
> >   #endif
> > @@ -173,6 +177,10 @@ struct vhost_device_ops {
> >
> >   	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);
> 	/**< triggered when a vring is enabled or disabled */
> >
> > +	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);
> > +
> >   	/**
> >   	 * Features could be changed after the feature negotiation.
> >   	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index b086ad9..8e42734 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -73,6 +73,8 @@
> >   	[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",
> >   	[VHOST_USER_CRYPTO_CREATE_SESS] =
> "VHOST_USER_CRYPTO_CREATE_SESS",
> >   	[VHOST_USER_CRYPTO_CLOSE_SESS] =
> "VHOST_USER_CRYPTO_CLOSE_SESS",
> >   	[VHOST_USER_POSTCOPY_ADVISE]  =
> "VHOST_USER_POSTCOPY_ADVISE",
> > @@ -359,6 +361,46 @@
> >   	return RTE_VHOST_MSG_RESULT_OK;
> >   }
> >
> > +static int
> > +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg
> *msg,
> > +		      int main_fd __rte_unused)
> > +{
> > +	struct virtio_net *dev = *pdev;
> > +
> > +	if (!dev->notify_ops->get_config) {
> > +		msg->size = sizeof(uint64_t);
> > +		return RTE_VHOST_MSG_RESULT_REPLY;
> > +	}
> > +
> > +	if (dev->notify_ops->get_config(dev->vid,
> > +					msg->payload.config.region,
> > +					msg->payload.config.size) != 0) {
> > +		msg->size = sizeof(uint64_t);
> > +	}
> > +
> > +	return RTE_VHOST_MSG_RESULT_REPLY;
> > +}
> > +
> > +static int
> > +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg
> *msg,
> > +		      int main_fd __rte_unused)
> > +{
> > +	struct virtio_net *dev = *pdev;
> > +
> > +	if (!dev->notify_ops->set_config)
> > +		return RTE_VHOST_MSG_RESULT_ERR;
> > +
> > +	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) {
> > +		return RTE_VHOST_MSG_RESULT_ERR;
> > +	}
> > +
> > +	return RTE_VHOST_MSG_RESULT_OK;
> > +}
> > +
> >   /*
> >    * Reallocate virtio_dev and vhost_virtqueue data structure to make them
> on the
> >    * same numa node as the memory of vring descriptor.
> > @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct
> virtio_net **pdev,
> >   	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
> >   	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
> >   	[VHOST_USER_POSTCOPY_ADVISE] =
> vhost_user_set_postcopy_advise,
> >   	[VHOST_USER_POSTCOPY_LISTEN] =
> vhost_user_set_postcopy_listen,
> >   	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
> > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> > index 2a650fe..057cdec 100644
> > --- a/lib/librte_vhost/vhost_user.h
> > +++ b/lib/librte_vhost/vhost_user.h
> > @@ -12,6 +12,11 @@
> >
> >   /* refer to hw/virtio/vhost-user.c */
> >
> > +/*
> > + * Maximum size of virtio device config space
> > + */
> > +#define VHOST_USER_MAX_CONFIG_SIZE 256
> > +
> >   #define VHOST_MEMORY_MAX_NREGIONS 8
> >
> >   #define VHOST_USER_PROTOCOL_FEATURES	((1ULL <<
> VHOST_USER_PROTOCOL_F_MQ) | \
> > @@ -49,6 +54,8 @@
> >   	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_CRYPTO_CREATE_SESS = 26,
> >   	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
> >   	VHOST_USER_POSTCOPY_ADVISE = 28,
> > @@ -60,6 +67,7 @@
> >   typedef enum VhostUserSlaveRequest {
> >   	VHOST_USER_SLAVE_NONE = 0,
> >   	VHOST_USER_SLAVE_IOTLB_MSG = 1,
> > +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
> >   	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
> >   	VHOST_USER_SLAVE_MAX
> >   } VhostUserSlaveRequest;
> > @@ -112,6 +120,13 @@
> >   	uint64_t offset;
> >   } VhostUserVringArea;
> >
> > +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 {
> >   		uint32_t master; /* a VhostUserRequest value */
> > @@ -131,6 +146,7 @@
> >   		struct vhost_vring_addr addr;
> >   		VhostUserMemory memory;
> >   		VhostUserLog    log;
> > +		VhostUserConfig config;
> >   		struct vhost_iotlb_msg iotlb;
> >   		VhostUserCryptoSessionParam crypto_session;
> >   		VhostUserVringArea area;
Stojaczyk, Dariusz Feb. 25, 2019, 12:05 p.m. UTC | #3 
> -----Original Message-----
> From: Liu, Changpeng
> Sent: Monday, February 25, 2019 8:52 AM
> To: dev@dpdk.org
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Stojaczyk, Dariusz
> <dariusz.stojaczyk@intel.com>; maxime.coquelin@redhat.com; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
> Subject: [PATCH] vhost: add virtio configuration space access socket
> messages
> 
> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> used to get/set virtio device's PCI configuration space.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---

After the commit msg is expanded:

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks,
D.
Ilya Maximets Feb. 25, 2019, 1:19 p.m. UTC | #4 
On 25.02.2019 10:51, Changpeng Liu wrote:
> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> used to get/set virtio device's PCI configuration space.

Beside the fact that some additional description and reasoning required,
I do not see the usage of this feature. You're defining the flag
VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk vhost
backends (vdpa, vhost-user) will use this feature.
You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
VDPA_SUPPORTED_PROTOCOL_FEATURES.

From the other side, current implementation forces application to properly
implement the get/set_config callbacks. Otherwise, receiving of the messages
will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost disconnection.
This looks strange, because supported protocol features normally enabled by
default. Am I misunderstood something ?

One more thing. According to spec: "vhost-user slave uses zero length of
payload to indicate an error to vhost-user master". However, current version
of 'vhost_user_get_config' replies with size equal to 'sizeof(uint64_t)'.

Best regards, Ilya Maximets.

> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
>  lib/librte_vhost/vhost_user.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 2753670..ab710ce 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -63,6 +63,10 @@
>  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
>  #endif
>  
> +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> +#endif
> +
>  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
>  #endif
> @@ -173,6 +177,10 @@ struct vhost_device_ops {
>  
>  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);	/**< triggered when a vring is enabled or disabled */
>  
> +	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);
> +
>  	/**
>  	 * Features could be changed after the feature negotiation.
>  	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index b086ad9..8e42734 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -73,6 +73,8 @@
>  	[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",
>  	[VHOST_USER_CRYPTO_CREATE_SESS] = "VHOST_USER_CRYPTO_CREATE_SESS",
>  	[VHOST_USER_CRYPTO_CLOSE_SESS] = "VHOST_USER_CRYPTO_CLOSE_SESS",
>  	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
> @@ -359,6 +361,46 @@
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
>  
> +static int
> +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> +		      int main_fd __rte_unused)
> +{
> +	struct virtio_net *dev = *pdev;
> +
> +	if (!dev->notify_ops->get_config) {
> +		msg->size = sizeof(uint64_t);
> +		return RTE_VHOST_MSG_RESULT_REPLY;
> +	}
> +
> +	if (dev->notify_ops->get_config(dev->vid,
> +					msg->payload.config.region,
> +					msg->payload.config.size) != 0) {
> +		msg->size = sizeof(uint64_t);
> +	}
> +
> +	return RTE_VHOST_MSG_RESULT_REPLY;
> +}
> +
> +static int
> +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> +		      int main_fd __rte_unused)
> +{
> +	struct virtio_net *dev = *pdev;
> +
> +	if (!dev->notify_ops->set_config)
> +		return RTE_VHOST_MSG_RESULT_ERR;
> +
> +	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) {
> +		return RTE_VHOST_MSG_RESULT_ERR;
> +	}
> +
> +	return RTE_VHOST_MSG_RESULT_OK;
> +}
> +
>  /*
>   * Reallocate virtio_dev and vhost_virtqueue data structure to make them on the
>   * same numa node as the memory of vring descriptor.
> @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
>  	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>  	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
>  	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
>  	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
>  	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index 2a650fe..057cdec 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -12,6 +12,11 @@
>  
>  /* refer to hw/virtio/vhost-user.c */
>  
> +/*
> + * Maximum size of virtio device config space
> + */
> +#define VHOST_USER_MAX_CONFIG_SIZE 256
> +
>  #define VHOST_MEMORY_MAX_NREGIONS 8
>  
>  #define VHOST_USER_PROTOCOL_FEATURES	((1ULL << VHOST_USER_PROTOCOL_F_MQ) | \
> @@ -49,6 +54,8 @@
>  	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_CRYPTO_CREATE_SESS = 26,
>  	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
>  	VHOST_USER_POSTCOPY_ADVISE = 28,
> @@ -60,6 +67,7 @@
>  typedef enum VhostUserSlaveRequest {
>  	VHOST_USER_SLAVE_NONE = 0,
>  	VHOST_USER_SLAVE_IOTLB_MSG = 1,
> +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
>  	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
>  	VHOST_USER_SLAVE_MAX
>  } VhostUserSlaveRequest;
> @@ -112,6 +120,13 @@
>  	uint64_t offset;
>  } VhostUserVringArea;
>  
> +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 {
>  		uint32_t master; /* a VhostUserRequest value */
> @@ -131,6 +146,7 @@
>  		struct vhost_vring_addr addr;
>  		VhostUserMemory memory;
>  		VhostUserLog    log;
> +		VhostUserConfig config;
>  		struct vhost_iotlb_msg iotlb;
>  		VhostUserCryptoSessionParam crypto_session;
>  		VhostUserVringArea area;
>
Ilya Maximets Feb. 25, 2019, 1:53 p.m. UTC | #5 
On 25.02.2019 10:51, Changpeng Liu wrote:
> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> used to get/set virtio device's PCI configuration space.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
>  lib/librte_vhost/vhost_user.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 2753670..ab710ce 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -63,6 +63,10 @@
>  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
>  #endif
>  
> +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> +#endif
> +
>  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
>  #endif
> @@ -173,6 +177,10 @@ struct vhost_device_ops {
>  
>  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);	/**< triggered when a vring is enabled or disabled */
>  
> +	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);

'struct vhost_device_ops' is user visible. This changes API and ABI.
You need to update docs/rel_notes and bump the library version accordingly.

Best regards, Ilya Maximets.
Liu, Changpeng Feb. 26, 2019, 7:01 a.m. UTC | #6 
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Monday, February 25, 2019 9:20 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> Subject: Re: vhost: add virtio configuration space access socket messages
> 
> On 25.02.2019 10:51, Changpeng Liu wrote:
> > This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> > used to get/set virtio device's PCI configuration space.
> 
> Beside the fact that some additional description and reasoning required,
> I do not see the usage of this feature. You're defining the flag
> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk vhost
> backends (vdpa, vhost-user) will use this feature.
> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
> VDPA_SUPPORTED_PROTOCOL_FEATURES.
> 
> From the other side, current implementation forces application to properly
> implement the get/set_config callbacks. Otherwise, receiving of the messages
> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
> disconnection.
> This looks strange, because supported protocol features normally enabled by
> default. Am I misunderstood something ?
QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG wasn't enabled.
> 
> One more thing. According to spec: "vhost-user slave uses zero length of
> payload to indicate an error to vhost-user master". However, current version
> of 'vhost_user_get_config' replies with size equal to 'sizeof(uint64_t)'.
Yeah, can be changed to 0 here, QEMU can process it.
> 
> Best regards, Ilya Maximets.
> 
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > ---
> >  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
> >  lib/librte_vhost/vhost_user.c | 44
> +++++++++++++++++++++++++++++++++++++++++++
> >  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
> >  3 files changed, 68 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index 2753670..ab710ce 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -63,6 +63,10 @@
> >  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
> >  #endif
> >
> > +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> > +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> > +#endif
> > +
> >  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
> >  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
> >  #endif
> > @@ -173,6 +177,10 @@ struct vhost_device_ops {
> >
> >  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);
> 	/**< triggered when a vring is enabled or disabled */
> >
> > +	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);
> > +
> >  	/**
> >  	 * Features could be changed after the feature negotiation.
> >  	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index b086ad9..8e42734 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -73,6 +73,8 @@
> >  	[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",
> >  	[VHOST_USER_CRYPTO_CREATE_SESS] =
> "VHOST_USER_CRYPTO_CREATE_SESS",
> >  	[VHOST_USER_CRYPTO_CLOSE_SESS] =
> "VHOST_USER_CRYPTO_CLOSE_SESS",
> >  	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
> > @@ -359,6 +361,46 @@
> >  	return RTE_VHOST_MSG_RESULT_OK;
> >  }
> >
> > +static int
> > +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> > +		      int main_fd __rte_unused)
> > +{
> > +	struct virtio_net *dev = *pdev;
> > +
> > +	if (!dev->notify_ops->get_config) {
> > +		msg->size = sizeof(uint64_t);
> > +		return RTE_VHOST_MSG_RESULT_REPLY;
> > +	}
> > +
> > +	if (dev->notify_ops->get_config(dev->vid,
> > +					msg->payload.config.region,
> > +					msg->payload.config.size) != 0) {
> > +		msg->size = sizeof(uint64_t);
> > +	}
> > +
> > +	return RTE_VHOST_MSG_RESULT_REPLY;
> > +}
> > +
> > +static int
> > +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> > +		      int main_fd __rte_unused)
> > +{
> > +	struct virtio_net *dev = *pdev;
> > +
> > +	if (!dev->notify_ops->set_config)
> > +		return RTE_VHOST_MSG_RESULT_ERR;
> > +
> > +	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) {
> > +		return RTE_VHOST_MSG_RESULT_ERR;
> > +	}
> > +
> > +	return RTE_VHOST_MSG_RESULT_OK;
> > +}
> > +
> >  /*
> >   * Reallocate virtio_dev and vhost_virtqueue data structure to make them on
> the
> >   * same numa node as the memory of vring descriptor.
> > @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct
> virtio_net **pdev,
> >  	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
> >  	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
> >  	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
> >  	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
> >  	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
> > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> > index 2a650fe..057cdec 100644
> > --- a/lib/librte_vhost/vhost_user.h
> > +++ b/lib/librte_vhost/vhost_user.h
> > @@ -12,6 +12,11 @@
> >
> >  /* refer to hw/virtio/vhost-user.c */
> >
> > +/*
> > + * Maximum size of virtio device config space
> > + */
> > +#define VHOST_USER_MAX_CONFIG_SIZE 256
> > +
> >  #define VHOST_MEMORY_MAX_NREGIONS 8
> >
> >  #define VHOST_USER_PROTOCOL_FEATURES	((1ULL <<
> VHOST_USER_PROTOCOL_F_MQ) | \
> > @@ -49,6 +54,8 @@
> >  	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_CRYPTO_CREATE_SESS = 26,
> >  	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
> >  	VHOST_USER_POSTCOPY_ADVISE = 28,
> > @@ -60,6 +67,7 @@
> >  typedef enum VhostUserSlaveRequest {
> >  	VHOST_USER_SLAVE_NONE = 0,
> >  	VHOST_USER_SLAVE_IOTLB_MSG = 1,
> > +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
> >  	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
> >  	VHOST_USER_SLAVE_MAX
> >  } VhostUserSlaveRequest;
> > @@ -112,6 +120,13 @@
> >  	uint64_t offset;
> >  } VhostUserVringArea;
> >
> > +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 {
> >  		uint32_t master; /* a VhostUserRequest value */
> > @@ -131,6 +146,7 @@
> >  		struct vhost_vring_addr addr;
> >  		VhostUserMemory memory;
> >  		VhostUserLog    log;
> > +		VhostUserConfig config;
> >  		struct vhost_iotlb_msg iotlb;
> >  		VhostUserCryptoSessionParam crypto_session;
> >  		VhostUserVringArea area;
> >
Liu, Changpeng Feb. 26, 2019, 7:02 a.m. UTC | #7 
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Monday, February 25, 2019 9:53 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> Subject: Re: vhost: add virtio configuration space access socket messages
> 
> On 25.02.2019 10:51, Changpeng Liu wrote:
> > This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> > used to get/set virtio device's PCI configuration space.
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > ---
> >  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
> >  lib/librte_vhost/vhost_user.c | 44
> +++++++++++++++++++++++++++++++++++++++++++
> >  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
> >  3 files changed, 68 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index 2753670..ab710ce 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -63,6 +63,10 @@
> >  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
> >  #endif
> >
> > +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> > +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> > +#endif
> > +
> >  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
> >  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
> >  #endif
> > @@ -173,6 +177,10 @@ struct vhost_device_ops {
> >
> >  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);
> 	/**< triggered when a vring is enabled or disabled */
> >
> > +	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);
> 
> 'struct vhost_device_ops' is user visible. This changes API and ABI.
> You need to update docs/rel_notes and bump the library version accordingly.
Sounds good.
> 
> Best regards, Ilya Maximets.
Ilya Maximets Feb. 26, 2019, 7:39 a.m. UTC | #8 
On 26.02.2019 10:01, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Monday, February 25, 2019 9:20 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>> Subject: Re: vhost: add virtio configuration space access socket messages
>>
>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>> used to get/set virtio device's PCI configuration space.
>>
>> Beside the fact that some additional description and reasoning required,
>> I do not see the usage of this feature. You're defining the flag
>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk vhost
>> backends (vdpa, vhost-user) will use this feature.
>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>
>> From the other side, current implementation forces application to properly
>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>> disconnection.
>> This looks strange, because supported protocol features normally enabled by
>> default. Am I misunderstood something ?
> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG wasn't enabled.

So, you're going to enable it only by explicit call to
'rte_vhost_driver_set_features' ?

In this case I'm assuming that you're implementing your own vhost backend.
But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
'vhost_crypto' backend ?


>>
>> One more thing. According to spec: "vhost-user slave uses zero length of
>> payload to indicate an error to vhost-user master". However, current version
>> of 'vhost_user_get_config' replies with size equal to 'sizeof(uint64_t)'.
> Yeah, can be changed to 0 here, QEMU can process it.
>>
>> Best regards, Ilya Maximets.
>>
>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>> Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
>>> ---
>>>  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
>>>  lib/librte_vhost/vhost_user.c | 44
>> +++++++++++++++++++++++++++++++++++++++++++
>>>  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
>>>  3 files changed, 68 insertions(+)
>>>
>>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>>> index 2753670..ab710ce 100644
>>> --- a/lib/librte_vhost/rte_vhost.h
>>> +++ b/lib/librte_vhost/rte_vhost.h
>>> @@ -63,6 +63,10 @@
>>>  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
>>>  #endif
>>>
>>> +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
>>> +#define VHOST_USER_PROTOCOL_F_CONFIG 9
>>> +#endif
>>> +
>>>  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>>>  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
>>>  #endif
>>> @@ -173,6 +177,10 @@ struct vhost_device_ops {
>>>
>>>  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);
>> 	/**< triggered when a vring is enabled or disabled */
>>>
>>> +	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);
>>> +
>>>  	/**
>>>  	 * Features could be changed after the feature negotiation.
>>>  	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index b086ad9..8e42734 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -73,6 +73,8 @@
>>>  	[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",
>>>  	[VHOST_USER_CRYPTO_CREATE_SESS] =
>> "VHOST_USER_CRYPTO_CREATE_SESS",
>>>  	[VHOST_USER_CRYPTO_CLOSE_SESS] =
>> "VHOST_USER_CRYPTO_CLOSE_SESS",
>>>  	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
>>> @@ -359,6 +361,46 @@
>>>  	return RTE_VHOST_MSG_RESULT_OK;
>>>  }
>>>
>>> +static int
>>> +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>> +		      int main_fd __rte_unused)
>>> +{
>>> +	struct virtio_net *dev = *pdev;
>>> +
>>> +	if (!dev->notify_ops->get_config) {
>>> +		msg->size = sizeof(uint64_t);
>>> +		return RTE_VHOST_MSG_RESULT_REPLY;
>>> +	}
>>> +
>>> +	if (dev->notify_ops->get_config(dev->vid,
>>> +					msg->payload.config.region,
>>> +					msg->payload.config.size) != 0) {
>>> +		msg->size = sizeof(uint64_t);
>>> +	}
>>> +
>>> +	return RTE_VHOST_MSG_RESULT_REPLY;
>>> +}
>>> +
>>> +static int
>>> +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>> +		      int main_fd __rte_unused)
>>> +{
>>> +	struct virtio_net *dev = *pdev;
>>> +
>>> +	if (!dev->notify_ops->set_config)
>>> +		return RTE_VHOST_MSG_RESULT_ERR;
>>> +
>>> +	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) {
>>> +		return RTE_VHOST_MSG_RESULT_ERR;
>>> +	}
>>> +
>>> +	return RTE_VHOST_MSG_RESULT_OK;
>>> +}
>>> +
>>>  /*
>>>   * Reallocate virtio_dev and vhost_virtqueue data structure to make them on
>> the
>>>   * same numa node as the memory of vring descriptor.
>>> @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct
>> virtio_net **pdev,
>>>  	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>>>  	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
>>>  	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
>>>  	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
>>>  	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
>>> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
>>> index 2a650fe..057cdec 100644
>>> --- a/lib/librte_vhost/vhost_user.h
>>> +++ b/lib/librte_vhost/vhost_user.h
>>> @@ -12,6 +12,11 @@
>>>
>>>  /* refer to hw/virtio/vhost-user.c */
>>>
>>> +/*
>>> + * Maximum size of virtio device config space
>>> + */
>>> +#define VHOST_USER_MAX_CONFIG_SIZE 256
>>> +
>>>  #define VHOST_MEMORY_MAX_NREGIONS 8
>>>
>>>  #define VHOST_USER_PROTOCOL_FEATURES	((1ULL <<
>> VHOST_USER_PROTOCOL_F_MQ) | \
>>> @@ -49,6 +54,8 @@
>>>  	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_CRYPTO_CREATE_SESS = 26,
>>>  	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
>>>  	VHOST_USER_POSTCOPY_ADVISE = 28,
>>> @@ -60,6 +67,7 @@
>>>  typedef enum VhostUserSlaveRequest {
>>>  	VHOST_USER_SLAVE_NONE = 0,
>>>  	VHOST_USER_SLAVE_IOTLB_MSG = 1,
>>> +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
>>>  	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
>>>  	VHOST_USER_SLAVE_MAX
>>>  } VhostUserSlaveRequest;
>>> @@ -112,6 +120,13 @@
>>>  	uint64_t offset;
>>>  } VhostUserVringArea;
>>>
>>> +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 {
>>>  		uint32_t master; /* a VhostUserRequest value */
>>> @@ -131,6 +146,7 @@
>>>  		struct vhost_vring_addr addr;
>>>  		VhostUserMemory memory;
>>>  		VhostUserLog    log;
>>> +		VhostUserConfig config;
>>>  		struct vhost_iotlb_msg iotlb;
>>>  		VhostUserCryptoSessionParam crypto_session;
>>>  		VhostUserVringArea area;
>>>
Liu, Changpeng Feb. 26, 2019, 8:13 a.m. UTC | #9 
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Tuesday, February 26, 2019 3:39 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> Subject: Re: vhost: add virtio configuration space access socket messages
> 
> On 26.02.2019 10:01, Liu, Changpeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >> Sent: Monday, February 25, 2019 9:20 PM
> >> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> >> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> >> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> >> Subject: Re: vhost: add virtio configuration space access socket messages
> >>
> >> On 25.02.2019 10:51, Changpeng Liu wrote:
> >>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> >>> used to get/set virtio device's PCI configuration space.
> >>
> >> Beside the fact that some additional description and reasoning required,
> >> I do not see the usage of this feature. You're defining the flag
> >> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
> vhost
> >> backends (vdpa, vhost-user) will use this feature.
> >> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
> >> VDPA_SUPPORTED_PROTOCOL_FEATURES.
> >>
> >> From the other side, current implementation forces application to properly
> >> implement the get/set_config callbacks. Otherwise, receiving of the messages
> >> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
> >> disconnection.
> >> This looks strange, because supported protocol features normally enabled by
> >> default. Am I misunderstood something ?
> > QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
> wasn't enabled.
> 
> So, you're going to enable it only by explicit call to
> 'rte_vhost_driver_set_features' ?
> 
> In this case I'm assuming that you're implementing your own vhost backend.
> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
> 'vhost_crypto' backend ?
The patch was developed one year ago, while DPDK didn't have external ops.
The get_config/set_config was defined for all the virtio devices, so I think it makes
more sense adding here.
> 
> 
> >>
> >> One more thing. According to spec: "vhost-user slave uses zero length of
> >> payload to indicate an error to vhost-user master". However, current version
> >> of 'vhost_user_get_config' replies with size equal to 'sizeof(uint64_t)'.
> > Yeah, can be changed to 0 here, QEMU can process it.
> >>
> >> Best regards, Ilya Maximets.
> >>
> >>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> >>> Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> >>> ---
> >>>  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
> >>>  lib/librte_vhost/vhost_user.c | 44
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>>  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
> >>>  3 files changed, 68 insertions(+)
> >>>
> >>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> >>> index 2753670..ab710ce 100644
> >>> --- a/lib/librte_vhost/rte_vhost.h
> >>> +++ b/lib/librte_vhost/rte_vhost.h
> >>> @@ -63,6 +63,10 @@
> >>>  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
> >>>  #endif
> >>>
> >>> +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
> >>> +#define VHOST_USER_PROTOCOL_F_CONFIG 9
> >>> +#endif
> >>> +
> >>>  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
> >>>  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
> >>>  #endif
> >>> @@ -173,6 +177,10 @@ struct vhost_device_ops {
> >>>
> >>>  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);
> >> 	/**< triggered when a vring is enabled or disabled */
> >>>
> >>> +	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);
> >>> +
> >>>  	/**
> >>>  	 * Features could be changed after the feature negotiation.
> >>>  	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
> >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >>> index b086ad9..8e42734 100644
> >>> --- a/lib/librte_vhost/vhost_user.c
> >>> +++ b/lib/librte_vhost/vhost_user.c
> >>> @@ -73,6 +73,8 @@
> >>>  	[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",
> >>>  	[VHOST_USER_CRYPTO_CREATE_SESS] =
> >> "VHOST_USER_CRYPTO_CREATE_SESS",
> >>>  	[VHOST_USER_CRYPTO_CLOSE_SESS] =
> >> "VHOST_USER_CRYPTO_CLOSE_SESS",
> >>>  	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
> >>> @@ -359,6 +361,46 @@
> >>>  	return RTE_VHOST_MSG_RESULT_OK;
> >>>  }
> >>>
> >>> +static int
> >>> +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> >>> +		      int main_fd __rte_unused)
> >>> +{
> >>> +	struct virtio_net *dev = *pdev;
> >>> +
> >>> +	if (!dev->notify_ops->get_config) {
> >>> +		msg->size = sizeof(uint64_t);
> >>> +		return RTE_VHOST_MSG_RESULT_REPLY;
> >>> +	}
> >>> +
> >>> +	if (dev->notify_ops->get_config(dev->vid,
> >>> +					msg->payload.config.region,
> >>> +					msg->payload.config.size) != 0) {
> >>> +		msg->size = sizeof(uint64_t);
> >>> +	}
> >>> +
> >>> +	return RTE_VHOST_MSG_RESULT_REPLY;
> >>> +}
> >>> +
> >>> +static int
> >>> +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
> >>> +		      int main_fd __rte_unused)
> >>> +{
> >>> +	struct virtio_net *dev = *pdev;
> >>> +
> >>> +	if (!dev->notify_ops->set_config)
> >>> +		return RTE_VHOST_MSG_RESULT_ERR;
> >>> +
> >>> +	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) {
> >>> +		return RTE_VHOST_MSG_RESULT_ERR;
> >>> +	}
> >>> +
> >>> +	return RTE_VHOST_MSG_RESULT_OK;
> >>> +}
> >>> +
> >>>  /*
> >>>   * Reallocate virtio_dev and vhost_virtqueue data structure to make them
> on
> >> the
> >>>   * same numa node as the memory of vring descriptor.
> >>> @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct
> >> virtio_net **pdev,
> >>>  	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
> >>>  	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
> >>>  	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
> >>>  	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
> >>>  	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
> >>> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> >>> index 2a650fe..057cdec 100644
> >>> --- a/lib/librte_vhost/vhost_user.h
> >>> +++ b/lib/librte_vhost/vhost_user.h
> >>> @@ -12,6 +12,11 @@
> >>>
> >>>  /* refer to hw/virtio/vhost-user.c */
> >>>
> >>> +/*
> >>> + * Maximum size of virtio device config space
> >>> + */
> >>> +#define VHOST_USER_MAX_CONFIG_SIZE 256
> >>> +
> >>>  #define VHOST_MEMORY_MAX_NREGIONS 8
> >>>
> >>>  #define VHOST_USER_PROTOCOL_FEATURES	((1ULL <<
> >> VHOST_USER_PROTOCOL_F_MQ) | \
> >>> @@ -49,6 +54,8 @@
> >>>  	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_CRYPTO_CREATE_SESS = 26,
> >>>  	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
> >>>  	VHOST_USER_POSTCOPY_ADVISE = 28,
> >>> @@ -60,6 +67,7 @@
> >>>  typedef enum VhostUserSlaveRequest {
> >>>  	VHOST_USER_SLAVE_NONE = 0,
> >>>  	VHOST_USER_SLAVE_IOTLB_MSG = 1,
> >>> +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
> >>>  	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
> >>>  	VHOST_USER_SLAVE_MAX
> >>>  } VhostUserSlaveRequest;
> >>> @@ -112,6 +120,13 @@
> >>>  	uint64_t offset;
> >>>  } VhostUserVringArea;
> >>>
> >>> +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 {
> >>>  		uint32_t master; /* a VhostUserRequest value */
> >>> @@ -131,6 +146,7 @@
> >>>  		struct vhost_vring_addr addr;
> >>>  		VhostUserMemory memory;
> >>>  		VhostUserLog    log;
> >>> +		VhostUserConfig config;
> >>>  		struct vhost_iotlb_msg iotlb;
> >>>  		VhostUserCryptoSessionParam crypto_session;
> >>>  		VhostUserVringArea area;
> >>>
Ilya Maximets Feb. 26, 2019, 8:42 a.m. UTC | #10 
On 26.02.2019 11:13, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Tuesday, February 26, 2019 3:39 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>> Subject: Re: vhost: add virtio configuration space access socket messages
>>
>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>
>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>> used to get/set virtio device's PCI configuration space.
>>>>
>>>> Beside the fact that some additional description and reasoning required,
>>>> I do not see the usage of this feature. You're defining the flag
>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>> vhost
>>>> backends (vdpa, vhost-user) will use this feature.
>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>
>>>> From the other side, current implementation forces application to properly
>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>> disconnection.
>>>> This looks strange, because supported protocol features normally enabled by
>>>> default. Am I misunderstood something ?
>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>> wasn't enabled.
>>
>> So, you're going to enable it only by explicit call to
>> 'rte_vhost_driver_set_features' ?
>>
>> In this case I'm assuming that you're implementing your own vhost backend.
>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>> 'vhost_crypto' backend ?
> The patch was developed one year ago, while DPDK didn't have external ops.

So, maybe it's time to reconsider the implementation.

> The get_config/set_config was defined for all the virtio devices, so I think it makes
> more sense adding here.

VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
too, however they makes sense for vhost_crypto backend only. These messages
(GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
implemented, so IMHO it's better to implement their handlers along with the
callbacks, i.e. inside the implementation of your vhost backend.

Maxime, Tiwei, what do you think ?

>>
>>
>>>>
>>>> One more thing. According to spec: "vhost-user slave uses zero length of
>>>> payload to indicate an error to vhost-user master". However, current version
>>>> of 'vhost_user_get_config' replies with size equal to 'sizeof(uint64_t)'.
>>> Yeah, can be changed to 0 here, QEMU can process it.
>>>>
>>>> Best regards, Ilya Maximets.
>>>>
>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>>> Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
>>>>> ---
>>>>>  lib/librte_vhost/rte_vhost.h  |  8 ++++++++
>>>>>  lib/librte_vhost/vhost_user.c | 44
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>  lib/librte_vhost/vhost_user.h | 16 ++++++++++++++++
>>>>>  3 files changed, 68 insertions(+)
>>>>>
>>>>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>>>>> index 2753670..ab710ce 100644
>>>>> --- a/lib/librte_vhost/rte_vhost.h
>>>>> +++ b/lib/librte_vhost/rte_vhost.h
>>>>> @@ -63,6 +63,10 @@
>>>>>  #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
>>>>>  #endif
>>>>>
>>>>> +#ifndef VHOST_USER_PROTOCOL_F_CONFIG
>>>>> +#define VHOST_USER_PROTOCOL_F_CONFIG 9
>>>>> +#endif
>>>>> +
>>>>>  #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>>>>>  #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
>>>>>  #endif
>>>>> @@ -173,6 +177,10 @@ struct vhost_device_ops {
>>>>>
>>>>>  	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);
>>>> 	/**< triggered when a vring is enabled or disabled */
>>>>>
>>>>> +	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);
>>>>> +
>>>>>  	/**
>>>>>  	 * Features could be changed after the feature negotiation.
>>>>>  	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
>>>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>>>> index b086ad9..8e42734 100644
>>>>> --- a/lib/librte_vhost/vhost_user.c
>>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>>> @@ -73,6 +73,8 @@
>>>>>  	[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",
>>>>>  	[VHOST_USER_CRYPTO_CREATE_SESS] =
>>>> "VHOST_USER_CRYPTO_CREATE_SESS",
>>>>>  	[VHOST_USER_CRYPTO_CLOSE_SESS] =
>>>> "VHOST_USER_CRYPTO_CLOSE_SESS",
>>>>>  	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
>>>>> @@ -359,6 +361,46 @@
>>>>>  	return RTE_VHOST_MSG_RESULT_OK;
>>>>>  }
>>>>>
>>>>> +static int
>>>>> +vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>>>> +		      int main_fd __rte_unused)
>>>>> +{
>>>>> +	struct virtio_net *dev = *pdev;
>>>>> +
>>>>> +	if (!dev->notify_ops->get_config) {
>>>>> +		msg->size = sizeof(uint64_t);
>>>>> +		return RTE_VHOST_MSG_RESULT_REPLY;
>>>>> +	}
>>>>> +
>>>>> +	if (dev->notify_ops->get_config(dev->vid,
>>>>> +					msg->payload.config.region,
>>>>> +					msg->payload.config.size) != 0) {
>>>>> +		msg->size = sizeof(uint64_t);
>>>>> +	}
>>>>> +
>>>>> +	return RTE_VHOST_MSG_RESULT_REPLY;
>>>>> +}
>>>>> +
>>>>> +static int
>>>>> +vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>>>> +		      int main_fd __rte_unused)
>>>>> +{
>>>>> +	struct virtio_net *dev = *pdev;
>>>>> +
>>>>> +	if (!dev->notify_ops->set_config)
>>>>> +		return RTE_VHOST_MSG_RESULT_ERR;
>>>>> +
>>>>> +	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) {
>>>>> +		return RTE_VHOST_MSG_RESULT_ERR;
>>>>> +	}
>>>>> +
>>>>> +	return RTE_VHOST_MSG_RESULT_OK;
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Reallocate virtio_dev and vhost_virtqueue data structure to make them
>> on
>>>> the
>>>>>   * same numa node as the memory of vring descriptor.
>>>>> @@ -1725,6 +1767,8 @@ typedef int (*vhost_message_handler_t)(struct
>>>> virtio_net **pdev,
>>>>>  	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>>>>>  	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
>>>>>  	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
>>>>>  	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
>>>>>  	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
>>>>> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
>>>>> index 2a650fe..057cdec 100644
>>>>> --- a/lib/librte_vhost/vhost_user.h
>>>>> +++ b/lib/librte_vhost/vhost_user.h
>>>>> @@ -12,6 +12,11 @@
>>>>>
>>>>>  /* refer to hw/virtio/vhost-user.c */
>>>>>
>>>>> +/*
>>>>> + * Maximum size of virtio device config space
>>>>> + */
>>>>> +#define VHOST_USER_MAX_CONFIG_SIZE 256
>>>>> +
>>>>>  #define VHOST_MEMORY_MAX_NREGIONS 8
>>>>>
>>>>>  #define VHOST_USER_PROTOCOL_FEATURES	((1ULL <<
>>>> VHOST_USER_PROTOCOL_F_MQ) | \
>>>>> @@ -49,6 +54,8 @@
>>>>>  	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_CRYPTO_CREATE_SESS = 26,
>>>>>  	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
>>>>>  	VHOST_USER_POSTCOPY_ADVISE = 28,
>>>>> @@ -60,6 +67,7 @@
>>>>>  typedef enum VhostUserSlaveRequest {
>>>>>  	VHOST_USER_SLAVE_NONE = 0,
>>>>>  	VHOST_USER_SLAVE_IOTLB_MSG = 1,
>>>>> +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
>>>>>  	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
>>>>>  	VHOST_USER_SLAVE_MAX
>>>>>  } VhostUserSlaveRequest;
>>>>> @@ -112,6 +120,13 @@
>>>>>  	uint64_t offset;
>>>>>  } VhostUserVringArea;
>>>>>
>>>>> +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 {
>>>>>  		uint32_t master; /* a VhostUserRequest value */
>>>>> @@ -131,6 +146,7 @@
>>>>>  		struct vhost_vring_addr addr;
>>>>>  		VhostUserMemory memory;
>>>>>  		VhostUserLog    log;
>>>>> +		VhostUserConfig config;
>>>>>  		struct vhost_iotlb_msg iotlb;
>>>>>  		VhostUserCryptoSessionParam crypto_session;
>>>>>  		VhostUserVringArea area;
>>>>>
Maxime Coquelin Feb. 26, 2019, 12:32 p.m. UTC | #11 
On 2/26/19 9:42 AM, Ilya Maximets wrote:
> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>
>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>
>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>
>>>>> Beside the fact that some additional description and reasoning required,
>>>>> I do not see the usage of this feature. You're defining the flag
>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>>> vhost
>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>
>>>>>  From the other side, current implementation forces application to properly
>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>> disconnection.
>>>>> This looks strange, because supported protocol features normally enabled by
>>>>> default. Am I misunderstood something ?
>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>> wasn't enabled.
>>>
>>> So, you're going to enable it only by explicit call to
>>> 'rte_vhost_driver_set_features' ?
>>>
>>> In this case I'm assuming that you're implementing your own vhost backend.
>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>> 'vhost_crypto' backend ?
>> The patch was developed one year ago, while DPDK didn't have external ops.
> 
> So, maybe it's time to reconsider the implementation.

+1

>> The get_config/set_config was defined for all the virtio devices, so I think it makes
>> more sense adding here.
> 
> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
> too, however they makes sense for vhost_crypto backend only. These messages
> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
> implemented, so IMHO it's better to implement their handlers along with the
> callbacks, i.e. inside the implementation of your vhost backend.
> 
> Maxime, Tiwei, what do you think ?

I would prefer it to be implemented in SPDK directly as a pre_handler
callback, as I don't foresee a need for it for other backends, and it
would avoid breaking the API.

It would imply fixing the beginning of vhost_user_msg_handler() to 
accept requests > VHOST_USER_MAX and add necessary check before doing
the debug logs.

With above change we would also be able to remove VHOST_CRYPTO requests
from vhost_user.c, and we could then work on moving vhost-net bits
out of this file too.

Regards,
Maxime
Ilya Maximets Feb. 26, 2019, 1:36 p.m. UTC | #12 
On 26.02.2019 15:32, Maxime Coquelin wrote:
> 
> 
> On 2/26/19 9:42 AM, Ilya Maximets wrote:
>> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>
>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>
>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>>
>>>>>> Beside the fact that some additional description and reasoning required,
>>>>>> I do not see the usage of this feature. You're defining the flag
>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>>>> vhost
>>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>>
>>>>>>  From the other side, current implementation forces application to properly
>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>>> disconnection.
>>>>>> This looks strange, because supported protocol features normally enabled by
>>>>>> default. Am I misunderstood something ?
>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>>> wasn't enabled.
>>>>
>>>> So, you're going to enable it only by explicit call to
>>>> 'rte_vhost_driver_set_features' ?
>>>>
>>>> In this case I'm assuming that you're implementing your own vhost backend.
>>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>>> 'vhost_crypto' backend ?
>>> The patch was developed one year ago, while DPDK didn't have external ops.
>>
>> So, maybe it's time to reconsider the implementation.
> 
> +1
> 
>>> The get_config/set_config was defined for all the virtio devices, so I think it makes
>>> more sense adding here.
>>
>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
>> too, however they makes sense for vhost_crypto backend only. These messages
>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
>> implemented, so IMHO it's better to implement their handlers along with the
>> callbacks, i.e. inside the implementation of your vhost backend.
>>
>> Maxime, Tiwei, what do you think ?
> 
> I would prefer it to be implemented in SPDK directly as a pre_handler
> callback, as I don't foresee a need for it for other backends, and it
> would avoid breaking the API.
> 
> It would imply fixing the beginning of vhost_user_msg_handler() to accept requests > VHOST_USER_MAX and add necessary check before doing
> the debug logs.

VHOST_USER_MAX is 31 and both new requests are
defined in the same enum VhostUserRequest:

	VHOST_USER_GET_CONFIG = 24,
	VHOST_USER_SET_CONFIG = 25

I don't think that any change is needed here.

> 
> With above change we would also be able to remove VHOST_CRYPTO requests
> from vhost_user.c,

Maybe you're looking at the different git HEAD ? I don't see any crypto
related code in vhost_user.c. Only name definition in vhost_message_str.

> and we could then work on moving vhost-net bits
> out of this file too.
> 
> Regards,
> Maxime
> 
> 
>
Maxime Coquelin Feb. 26, 2019, 1:43 p.m. UTC | #13 
On 2/26/19 2:36 PM, Ilya Maximets wrote:
> On 26.02.2019 15:32, Maxime Coquelin wrote:
>>
>>
>> On 2/26/19 9:42 AM, Ilya Maximets wrote:
>>> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>
>>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>
>>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>>>
>>>>>>> Beside the fact that some additional description and reasoning required,
>>>>>>> I do not see the usage of this feature. You're defining the flag
>>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>>>>> vhost
>>>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>>>
>>>>>>>   From the other side, current implementation forces application to properly
>>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>>>> disconnection.
>>>>>>> This looks strange, because supported protocol features normally enabled by
>>>>>>> default. Am I misunderstood something ?
>>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>>>> wasn't enabled.
>>>>>
>>>>> So, you're going to enable it only by explicit call to
>>>>> 'rte_vhost_driver_set_features' ?
>>>>>
>>>>> In this case I'm assuming that you're implementing your own vhost backend.
>>>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>>>> 'vhost_crypto' backend ?
>>>> The patch was developed one year ago, while DPDK didn't have external ops.
>>>
>>> So, maybe it's time to reconsider the implementation.
>>
>> +1
>>
>>>> The get_config/set_config was defined for all the virtio devices, so I think it makes
>>>> more sense adding here.
>>>
>>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
>>> too, however they makes sense for vhost_crypto backend only. These messages
>>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
>>> implemented, so IMHO it's better to implement their handlers along with the
>>> callbacks, i.e. inside the implementation of your vhost backend.
>>>
>>> Maxime, Tiwei, what do you think ?
>>
>> I would prefer it to be implemented in SPDK directly as a pre_handler
>> callback, as I don't foresee a need for it for other backends, and it
>> would avoid breaking the API.
>>
>> It would imply fixing the beginning of vhost_user_msg_handler() to accept requests > VHOST_USER_MAX and add necessary check before doing
>> the debug logs.
> 
> VHOST_USER_MAX is 31 and both new requests are
> defined in the same enum VhostUserRequest:
> 
> 	VHOST_USER_GET_CONFIG = 24,
> 	VHOST_USER_SET_CONFIG = 25
> 
> I don't think that any change is needed here.

I didn't meant GET_SET_CONFIG specifically. I meant that if we want
something really generic, we would need to do that.

BTW, it would crash as vhost_message_str[VHOST_USER_GET/SET_CONFIG] 
would not be defined.

> 
>>
>> With above change we would also be able to remove VHOST_CRYPTO requests
>> from vhost_user.c,
> 
> Maybe you're looking at the different git HEAD ? I don't see any crypto
> related code in vhost_user.c. Only name definition in vhost_message_str.

Yes, I meant removing their definition in vhost_message_str[].

My point is that if we want to have external backends to handle their
specific requests, we should not have to modify vhost_user.c as it
creates a useless dependency.

>> and we could then work on moving vhost-net bits
>> out of this file too.
>>
>> Regards,
>> Maxime
>>
>>
>>
Ilya Maximets Feb. 26, 2019, 2:07 p.m. UTC | #14 
On 26.02.2019 16:43, Maxime Coquelin wrote:
> 
> 
> On 2/26/19 2:36 PM, Ilya Maximets wrote:
>> On 26.02.2019 15:32, Maxime Coquelin wrote:
>>>
>>>
>>> On 2/26/19 9:42 AM, Ilya Maximets wrote:
>>>> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>
>>>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>>
>>>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>>>>
>>>>>>>> Beside the fact that some additional description and reasoning required,
>>>>>>>> I do not see the usage of this feature. You're defining the flag
>>>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>>>>>> vhost
>>>>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>>>>
>>>>>>>>   From the other side, current implementation forces application to properly
>>>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>>>>> disconnection.
>>>>>>>> This looks strange, because supported protocol features normally enabled by
>>>>>>>> default. Am I misunderstood something ?
>>>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>>>>> wasn't enabled.
>>>>>>
>>>>>> So, you're going to enable it only by explicit call to
>>>>>> 'rte_vhost_driver_set_features' ?
>>>>>>
>>>>>> In this case I'm assuming that you're implementing your own vhost backend.
>>>>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>>>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>>>>> 'vhost_crypto' backend ?
>>>>> The patch was developed one year ago, while DPDK didn't have external ops.
>>>>
>>>> So, maybe it's time to reconsider the implementation.
>>>
>>> +1
>>>
>>>>> The get_config/set_config was defined for all the virtio devices, so I think it makes
>>>>> more sense adding here.
>>>>
>>>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
>>>> too, however they makes sense for vhost_crypto backend only. These messages
>>>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
>>>> implemented, so IMHO it's better to implement their handlers along with the
>>>> callbacks, i.e. inside the implementation of your vhost backend.
>>>>
>>>> Maxime, Tiwei, what do you think ?
>>>
>>> I would prefer it to be implemented in SPDK directly as a pre_handler
>>> callback, as I don't foresee a need for it for other backends, and it
>>> would avoid breaking the API.
>>>
>>> It would imply fixing the beginning of vhost_user_msg_handler() to accept requests > VHOST_USER_MAX and add necessary check before doing
>>> the debug logs.
>>
>> VHOST_USER_MAX is 31 and both new requests are
>> defined in the same enum VhostUserRequest:
>>
>>     VHOST_USER_GET_CONFIG = 24,
>>     VHOST_USER_SET_CONFIG = 25
>>
>> I don't think that any change is needed here.
> 
> I didn't meant GET_SET_CONFIG specifically. I meant that if we want
> something really generic, we would need to do that.

OK. I understand now.

> 
> BTW, it would crash as vhost_message_str[VHOST_USER_GET/SET_CONFIG] would not be defined.
> 
>>
>>>
>>> With above change we would also be able to remove VHOST_CRYPTO requests
>>> from vhost_user.c,
>>
>> Maybe you're looking at the different git HEAD ? I don't see any crypto
>> related code in vhost_user.c. Only name definition in vhost_message_str.
> 
> Yes, I meant removing their definition in vhost_message_str[].
> 
> My point is that if we want to have external backends to handle their
> specific requests, we should not have to modify vhost_user.c as it
> creates a useless dependency.

That's a good point. I agree.

Maybe we'll need some new API to make vhost library more dynamic?
Something like
    rte_vhost_message_register(enum VhostUserRequest request,
                               const char *resuest_str,
                               vhost_message_handler_t handler);
This could be flexible.

> 
>>> and we could then work on moving vhost-net bits
>>> out of this file too.
>>>
>>> Regards,
>>> Maxime
>>>
>>>
>>>
> 
>
Liu, Changpeng Feb. 27, 2019, 1:31 a.m. UTC | #15 
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Tuesday, February 26, 2019 8:32 PM
> To: Ilya Maximets <i.maximets@samsung.com>; Liu, Changpeng
> <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Jason Wang
> <jasowang@redhat.com>
> Subject: Re: vhost: add virtio configuration space access socket messages
> 
> 
> 
> On 2/26/19 9:42 AM, Ilya Maximets wrote:
> > On 26.02.2019 11:13, Liu, Changpeng wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >>> Sent: Tuesday, February 26, 2019 3:39 PM
> >>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> >>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> >>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> >>> Subject: Re: vhost: add virtio configuration space access socket messages
> >>>
> >>> On 26.02.2019 10:01, Liu, Changpeng wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >>>>> Sent: Monday, February 25, 2019 9:20 PM
> >>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> >>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> >>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> >>>>> Subject: Re: vhost: add virtio configuration space access socket messages
> >>>>>
> >>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
> >>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> >>>>>> used to get/set virtio device's PCI configuration space.
> >>>>>
> >>>>> Beside the fact that some additional description and reasoning required,
> >>>>> I do not see the usage of this feature. You're defining the flag
> >>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of
> dpdk
> >>> vhost
> >>>>> backends (vdpa, vhost-user) will use this feature.
> >>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
> >>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
> >>>>>
> >>>>>  From the other side, current implementation forces application to
> properly
> >>>>> implement the get/set_config callbacks. Otherwise, receiving of the
> messages
> >>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
> >>>>> disconnection.
> >>>>> This looks strange, because supported protocol features normally enabled
> by
> >>>>> default. Am I misunderstood something ?
> >>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
> >>> wasn't enabled.
> >>>
> >>> So, you're going to enable it only by explicit call to
> >>> 'rte_vhost_driver_set_features' ?
> >>>
> >>> In this case I'm assuming that you're implementing your own vhost backend.
> >>> But why you're not using 'dev->extern_ops' and corresponding
> 'pre_msg_handle'
> >>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
> >>> 'vhost_crypto' backend ?
> >> The patch was developed one year ago, while DPDK didn't have external ops.
> >
> > So, maybe it's time to reconsider the implementation.
> 
> +1
Okay, I will only add related messages definition in this patch and remove the
Callbacks.
> 
> >> The get_config/set_config was defined for all the virtio devices, so I think it
> makes
> >> more sense adding here.
> >
> > VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio
> devices
> > too, however they makes sense for vhost_crypto backend only. These
> messages
> > (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
> > implemented, so IMHO it's better to implement their handlers along with the
> > callbacks, i.e. inside the implementation of your vhost backend.
> >
> > Maxime, Tiwei, what do you think ?
> 
> I would prefer it to be implemented in SPDK directly as a pre_handler
> callback, as I don't foresee a need for it for other backends, and it
> would avoid breaking the API.
> 
> It would imply fixing the beginning of vhost_user_msg_handler() to
> accept requests > VHOST_USER_MAX and add necessary check before doing
> the debug logs.
> 
> With above change we would also be able to remove VHOST_CRYPTO requests
> from vhost_user.c, and we could then work on moving vhost-net bits
> out of this file too.
> 
> Regards,
> Maxime
Tiwei Bie Feb. 27, 2019, 1:41 a.m. UTC | #16 
On Tue, Feb 26, 2019 at 01:32:24PM +0100, Maxime Coquelin wrote:
> On 2/26/19 9:42 AM, Ilya Maximets wrote:
> > On 26.02.2019 11:13, Liu, Changpeng wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Ilya Maximets [mailto:i.maximets@samsung.com]
> > > > Sent: Tuesday, February 26, 2019 3:39 PM
> > > > To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> > > > Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> > > > maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> > > > Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> > > > Subject: Re: vhost: add virtio configuration space access socket messages
> > > > 
> > > > On 26.02.2019 10:01, Liu, Changpeng wrote:
> > > > > 
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Ilya Maximets [mailto:i.maximets@samsung.com]
> > > > > > Sent: Monday, February 25, 2019 9:20 PM
> > > > > > To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> > > > > > Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> > > > > > maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> > > > > > Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> > > > > > Subject: Re: vhost: add virtio configuration space access socket messages
> > > > > > 
> > > > > > On 25.02.2019 10:51, Changpeng Liu wrote:
> > > > > > > This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
> > > > > > > used to get/set virtio device's PCI configuration space.
> > > > > > 
> > > > > > Beside the fact that some additional description and reasoning required,
> > > > > > I do not see the usage of this feature. You're defining the flag
> > > > > > VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
> > > > vhost
> > > > > > backends (vdpa, vhost-user) will use this feature.
> > > > > > You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
> > > > > > VDPA_SUPPORTED_PROTOCOL_FEATURES.
> > > > > > 
> > > > > >  From the other side, current implementation forces application to properly
> > > > > > implement the get/set_config callbacks. Otherwise, receiving of the messages
> > > > > > will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
> > > > > > disconnection.
> > > > > > This looks strange, because supported protocol features normally enabled by
> > > > > > default. Am I misunderstood something ?
> > > > > QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
> > > > wasn't enabled.
> > > > 
> > > > So, you're going to enable it only by explicit call to
> > > > 'rte_vhost_driver_set_features' ?
> > > > 
> > > > In this case I'm assuming that you're implementing your own vhost backend.
> > > > But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
> > > > or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
> > > > 'vhost_crypto' backend ?
> > > The patch was developed one year ago, while DPDK didn't have external ops.
> > 
> > So, maybe it's time to reconsider the implementation.
> 
> +1
> 
> > > The get_config/set_config was defined for all the virtio devices, so I think it makes
> > > more sense adding here.
> > 
> > VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
> > too, however they makes sense for vhost_crypto backend only. These messages
> > (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
> > implemented, so IMHO it's better to implement their handlers along with the
> > callbacks, i.e. inside the implementation of your vhost backend.
> > 
> > Maxime, Tiwei, what do you think ?
> 
> I would prefer it to be implemented in SPDK directly as a pre_handler
> callback, as I don't foresee a need for it for other backends, and it
> would avoid breaking the API.
> 
> It would imply fixing the beginning of vhost_user_msg_handler() to accept
> requests > VHOST_USER_MAX and add necessary check before doing
> the debug logs.
> 
> With above change we would also be able to remove VHOST_CRYPTO requests
> from vhost_user.c, and we could then work on moving vhost-net bits
> out of this file too.

+1

> 
> Regards,
> Maxime
>
Maxime Coquelin Feb. 27, 2019, 9:04 a.m. UTC | #17 
On 2/26/19 3:07 PM, Ilya Maximets wrote:
> On 26.02.2019 16:43, Maxime Coquelin wrote:
>>
>>
>> On 2/26/19 2:36 PM, Ilya Maximets wrote:
>>> On 26.02.2019 15:32, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 2/26/19 9:42 AM, Ilya Maximets wrote:
>>>>> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>
>>>>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>>>
>>>>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>>>>>
>>>>>>>>> Beside the fact that some additional description and reasoning required,
>>>>>>>>> I do not see the usage of this feature. You're defining the flag
>>>>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>>>>>>> vhost
>>>>>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>>>>>
>>>>>>>>>    From the other side, current implementation forces application to properly
>>>>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>>>>>> disconnection.
>>>>>>>>> This looks strange, because supported protocol features normally enabled by
>>>>>>>>> default. Am I misunderstood something ?
>>>>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>>>>>> wasn't enabled.
>>>>>>>
>>>>>>> So, you're going to enable it only by explicit call to
>>>>>>> 'rte_vhost_driver_set_features' ?
>>>>>>>
>>>>>>> In this case I'm assuming that you're implementing your own vhost backend.
>>>>>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>>>>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>>>>>> 'vhost_crypto' backend ?
>>>>>> The patch was developed one year ago, while DPDK didn't have external ops.
>>>>>
>>>>> So, maybe it's time to reconsider the implementation.
>>>>
>>>> +1
>>>>
>>>>>> The get_config/set_config was defined for all the virtio devices, so I think it makes
>>>>>> more sense adding here.
>>>>>
>>>>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
>>>>> too, however they makes sense for vhost_crypto backend only. These messages
>>>>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
>>>>> implemented, so IMHO it's better to implement their handlers along with the
>>>>> callbacks, i.e. inside the implementation of your vhost backend.
>>>>>
>>>>> Maxime, Tiwei, what do you think ?
>>>>
>>>> I would prefer it to be implemented in SPDK directly as a pre_handler
>>>> callback, as I don't foresee a need for it for other backends, and it
>>>> would avoid breaking the API.
>>>>
>>>> It would imply fixing the beginning of vhost_user_msg_handler() to accept requests > VHOST_USER_MAX and add necessary check before doing
>>>> the debug logs.
>>>
>>> VHOST_USER_MAX is 31 and both new requests are
>>> defined in the same enum VhostUserRequest:
>>>
>>>      VHOST_USER_GET_CONFIG = 24,
>>>      VHOST_USER_SET_CONFIG = 25
>>>
>>> I don't think that any change is needed here.
>>
>> I didn't meant GET_SET_CONFIG specifically. I meant that if we want
>> something really generic, we would need to do that.
> 
> OK. I understand now.
> 
>>
>> BTW, it would crash as vhost_message_str[VHOST_USER_GET/SET_CONFIG] would not be defined.
>>
>>>
>>>>
>>>> With above change we would also be able to remove VHOST_CRYPTO requests
>>>> from vhost_user.c,
>>>
>>> Maybe you're looking at the different git HEAD ? I don't see any crypto
>>> related code in vhost_user.c. Only name definition in vhost_message_str.
>>
>> Yes, I meant removing their definition in vhost_message_str[].
>>
>> My point is that if we want to have external backends to handle their
>> specific requests, we should not have to modify vhost_user.c as it
>> creates a useless dependency.
> 
> That's a good point. I agree.
> 
> Maybe we'll need some new API to make vhost library more dynamic?
> Something like
>      rte_vhost_message_register(enum VhostUserRequest request,
>                                 const char *resuest_str,
>                                 vhost_message_handler_t handler);
> This could be flexible.

Agree it could be done like that.

Now, I think we have pretty much every thing we need in the API to
implement it, but maybe I'm missing something?

I.e., by implementing the .pre_msg_handle callback and setting its
skip_master to 1, we have the same result. Except that we don't
have the debug message.

Also, it means we would need to either rework all current handlers,
or make "struct virtio_net" part of the API.
So maybe we'll have to come to this, but we would need first to do
a significant rework of the library to move all the net specific
stuff out of the generic vhost part.

Thanks,
Maxime
> 
>>
>>>> and we could then work on moving vhost-net bits
>>>> out of this file too.
>>>>
>>>> Regards,
>>>> Maxime
>>>>
>>>>
>>>>
>>
>>
Maxime Coquelin Feb. 27, 2019, 9:12 a.m. UTC | #18 
On 2/27/19 2:31 AM, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Tuesday, February 26, 2019 8:32 PM
>> To: Ilya Maximets <i.maximets@samsung.com>; Liu, Changpeng
>> <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Jason Wang
>> <jasowang@redhat.com>
>> Subject: Re: vhost: add virtio configuration space access socket messages
>>
>>
>>
>> On 2/26/19 9:42 AM, Ilya Maximets wrote:
>>> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>
>>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>
>>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>>>
>>>>>>> Beside the fact that some additional description and reasoning required,
>>>>>>> I do not see the usage of this feature. You're defining the flag
>>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of
>> dpdk
>>>>> vhost
>>>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>>>
>>>>>>>   From the other side, current implementation forces application to
>> properly
>>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the
>> messages
>>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>>>> disconnection.
>>>>>>> This looks strange, because supported protocol features normally enabled
>> by
>>>>>>> default. Am I misunderstood something ?
>>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>>>> wasn't enabled.
>>>>>
>>>>> So, you're going to enable it only by explicit call to
>>>>> 'rte_vhost_driver_set_features' ?
>>>>>
>>>>> In this case I'm assuming that you're implementing your own vhost backend.
>>>>> But why you're not using 'dev->extern_ops' and corresponding
>> 'pre_msg_handle'
>>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>>>> 'vhost_crypto' backend ?
>>>> The patch was developed one year ago, while DPDK didn't have external ops.
>>>
>>> So, maybe it's time to reconsider the implementation.
>>
>> +1
> Okay, I will only add related messages definition in this patch and remove the
> Callbacks.

What we need to do is fix vhost lib so that you don't have anything to
do in dpdk to add support for the new requests.

So we need to fix the few bits in vhost_user_msg_handler() I mentionned
yesterday. And I also notice we are missing
rte_vhost_driver_set_protocol_feature(), so that you can advertise
VHOST_USER_PROTOCOL_F_CONFIG support by your external backend.

I'll try to cook the dpdk patch today.

Cheers,
Maxime

>>
>>>> The get_config/set_config was defined for all the virtio devices, so I think it
>> makes
>>>> more sense adding here.
>>>
>>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio
>> devices
>>> too, however they makes sense for vhost_crypto backend only. These
>> messages
>>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
>>> implemented, so IMHO it's better to implement their handlers along with the
>>> callbacks, i.e. inside the implementation of your vhost backend.
>>>
>>> Maxime, Tiwei, what do you think ?
>>
>> I would prefer it to be implemented in SPDK directly as a pre_handler
>> callback, as I don't foresee a need for it for other backends, and it
>> would avoid breaking the API.
>>
>> It would imply fixing the beginning of vhost_user_msg_handler() to
>> accept requests > VHOST_USER_MAX and add necessary check before doing
>> the debug logs.
>>
>> With above change we would also be able to remove VHOST_CRYPTO requests
>> from vhost_user.c, and we could then work on moving vhost-net bits
>> out of this file too.
>>
>> Regards,
>> Maxime
>
Liu, Changpeng Feb. 27, 2019, 9:50 a.m. UTC | #19 
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, February 27, 2019 5:12 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; Ilya Maximets
> <i.maximets@samsung.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Jason Wang
> <jasowang@redhat.com>
> Subject: Re: vhost: add virtio configuration space access socket messages
> 
> 
> 
> On 2/27/19 2:31 AM, Liu, Changpeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >> Sent: Tuesday, February 26, 2019 8:32 PM
> >> To: Ilya Maximets <i.maximets@samsung.com>; Liu, Changpeng
> >> <changpeng.liu@intel.com>; dev@dpdk.org
> >> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Bie, Tiwei
> >> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Jason
> Wang
> >> <jasowang@redhat.com>
> >> Subject: Re: vhost: add virtio configuration space access socket messages
> >>
> >>
> >>
> >> On 2/26/19 9:42 AM, Ilya Maximets wrote:
> >>> On 26.02.2019 11:13, Liu, Changpeng wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >>>>> Sent: Tuesday, February 26, 2019 3:39 PM
> >>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> >>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> >>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
> >>>>> Subject: Re: vhost: add virtio configuration space access socket messages
> >>>>>
> >>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >>>>>>> Sent: Monday, February 25, 2019 9:20 PM
> >>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
> >>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> >>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang
> <jasowang@redhat.com>
> >>>>>>> Subject: Re: vhost: add virtio configuration space access socket
> messages
> >>>>>>>
> >>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
> >>>>>>>> This patch adds new vhost user messages GET_CONFIG and
> SET_CONFIG
> >>>>>>>> used to get/set virtio device's PCI configuration space.
> >>>>>>>
> >>>>>>> Beside the fact that some additional description and reasoning required,
> >>>>>>> I do not see the usage of this feature. You're defining the flag
> >>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of
> >> dpdk
> >>>>> vhost
> >>>>>>> backends (vdpa, vhost-user) will use this feature.
> >>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES
> or
> >>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
> >>>>>>>
> >>>>>>>   From the other side, current implementation forces application to
> >> properly
> >>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the
> >> messages
> >>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
> >>>>>>> disconnection.
> >>>>>>> This looks strange, because supported protocol features normally
> enabled
> >> by
> >>>>>>> default. Am I misunderstood something ?
> >>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
> >>>>> wasn't enabled.
> >>>>>
> >>>>> So, you're going to enable it only by explicit call to
> >>>>> 'rte_vhost_driver_set_features' ?
> >>>>>
> >>>>> In this case I'm assuming that you're implementing your own vhost
> backend.
> >>>>> But why you're not using 'dev->extern_ops' and corresponding
> >> 'pre_msg_handle'
> >>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it
> does
> >>>>> 'vhost_crypto' backend ?
> >>>> The patch was developed one year ago, while DPDK didn't have external
> ops.
> >>>
> >>> So, maybe it's time to reconsider the implementation.
> >>
> >> +1
> > Okay, I will only add related messages definition in this patch and remove the
> > Callbacks.
> 
> What we need to do is fix vhost lib so that you don't have anything to
> do in dpdk to add support for the new requests.
> 
> So we need to fix the few bits in vhost_user_msg_handler() I mentionned
> yesterday. And I also notice we are missing
> rte_vhost_driver_set_protocol_feature(), so that you can advertise
> VHOST_USER_PROTOCOL_F_CONFIG support by your external backend.
Sounds good to me, I will not post a new version and wait for the new API.
> 
> I'll try to cook the dpdk patch today.
> 
> Cheers,
> Maxime
> 
> >>
> >>>> The get_config/set_config was defined for all the virtio devices, so I think it
> >> makes
> >>>> more sense adding here.
> >>>
> >>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio
> >> devices
> >>> too, however they makes sense for vhost_crypto backend only. These
> >> messages
> >>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
> >>> implemented, so IMHO it's better to implement their handlers along with
> the
> >>> callbacks, i.e. inside the implementation of your vhost backend.
> >>>
> >>> Maxime, Tiwei, what do you think ?
> >>
> >> I would prefer it to be implemented in SPDK directly as a pre_handler
> >> callback, as I don't foresee a need for it for other backends, and it
> >> would avoid breaking the API.
> >>
> >> It would imply fixing the beginning of vhost_user_msg_handler() to
> >> accept requests > VHOST_USER_MAX and add necessary check before doing
> >> the debug logs.
> >>
> >> With above change we would also be able to remove VHOST_CRYPTO
> requests
> >> from vhost_user.c, and we could then work on moving vhost-net bits
> >> out of this file too.
> >>
> >> Regards,
> >> Maxime
> >
Maxime Coquelin Feb. 27, 2019, 10:04 a.m. UTC | #20 
On 2/27/19 10:50 AM, Liu, Changpeng wrote:
>> What we need to do is fix vhost lib so that you don't have anything to
>> do in dpdk to add support for the new requests.
>>
>> So we need to fix the few bits in vhost_user_msg_handler() I mentionned
>> yesterday. And I also notice we are missing
>> rte_vhost_driver_set_protocol_feature(), so that you can advertise
>> VHOST_USER_PROTOCOL_F_CONFIG support by your external backend.
> Sounds good to me, I will not post a new version and wait for the new API.

I just posted the RFC, could you try it and let us know if it works for
you?

Thanks,
Maxime

>> I'll try to cook the dpdk patch today.
>>
>> Cheers,
Ilya Maximets Feb. 27, 2019, 11:48 a.m. UTC | #21 
On 27.02.2019 12:04, Maxime Coquelin wrote:
> 
> 
> On 2/26/19 3:07 PM, Ilya Maximets wrote:
>> On 26.02.2019 16:43, Maxime Coquelin wrote:
>>>
>>>
>>> On 2/26/19 2:36 PM, Ilya Maximets wrote:
>>>> On 26.02.2019 15:32, Maxime Coquelin wrote:
>>>>>
>>>>>
>>>>> On 2/26/19 9:42 AM, Ilya Maximets wrote:
>>>>>> On 26.02.2019 11:13, Liu, Changpeng wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>>> Sent: Tuesday, February 26, 2019 3:39 PM
>>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>>
>>>>>>>> On 26.02.2019 10:01, Liu, Changpeng wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>>>>>>>>> Sent: Monday, February 25, 2019 9:20 PM
>>>>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages
>>>>>>>>>>
>>>>>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote:
>>>>>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
>>>>>>>>>>> used to get/set virtio device's PCI configuration space.
>>>>>>>>>>
>>>>>>>>>> Beside the fact that some additional description and reasoning required,
>>>>>>>>>> I do not see the usage of this feature. You're defining the flag
>>>>>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk
>>>>>>>> vhost
>>>>>>>>>> backends (vdpa, vhost-user) will use this feature.
>>>>>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
>>>>>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES.
>>>>>>>>>>
>>>>>>>>>>    From the other side, current implementation forces application to properly
>>>>>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages
>>>>>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
>>>>>>>>>> disconnection.
>>>>>>>>>> This looks strange, because supported protocol features normally enabled by
>>>>>>>>>> default. Am I misunderstood something ?
>>>>>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
>>>>>>>> wasn't enabled.
>>>>>>>>
>>>>>>>> So, you're going to enable it only by explicit call to
>>>>>>>> 'rte_vhost_driver_set_features' ?
>>>>>>>>
>>>>>>>> In this case I'm assuming that you're implementing your own vhost backend.
>>>>>>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle'
>>>>>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
>>>>>>>> 'vhost_crypto' backend ?
>>>>>>> The patch was developed one year ago, while DPDK didn't have external ops.
>>>>>>
>>>>>> So, maybe it's time to reconsider the implementation.
>>>>>
>>>>> +1
>>>>>
>>>>>>> The get_config/set_config was defined for all the virtio devices, so I think it makes
>>>>>>> more sense adding here.
>>>>>>
>>>>>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices
>>>>>> too, however they makes sense for vhost_crypto backend only. These messages
>>>>>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
>>>>>> implemented, so IMHO it's better to implement their handlers along with the
>>>>>> callbacks, i.e. inside the implementation of your vhost backend.
>>>>>>
>>>>>> Maxime, Tiwei, what do you think ?
>>>>>
>>>>> I would prefer it to be implemented in SPDK directly as a pre_handler
>>>>> callback, as I don't foresee a need for it for other backends, and it
>>>>> would avoid breaking the API.
>>>>>
>>>>> It would imply fixing the beginning of vhost_user_msg_handler() to accept requests > VHOST_USER_MAX and add necessary check before doing
>>>>> the debug logs.
>>>>
>>>> VHOST_USER_MAX is 31 and both new requests are
>>>> defined in the same enum VhostUserRequest:
>>>>
>>>>      VHOST_USER_GET_CONFIG = 24,
>>>>      VHOST_USER_SET_CONFIG = 25
>>>>
>>>> I don't think that any change is needed here.
>>>
>>> I didn't meant GET_SET_CONFIG specifically. I meant that if we want
>>> something really generic, we would need to do that.
>>
>> OK. I understand now.
>>
>>>
>>> BTW, it would crash as vhost_message_str[VHOST_USER_GET/SET_CONFIG] would not be defined.
>>>
>>>>
>>>>>
>>>>> With above change we would also be able to remove VHOST_CRYPTO requests
>>>>> from vhost_user.c,
>>>>
>>>> Maybe you're looking at the different git HEAD ? I don't see any crypto
>>>> related code in vhost_user.c. Only name definition in vhost_message_str.
>>>
>>> Yes, I meant removing their definition in vhost_message_str[].
>>>
>>> My point is that if we want to have external backends to handle their
>>> specific requests, we should not have to modify vhost_user.c as it
>>> creates a useless dependency.
>>
>> That's a good point. I agree.
>>
>> Maybe we'll need some new API to make vhost library more dynamic?
>> Something like
>>      rte_vhost_message_register(enum VhostUserRequest request,
>>                                 const char *resuest_str,
>>                                 vhost_message_handler_t handler);
>> This could be flexible.
> 
> Agree it could be done like that.
> 
> Now, I think we have pretty much every thing we need in the API to
> implement it, but maybe I'm missing something?

Yes, looks like we have.

> 
> I.e., by implementing the .pre_msg_handle callback and setting its
> skip_master to 1, we have the same result. Except that we don't
> have the debug message.

Sure. This should work.

> 
> Also, it means we would need to either rework all current handlers,
> or make "struct virtio_net" part of the API.

Actually, 'vhost_crypto' already uses the 'struct virtio_net'. So, it's
kind of a part of the API anyway. Do we need a special API to get
'extern_data' ? Right now 'vhost_crypto' gets it directly from the
'vhost_net' structure.

'vhost_crypto' seems very strange though. I don't think that it works.
(example calls 'set_features' via 'crypto_create' inside the 'new_device'
callback, this should not work). Also, it uses a lot of internal stuff for
which we have public APIs.

> So maybe we'll have to come to this, but we would need first to do
> a significant rework of the library to move all the net specific
> stuff out of the generic vhost part.

Sure. It seems to me that we need to split out network related stuff
from the 'struct virtio_net' to a separate 'struct vhost_net' and rename
'struct virtio_net' to, for example, 'struct virtio_dev'. Making some
kind of inheritance between them. Maybe having a 'backend' pointer to the
corresponding 'struct vhost_net/crypto/scsi' and replace the 'extern_data'
with it.

> 
> Thanks,
> Maxime
>>
>>>
>>>>> and we could then work on moving vhost-net bits
>>>>> out of this file too.
>>>>>
>>>>> Regards,
>>>>> Maxime
>>>>>
>>>>>
>>>>>
>>>
>>>
> 
>
Liu, Changpeng Feb. 28, 2019, 12:49 p.m. UTC | #22 
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, February 27, 2019 6:05 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; Ilya Maximets
> <i.maximets@samsung.com>; dev@dpdk.org
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Jason Wang
> <jasowang@redhat.com>
> Subject: Re: vhost: add virtio configuration space access socket messages
> 
> 
> 
> On 2/27/19 10:50 AM, Liu, Changpeng wrote:
> >> What we need to do is fix vhost lib so that you don't have anything to
> >> do in dpdk to add support for the new requests.
> >>
> >> So we need to fix the few bits in vhost_user_msg_handler() I mentionned
> >> yesterday. And I also notice we are missing
> >> rte_vhost_driver_set_protocol_feature(), so that you can advertise
> >> VHOST_USER_PROTOCOL_F_CONFIG support by your external backend.
> > Sounds good to me, I will not post a new version and wait for the new API.
> 
> I just posted the RFC, could you try it and let us know if it works for
> you?
Yeah, it can work for us. 
> 
> Thanks,
> Maxime
> 
> >> I'll try to cook the dpdk patch today.
> >>
> >> Cheers,
diff mbox series

Patch

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 2753670..ab710ce 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -63,6 +63,10 @@ 
 #define VHOST_USER_PROTOCOL_F_PAGEFAULT 8
 #endif
 
+#ifndef VHOST_USER_PROTOCOL_F_CONFIG
+#define VHOST_USER_PROTOCOL_F_CONFIG 9
+#endif
+
 #ifndef VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
 #define VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD 10
 #endif
@@ -173,6 +177,10 @@  struct vhost_device_ops {
 
 	int (*vring_state_changed)(int vid, uint16_t queue_id, int enable);	/**< triggered when a vring is enabled or disabled */
 
+	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);
+
 	/**
 	 * Features could be changed after the feature negotiation.
 	 * For example, VHOST_F_LOG_ALL will be set/cleared at the
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index b086ad9..8e42734 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -73,6 +73,8 @@ 
 	[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",
 	[VHOST_USER_CRYPTO_CREATE_SESS] = "VHOST_USER_CRYPTO_CREATE_SESS",
 	[VHOST_USER_CRYPTO_CLOSE_SESS] = "VHOST_USER_CRYPTO_CLOSE_SESS",
 	[VHOST_USER_POSTCOPY_ADVISE]  = "VHOST_USER_POSTCOPY_ADVISE",
@@ -359,6 +361,46 @@ 
 	return RTE_VHOST_MSG_RESULT_OK;
 }
 
+static int
+vhost_user_get_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
+		      int main_fd __rte_unused)
+{
+	struct virtio_net *dev = *pdev;
+
+	if (!dev->notify_ops->get_config) {
+		msg->size = sizeof(uint64_t);
+		return RTE_VHOST_MSG_RESULT_REPLY;
+	}
+
+	if (dev->notify_ops->get_config(dev->vid,
+					msg->payload.config.region,
+					msg->payload.config.size) != 0) {
+		msg->size = sizeof(uint64_t);
+	}
+
+	return RTE_VHOST_MSG_RESULT_REPLY;
+}
+
+static int
+vhost_user_set_config(struct virtio_net **pdev, struct VhostUserMsg *msg,
+		      int main_fd __rte_unused)
+{
+	struct virtio_net *dev = *pdev;
+
+	if (!dev->notify_ops->set_config)
+		return RTE_VHOST_MSG_RESULT_ERR;
+
+	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) {
+		return RTE_VHOST_MSG_RESULT_ERR;
+	}
+
+	return RTE_VHOST_MSG_RESULT_OK;
+}
+
 /*
  * Reallocate virtio_dev and vhost_virtqueue data structure to make them on the
  * same numa node as the memory of vring descriptor.
@@ -1725,6 +1767,8 @@  typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
 	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
 	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_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,
 	[VHOST_USER_POSTCOPY_ADVISE] = vhost_user_set_postcopy_advise,
 	[VHOST_USER_POSTCOPY_LISTEN] = vhost_user_set_postcopy_listen,
 	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index 2a650fe..057cdec 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -12,6 +12,11 @@ 
 
 /* refer to hw/virtio/vhost-user.c */
 
+/*
+ * Maximum size of virtio device config space
+ */
+#define VHOST_USER_MAX_CONFIG_SIZE 256
+
 #define VHOST_MEMORY_MAX_NREGIONS 8
 
 #define VHOST_USER_PROTOCOL_FEATURES	((1ULL << VHOST_USER_PROTOCOL_F_MQ) | \
@@ -49,6 +54,8 @@ 
 	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_CRYPTO_CREATE_SESS = 26,
 	VHOST_USER_CRYPTO_CLOSE_SESS = 27,
 	VHOST_USER_POSTCOPY_ADVISE = 28,
@@ -60,6 +67,7 @@ 
 typedef enum VhostUserSlaveRequest {
 	VHOST_USER_SLAVE_NONE = 0,
 	VHOST_USER_SLAVE_IOTLB_MSG = 1,
+	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
 	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
 	VHOST_USER_SLAVE_MAX
 } VhostUserSlaveRequest;
@@ -112,6 +120,13 @@ 
 	uint64_t offset;
 } VhostUserVringArea;
 
+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 {
 		uint32_t master; /* a VhostUserRequest value */
@@ -131,6 +146,7 @@ 
 		struct vhost_vring_addr addr;
 		VhostUserMemory memory;
 		VhostUserLog    log;
+		VhostUserConfig config;
 		struct vhost_iotlb_msg iotlb;
 		VhostUserCryptoSessionParam crypto_session;
 		VhostUserVringArea area;