Hi Maxime, > -----Original Message----- > From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] > Sent: Thursday, March 22, 2018 4:58 AM > To: Wang, Xiao W <xiao.w.w...@intel.com>; y...@fridaylinux.org > Cc: dev@dpdk.org; Wang, Zhihong <zhihong.w...@intel.com>; Bie, Tiwei > <tiwei....@intel.com>; Chen, Junjie J <junjie.j.c...@intel.com>; Xu, Rosen > <rosen...@intel.com>; Daly, Dan <dan.d...@intel.com>; Liang, Cunming > <cunming.li...@intel.com>; Burakov, Anatoly <anatoly.bura...@intel.com>; > gaetan.ri...@6wind.com > Subject: Re: [PATCH v2 3/3] net/ifcvf: add ifcvf driver > > > > On 03/21/2018 02:21 PM, Xiao Wang wrote: > > ifcvf driver uses vdev as a control domain to manage ifc VFs that belong > > to it. It registers vDPA device ops to vhost lib to enable these VFs to be > > used as vhost data path accelerator. > > > > Live migration feature is supported by ifc VF and this driver enables > > it based on vhost lib. > > > > Because vDPA driver needs to set up MSI-X vector to interrupt the guest, > > only vfio-pci is supported currently. > > > > Signed-off-by: Xiao Wang <xiao.w.w...@intel.com> > > Signed-off-by: Rosen Xu <rosen...@intel.com> > > --- > > v2: > > - Rebase on Zhihong's vDPA v3 patch set. > > --- > > config/common_base | 6 + > > config/common_linuxapp | 1 + > > drivers/net/Makefile | 1 + > > drivers/net/ifcvf/Makefile | 40 + > > drivers/net/ifcvf/base/ifcvf.c | 329 ++++++++ > > drivers/net/ifcvf/base/ifcvf.h | 156 ++++ > > drivers/net/ifcvf/base/ifcvf_osdep.h | 52 ++ > > drivers/net/ifcvf/ifcvf_ethdev.c | 1240 > +++++++++++++++++++++++++++++++ > > drivers/net/ifcvf/rte_ifcvf_version.map | 4 + > > mk/rte.app.mk | 1 + > > 10 files changed, 1830 insertions(+) > > create mode 100644 drivers/net/ifcvf/Makefile > > create mode 100644 drivers/net/ifcvf/base/ifcvf.c > > create mode 100644 drivers/net/ifcvf/base/ifcvf.h > > create mode 100644 drivers/net/ifcvf/base/ifcvf_osdep.h > > create mode 100644 drivers/net/ifcvf/ifcvf_ethdev.c > > create mode 100644 drivers/net/ifcvf/rte_ifcvf_version.map > > > > ... > > > +static int > > +eth_dev_ifcvf_create(struct rte_vdev_device *dev, > > + struct rte_pci_addr *pci_addr, int devices) > > +{ > > + const char *name = rte_vdev_device_name(dev); > > + struct rte_eth_dev *eth_dev = NULL; > > + struct ether_addr *eth_addr = NULL; > > + struct ifcvf_internal *internal = NULL; > > + struct internal_list *list = NULL; > > + struct rte_eth_dev_data *data = NULL; > > + struct rte_pci_addr pf_addr = *pci_addr; > > + int i; > > + > > + list = rte_zmalloc_socket(name, sizeof(*list), 0, > > + dev->device.numa_node); > > + if (list == NULL) > > + goto error; > > + > > + /* reserve an ethdev entry */ > > + eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal)); > > + if (eth_dev == NULL) > > + goto error; > > + > > + eth_addr = rte_zmalloc_socket(name, sizeof(*eth_addr), 0, > > + dev->device.numa_node); > > + if (eth_addr == NULL) > > + goto error; > > + > > + *eth_addr = base_eth_addr; > > + eth_addr->addr_bytes[5] = eth_dev->data->port_id; > > + > > + internal = eth_dev->data->dev_private; > > + internal->dev_name = strdup(name); > > + if (internal->dev_name == NULL) > > + goto error; > > + > > + internal->eng_addr.pci_addr = *pci_addr; > > + for (i = 0; i < devices; i++) { > > + pf_addr.domain = pci_addr->domain; > > + pf_addr.bus = pci_addr->bus; > > + pf_addr.devid = pci_addr->devid + (i + 1) / 8; > > + pf_addr.function = pci_addr->function + (i + 1) % 8; > > + internal->vf_info[i].pdev.addr = pf_addr; > > + rte_spinlock_init(&internal->vf_info[i].lock); > > + } > > + internal->max_devices = devices; > > + > > + list->eth_dev = eth_dev; > > + pthread_mutex_lock(&internal_list_lock); > > + TAILQ_INSERT_TAIL(&internal_list, list, next); > > + pthread_mutex_unlock(&internal_list_lock); > > + > > + data = eth_dev->data; > > + data->nb_rx_queues = IFCVF_MAX_QUEUES; > > + data->nb_tx_queues = IFCVF_MAX_QUEUES; > > + data->dev_link = vdpa_link; > > + data->mac_addrs = eth_addr; > > We might want one ethernet device per VF, as for example you set > dev_link.link_status to UP as soon as a VF is configured, and DOWN > as when a single VF is removed.
Ideally it will be one representor port per VF, each representor port has a link_status. Will integrate port representor when it's ready. I will remove the vdev's ethdev registering for now, and add it back when we need to implement flow APIs on the vdev. > > > + data->dev_flags = RTE_ETH_DEV_INTR_LSC; > > + eth_dev->dev_ops = &ops; > > + > > + /* assign rx and tx ops, could be used as vDPA fallback */ > > + eth_dev->rx_pkt_burst = eth_ifcvf_rx; > > + eth_dev->tx_pkt_burst = eth_ifcvf_tx; > > + > > + if (rte_vdpa_register_engine(vdpa_ifcvf_driver.name, > > + &internal->eng_addr) < 0) > > + goto error; > > + > > + return 0; > > + > > +error: > > + rte_free(list); > > + rte_free(eth_addr); > > + if (internal && internal->dev_name) > > + free(internal->dev_name); > > + rte_free(internal); > > + if (eth_dev) > > + rte_eth_dev_release_port(eth_dev); > > + > > + return -1; > > +} > > + > > +static int > > +get_pci_addr(const char *key __rte_unused, const char *value, void > *extra_args) > > +{ > > + if (value == NULL || extra_args == NULL) > > + return -1; > > + > > + return rte_pci_addr_parse(value, extra_args); > > +} > > + > > +static inline int > > +open_int(const char *key __rte_unused, const char *value, void > *extra_args) > > +{ > > + uint16_t *n = extra_args; > > + > > + if (value == NULL || extra_args == NULL) > > + return -EINVAL; > > + > > + *n = (uint16_t)strtoul(value, NULL, 0); > > + if (*n == USHRT_MAX && errno == ERANGE) > > + return -1; > > + > > + return 0; > > +} > > + > > +/* > > + * If this vdev is created by user, then ifcvf will be taken by > > + * this vdev. > > + */ > > +static int > > +ifcvf_take_over(struct rte_pci_addr *pci_addr, int num) > > +{ > > + uint16_t port_id; > > + int i, ret; > > + char devname[RTE_DEV_NAME_MAX_LEN]; > > + struct rte_pci_addr vf_addr = *pci_addr; > > + > > + for (i = 0; i < num; i++) { > > + vf_addr.function += i % 8; > > + vf_addr.devid += i / 8; > > + rte_pci_device_name(&vf_addr, devname, > RTE_DEV_NAME_MAX_LEN); > > + ret = rte_eth_dev_get_port_by_name(devname, &port_id); > > + if (ret == 0) { > > + rte_eth_dev_close(port_id); > > + if (rte_eth_dev_detach(port_id, devname) < 0) > > + return -1; > > + } > That seems a bit hard. > Shouldn't we at least check the port is not started? That looks better, will do it. Thanks for the comments, -Xiao