RE: [PATCH v9 03/13] vhost: add vhost msg support for get/set config

["Pei, Andy" <andy.pei@intel.com>]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Tuesday, May 24, 2022 10:52 AM
> To: Pei, Andy <andy.pei@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Cao, Gang <gang.cao@intel.com>; Liu,
> Changpeng <changpeng.liu@intel.com>; Xu, Rosen <rosen.xu@intel.com>;
> Xiao, QimaiX <qimaix.xiao@intel.com>
> Subject: RE: [PATCH v9 03/13] vhost: add vhost msg support for get/set
> config
> 
> > -----Original Message-----
> > From: Pei, Andy <andy.pei@intel.com>
> > Sent: Monday, May 23, 2022 5:35 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com;
> > Cao, Gang <gang.cao@intel.com>; Liu, Changpeng
> > <changpeng.liu@intel.com>; Xu, Rosen <rosen.xu@intel.com>; Xiao,
> > QimaiX <qimaix.xiao@intel.com>
> > Subject: [PATCH v9 03/13] vhost: add vhost msg support for get/set
> > config
> >
> > Add support for VHOST_USER_GET_CONFIG and
> VHOST_USER_SET_CONFIG.
> > VHOST_USER_GET_CONFIG and VHOST_USER_SET_CONFIG message is only
> > supported by virtio blk VDPA device.
> >
> > Signed-off-by: Andy Pei <andy.pei@intel.com>
> > ---
> >  lib/vhost/vhost_user.c | 88
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/vhost/vhost_user.h | 13 ++++++++
> >  2 files changed, 101 insertions(+)
> >
> > diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c index
> > 850848c..b37d814 100644
> > --- a/lib/vhost/vhost_user.c
> > +++ b/lib/vhost/vhost_user.c
> > @@ -2468,6 +2468,92 @@ static int is_vring_iotlb(struct virtio_net
> > *dev,  }
> >
> >  static int
> > +vhost_user_get_config(struct virtio_net **pdev,
> > +			struct vhu_msg_context *ctx,
> > +			int main_fd __rte_unused)
> > +{
> > +	struct virtio_net *dev = *pdev;
> > +	struct rte_vdpa_device *vdpa_dev = dev->vdpa_dev;
> > +	int ret = 0;
> > +
> > +	if (validate_msg_fds(dev, ctx, 0) != 0)
> > +		return RTE_VHOST_MSG_RESULT_ERR;
> > +
> > +	if (!vdpa_dev) {
> > +		VHOST_LOG_CONFIG(ERR,
> > +				 "(%s) is not vDPA device!\n",
> > +				 dev->ifname);
> > +		goto out;
> > +	}
> 
> In my understanding, this message is only targeted for vDPA device.
> Traditional SW back-end will not use it. So if vdpa_dev is not attached, you
> can just return RTE_VHOST_MSG_RESULT_ERR here. Another way is setting
> msg size to 0 and reply, but I think in this case. We should just break the msg
> handling.
> 
OK. I will just return RTE_VHOST_MSG_RESULT_ERR here.

> Besides, two lines are enough for the log above:
> 
> +		VHOST_LOG_CONFIG(ERR, "(%s) is not vDPA device!\n",
> +				 dev->ifname);
> 
Sure.
> Thanks,
> Chenbo
> 
> > +
> > +	if (vdpa_dev->ops->get_config) {
> > +		ret = vdpa_dev->ops->get_config(dev->vid,
> > +					   ctx->msg.payload.cfg.region,
> > +					   ctx->msg.payload.cfg.size);
> > +		if (ret != 0) {
> > +			ctx->msg.size = 0;
> > +			VHOST_LOG_CONFIG(ERR,
> > +					 "(%s) get_config() return error!\n",
> > +					 dev->ifname);
> > +		}
> > +	} else {
> > +		VHOST_LOG_CONFIG(ERR, "(%s) get_config() not
> supported!\n",
> > +				 dev->ifname);
> > +	}
> > +
> > +out:
> > +	return RTE_VHOST_MSG_RESULT_REPLY;
> > +}
> > +
> > +static int
> > +vhost_user_set_config(struct virtio_net **pdev,
> > +			struct vhu_msg_context *ctx,
> > +			int main_fd __rte_unused)
> > +{
> > +	struct virtio_net *dev = *pdev;
> > +	struct rte_vdpa_device *vdpa_dev = dev->vdpa_dev;
> > +	int ret = 0;
> > +
> > +	if (validate_msg_fds(dev, ctx, 0) != 0)
> > +		return RTE_VHOST_MSG_RESULT_ERR;
> > +
> > +	if (ctx->msg.payload.cfg.size > VHOST_USER_MAX_CONFIG_SIZE) {
> > +		VHOST_LOG_CONFIG(ERR,
> > +			"(%s) vhost_user_config size: %"PRIu32", should not
> be
> > larger than %d\n",
> > +			dev->ifname, ctx->msg.payload.cfg.size,
> > +			VHOST_USER_MAX_CONFIG_SIZE);
> > +		goto out;
> > +	}
> > +
> > +	if (!vdpa_dev) {
> > +		VHOST_LOG_CONFIG(ERR,
> > +				 "(%s) is not vDPA device!\n",
> > +				 dev->ifname);
> > +		goto out;
> > +	}
> > +
> > +	if (vdpa_dev->ops->set_config) {
> > +		ret = vdpa_dev->ops->set_config(dev->vid,
> > +			ctx->msg.payload.cfg.region,
> > +			ctx->msg.payload.cfg.offset,
> > +			ctx->msg.payload.cfg.size,
> > +			ctx->msg.payload.cfg.flags);
> > +		if (ret)
> > +			VHOST_LOG_CONFIG(ERR,
> > +					 "(%s) set_config() return error!\n",
> > +					 dev->ifname);
> > +	} else {
> > +		VHOST_LOG_CONFIG(ERR, "(%s) set_config() not
> supported!\n",
> > +				 dev->ifname);
> > +	}
> > +
> > +	return RTE_VHOST_MSG_RESULT_OK;
> > +
> > +out:
> > +	return RTE_VHOST_MSG_RESULT_ERR;
> > +}
> > +
> > +static int
> >  vhost_user_iotlb_msg(struct virtio_net **pdev,
> >  			struct vhu_msg_context *ctx,
> >  			int main_fd __rte_unused)
> > @@ -2686,6 +2772,8 @@ static int is_vring_iotlb(struct virtio_net
> > *dev,  VHOST_MESSAGE_HANDLER(VHOST_USER_NET_SET_MTU,
> > vhost_user_net_set_mtu,
> > false) \
> >  VHOST_MESSAGE_HANDLER(VHOST_USER_SET_SLAVE_REQ_FD,
> > vhost_user_set_req_fd,
> > true) \
> >  VHOST_MESSAGE_HANDLER(VHOST_USER_IOTLB_MSG,
> vhost_user_iotlb_msg,
> > false) \
> > +VHOST_MESSAGE_HANDLER(VHOST_USER_GET_CONFIG,
> vhost_user_get_config,
> > +false)
> > \
> > +VHOST_MESSAGE_HANDLER(VHOST_USER_SET_CONFIG,
> vhost_user_set_config,
> > +false)
> > \
> >  VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_ADVISE,
> > vhost_user_set_postcopy_advise, false) \
> > VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_LISTEN,
> > vhost_user_set_postcopy_listen, false) \
> > VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_END,
> > vhost_user_postcopy_end,
> > false) \
> > diff --git a/lib/vhost/vhost_user.h b/lib/vhost/vhost_user.h index
> > ba1c5c7..c4d091e 100644
> > --- a/lib/vhost/vhost_user.h
> > +++ b/lib/vhost/vhost_user.h
> > @@ -50,6 +50,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,
> > @@ -123,6 +125,16 @@
> >  	uint16_t queue_size;
> >  } VhostUserInflight;
> >
> > +#define VHOST_USER_MAX_CONFIG_SIZE		256
> > +
> > +/** Get/set config msg payload */
> > +struct vhost_user_config {
> > +	uint32_t offset;
> > +	uint32_t size;
> > +	uint32_t flags;
> > +	uint8_t region[VHOST_USER_MAX_CONFIG_SIZE];
> > +};
> > +
> >  typedef struct VhostUserMsg {
> >  	union {
> >  		uint32_t master; /* a VhostUserRequest value */ @@ -146,6
> +158,7 @@
> >  		VhostUserCryptoSessionParam crypto_session;
> >  		VhostUserVringArea area;
> >  		VhostUserInflight inflight;
> > +		struct vhost_user_config cfg;
> >  	} payload;
> >  	/* Nothing should be added after the payload */  } __rte_packed
> > VhostUserMsg;
> > --
> > 1.8.3.1