RE: [PATCH v6 03/16] vhost: add vhost msg support

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

Hi Chenbo, 

Thanks for your reply.
My reply is inline.

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Tuesday, April 26, 2022 5:17 PM
> 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>
> Subject: RE: [PATCH v6 03/16] vhost: add vhost msg support
> 
> > -----Original Message-----
> > From: Pei, Andy <andy.pei@intel.com>
> > Sent: Tuesday, April 26, 2022 4:56 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> > Cc: maxime.coquelin@redhat.com; Cao, Gang <gang.cao@intel.com>; Liu,
> > Changpeng <changpeng.liu@intel.com>
> > Subject: RE: [PATCH v6 03/16] vhost: add vhost msg support
> >
> > HI Chenbo,
> >
> > Thanks for your reply.
> > My reply is inline.
> >
> > > -----Original Message-----
> > > From: Xia, Chenbo <chenbo.xia@intel.com>
> > > Sent: Monday, April 25, 2022 8:42 PM
> > > 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>
> > > Subject: RE: [PATCH v6 03/16] vhost: add vhost msg support
> > >
> > > Hi Andy,
> > >
> > > > -----Original Message-----
> > > > From: Pei, Andy <andy.pei@intel.com>
> > > > Sent: Thursday, April 21, 2022 4:34 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>
> > > > Subject: [PATCH v6 03/16] vhost: add vhost msg support
> > > >
> > > > 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 | 69
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  lib/vhost/vhost_user.h | 13 ++++++++++
> > > >  2 files changed, 82 insertions(+)
> > > >
> > > > diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c index
> > > > 1d39067..3780804 100644
> > > > --- a/lib/vhost/vhost_user.c
> > > > +++ b/lib/vhost/vhost_user.c
> > > > @@ -80,6 +80,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", @@
> > > > -2542,6 +2544,71 @@ 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 (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
> > > supportted!\n",
> > >
> > > Supported
> > >
> > I will send out a new version to fix this.
> > > > + dev->ifname);
> > > > +}
> > > > +
> > > > +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 (ctx->msg.size != sizeof(struct vhost_user_config)) {
> > >
> > > I think you should do sanity check on payload.cfg.size and make sure
> > it's
> > > smaller than VHOST_USER_MAX_CONFIG_SIZE
> > >
> > > and same check for offset
> > >
> > I think payload.cfg.size can be smaller than or equal to
> > VHOST_USER_MAX_CONFIG_SIZE.
> > payload.cfg.ofset can be smaller than or equal to
> > VHOST_USER_MAX_CONFIG_SIZE as well
> 
> After double check: offset is the config space offset, so this should be
> checked in vdpa driver. Size check on vhost lib layer should be just <=
> MAX_you_defined
> 
OK.
> Thanks,
> Chenbo
> 
> >
> > > > +VHOST_LOG_CONFIG(ERR,
> > > > +"(%s) invalid set config msg size: %"PRId32" != %d\n",
> > > > +dev->ifname, ctx->msg.size,
> > >
> > > Based on you will change the log too, payload.cfg.size is uint32_t,
> > > so
> > PRId32 ->
> > > PRIu32
> > >
> > > > +(int)sizeof(struct vhost_user_config));
> > >
> > > So this can be %u
> > >
> > Sure.
> > > > +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
> > > supportted!\n",
> > >
> > > Supported
> > >
> > I will send out a new version to fix this.
> > > > + dev->ifname);
> > > > +}
> > > > +
> > > > +return RTE_VHOST_MSG_RESULT_OK;
> > > > +
> > > > +OUT:
> > >
> > > Lower case looks better
> > >
> > OK. I will send out a new version to fix this.
> > > > +return RTE_VHOST_MSG_RESULT_ERR;
> > > > +}
> > >
> > > Almost all handlers need check on expected fd num (this case is 0),
> > > so
> > the
> > > above new handlers should also do that. Please refer to
> > > validate_msg_fds
> > in
> > > other handlers.
> > >
> > > BTW, you can wait for review for other patches and send new versions
> > later.
> > >
> > I will send out new patch after vhost: validate fds attached to
> > messages from David Marchand is merged.
> > > Thanks,
> > > Chenbo
> > >
> > > > +
> > > > +static int
> > > >  vhost_user_iotlb_msg(struct virtio_net **pdev,  struct
> > > > vhu_msg_context *ctx,  int main_fd __rte_unused) @@ -2782,6
> > > > +2849,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/vhost/vhost_user.h b/lib/vhost/vhost_user.h index
> > > > c946cc2..97cfb2f 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, @@ -125,6 +127,16 @@
> > > > uint16_t queue_size;  } VhostUserInflight;
> > > >
> > > > +#define VHOST_USER_MAX_CONFIG_SIZE256
> > > > +
> > > > +/** 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 */ @@ -148,6
> > > +160,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
> > >
> >
>