Hi Rosen,
> -----Original Message-----
> From: Xu, Rosen <rosen...@intel.com>
> Sent: Sunday, July 26, 2020 9:27 PM
> To: Xia, Chenbo <chenbo....@intel.com>; dev@dpdk.org
> Cc: maxime.coque...@redhat.com; Yigit, Ferruh <ferruh.yi...@intel.com>;
> Wang, Xiao W <xiao.w.w...@intel.com>
> Subject: RE: [PATCH v1] vdpa/ifc: fix vring update after device config
>
> Hi,
>
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo....@intel.com>
> > Sent: Saturday, July 25, 2020 1:09
> > To: dev@dpdk.org
> > Cc: maxime.coque...@redhat.com; Yigit, Ferruh
> > <ferruh.yi...@intel.com>; Wang, Xiao W <xiao.w.w...@intel.com>; Xu,
> > Rosen <rosen...@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.coque...@redhat.com>
> > Signed-off-by: Chenbo Xia <chenbo....@intel.com>
> > Acked-by: Wang Xiao W <xiao.w.w...@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?
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
