DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Xu, Rosen" <rosen.xu@intel.com>
To: "Xia, Chenbo" <chenbo.xia@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Wang, Xiao W" <xiao.w.wang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1] vdpa/ifc: fix vring update after device config
Date: Tue, 28 Jul 2020 03:07:56 +0000
Message-ID: <BYAPR11MB2901F02123C858055A247DE889730@BYAPR11MB2901.namprd11.prod.outlook.com> (raw)
In-Reply-To: <MN2PR11MB4063066CF6453D38FCFBD8209C730@MN2PR11MB4063.namprd11.prod.outlook.com>

Hi Chenbo,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Tuesday, July 28, 2020 10:35
> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Wang, Xiao W <xiao.w.wang@intel.com>
> Subject: RE: [PATCH v1] vdpa/ifc: fix vring update after device config
> 
> Hi Rosen,
> 
> > -----Original Message-----
> > From: Xu, Rosen <rosen.xu@intel.com>
> > Sent: Sunday, July 26, 2020 9:27 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> > Cc: maxime.coquelin@redhat.com; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>
> > Subject: RE: [PATCH v1] vdpa/ifc: fix vring update after device config
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Xia, Chenbo <chenbo.xia@intel.com>
> > > Sent: Saturday, July 25, 2020 1:09
> > > To: dev@dpdk.org
> > > Cc: maxime.coquelin@redhat.com; Yigit, Ferruh
> > > <ferruh.yigit@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>; Xu,
> > > Rosen <rosen.xu@intel.com>
> > > Subject: [PATCH v1] vdpa/ifc: fix vring update after device config
> > >
> > > The device ready state in vhost lib is now defined as the state that
> > > first queue pair is ready. And kick/callfd may be updated by QEMU
> > > when ifc device is configured.
> > >
> > > Although now ifc driver only supports one queue pair, it still has
> > > to update callfd when working with QEMU. This patch fixes this vring
> > > update problem by implementing the set_vring_state callback.
> > >
> > > Fixes: a3f8150eac6d (net/ifcvf: add ifcvf vDPA driver)
> > >
> > > Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > Signed-off-by: Chenbo Xia <chenbo.xia@intel.com>
> > > Acked-by: Wang Xiao W <xiao.w.wang@intel.com>
> > > ---
> > >  drivers/vdpa/ifc/base/ifcvf.h |  1 +  drivers/vdpa/ifc/ifcvf_vdpa.c
> > > | 54
> > > ++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 54 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/ifc/base/ifcvf.h
> > > b/drivers/vdpa/ifc/base/ifcvf.h index eb04a9406..a288ce57d 100644
> > > --- a/drivers/vdpa/ifc/base/ifcvf.h
> > > +++ b/drivers/vdpa/ifc/base/ifcvf.h
> > > @@ -115,6 +115,7 @@ struct vring_info {
> > >  	u16 size;
> > >  	u16 last_avail_idx;
> > >  	u16 last_used_idx;
> > > +	bool enable;
> > >  };
> > >
> > >  struct ifcvf_hw {
> > > diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c
> > > b/drivers/vdpa/ifc/ifcvf_vdpa.c index a757d45ec..9da25f0c7 100644
> > > --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
> > > +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
> > > @@ -49,6 +49,7 @@ static const char * const ifcvf_valid_arguments[]
> > > = {  struct ifcvf_internal {
> > >  	struct rte_pci_device *pdev;
> > >  	struct ifcvf_hw hw;
> > > +	int configured;
> > >  	int vfio_container_fd;
> > >  	int vfio_group_fd;
> > >  	int vfio_dev_fd;
> > > @@ -897,6 +898,7 @@ ifcvf_dev_config(int vid)
> > >  		DRV_LOG(NOTICE, "vDPA (%s): software relay is used.",
> > >  				vdev->device->name);
> > >
> > > +	internal->configured = 1;
> > >  	return 0;
> > >  }
> > >
> > > @@ -935,6 +937,7 @@ ifcvf_dev_close(int vid)
> > >  		update_datapath(internal);
> > >  	}
> > >
> > > +	internal->configured = 0;
> > >  	return 0;
> > >  }
> > >
> > > @@ -1084,13 +1087,61 @@ ifcvf_get_protocol_features(struct
> > > rte_vdpa_device *vdev, uint64_t *features)
> > >  	return 0;
> > >  }
> > >
> > > +static int
> > > +ifcvf_set_vring_state(int vid, int vring, int state) {
> > > +	struct rte_vdpa_device *vdev;
> > > +	struct internal_list *list;
> > > +	struct ifcvf_internal *internal;
> > > +	struct ifcvf_hw *hw;
> > > +	struct ifcvf_pci_common_cfg *cfg;
> > > +	int ret = 0;
> > > +
> > > +	vdev = rte_vhost_get_vdpa_device(vid);
> > > +	list = find_internal_resource_by_vdev(vdev);
> > > +	if (list == NULL) {
> > > +		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
> > > +		return -1;
> > > +	}
> > > +
> > > +	internal = list->internal;
> > > +	if (vring < 0 || vring >= internal->max_queues * 2) {
> > > +		DRV_LOG(ERR, "Vring index %d not correct", vring);
> > > +		return -1;
> > > +	}
> > > +
> > > +	hw = &internal->hw;
> > > +	if (!internal->configured)
> > > +		goto exit;
> > > +
> > > +	cfg = hw->common_cfg;
> > > +	IFCVF_WRITE_REG16(vring, &cfg->queue_select);
> > > +	IFCVF_WRITE_REG16(state, &cfg->queue_enable);
> > > +
> > > +	if (!state && hw->vring[vring].enable) {
> > > +		ret = vdpa_disable_vfio_intr(internal);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	if (state && !hw->vring[vring].enable) {
> > > +		ret = vdpa_enable_vfio_intr(internal, 0);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +exit:
> > > +	hw->vring[vring].enable = !!state;
> >
> > Redundant operation on variable "state".
> 
> The state should be 0/1 by default, but as the callback definition uses 'int',
> for other lib passing non-0/1 to the callback, we'd better to make sure it's 0/1
> internally. What do you think?

Got, it make sense.

> And thanks to your comments, I find that the register writing does not make
> sure it's a 0/1.
> 
> Thanks!
> Chenbo
> 
> >
> > > +	return 0;
> > > +}
> > > +
> > >  static struct rte_vdpa_dev_ops ifcvf_ops = {
> > >  	.get_queue_num = ifcvf_get_queue_num,
> > >  	.get_features = ifcvf_get_vdpa_features,
> > >  	.get_protocol_features = ifcvf_get_protocol_features,
> > >  	.dev_conf = ifcvf_dev_config,
> > >  	.dev_close = ifcvf_dev_close,
> > > -	.set_vring_state = NULL,
> > > +	.set_vring_state = ifcvf_set_vring_state,
> > >  	.set_features = ifcvf_set_features,
> > >  	.migration_done = NULL,
> > >  	.get_vfio_group_fd = ifcvf_get_vfio_group_fd, @@ -1170,6 +1221,7
> > @@
> > > ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> > >  		goto error;
> > >  	}
> > >
> > > +	internal->configured = 0;
> > >  	internal->max_queues = IFCVF_MAX_QUEUES;
> > >  	features = ifcvf_get_features(&internal->hw);
> > >  	internal->features = (features &
> > > --
> > > 2.17.1


  reply	other threads:[~2020-07-28  3:08 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-24 17:09 Chenbo Xia
2020-07-26 13:27 ` Xu, Rosen
2020-07-28  2:35   ` Xia, Chenbo
2020-07-28  3:07     ` Xu, Rosen [this message]
2020-07-28 14:32 ` [dpdk-dev] [PATCH v2] " Chenbo Xia
2020-07-28  8:24   ` Wang, Xiao W
2020-07-28  9:05   ` Xu, Rosen
2020-07-28 14:38   ` Maxime Coquelin
2020-07-28 15:56   ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR11MB2901F02123C858055A247DE889730@BYAPR11MB2901.namprd11.prod.outlook.com \
    --to=rosen.xu@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=xiao.w.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git