From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5DD7DA00BE; Fri, 12 Jun 2020 14:37:25 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 33FB82BE2; Fri, 12 Jun 2020 14:37:24 +0200 (CEST) Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [217.70.178.230]) by dpdk.org (Postfix) with ESMTP id 4DEA31150 for ; Fri, 12 Jun 2020 14:37:23 +0200 (CEST) Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay10.mail.gandi.net (Postfix) with ESMTPSA id 8805D24000F; Fri, 12 Jun 2020 12:37:20 +0000 (UTC) Date: Fri, 12 Jun 2020 14:37:15 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Maxime Coquelin Cc: dev@dpdk.org, matan@mellanox.com, xiao.w.wang@intel.com, zhihong.wang@intel.com, xiaolong.ye@intel.com, chenbo.xia@intel.com, david.marchand@redhat.com, amorenoz@redhat.com, shreyansh.jain@nxp.com, viacheslavo@mellanox.com, hemant.agrawal@nxp.com, sachin.saxena@nxp.com Message-ID: <20200612123715.hfboy57s4t66xnor@u256.net> References: <20200611213748.1967029-1-maxime.coquelin@redhat.com> <20200611213748.1967029-4-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200611213748.1967029-4-maxime.coquelin@redhat.com> Subject: Re: [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 11/06/20 23:37 +0200, Maxime Coquelin wrote: > This patch introduces vDPA device class. It will enable > application to iterate over the vDPA devices. > > Signed-off-by: Maxime Coquelin > --- > lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++------- > 1 file changed, 98 insertions(+), 18 deletions(-) > > diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c > index b2b2a105f1..61ab9aadb4 100644 > --- a/lib/librte_vhost/vdpa.c > +++ b/lib/librte_vhost/vdpa.c > @@ -10,11 +10,12 @@ > > #include > > +#include > #include > #include "rte_vdpa.h" > #include "vhost.h" > > -static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE]; > +static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE]; > static uint32_t vdpa_device_num; > > static bool > @@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, > struct rte_vdpa_dev_ops *ops) > { > struct rte_vdpa_device *dev; > - char device_name[MAX_VDPA_NAME_LEN]; > int i; > > if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL) > return -1; > > for (i = 0; i < MAX_VHOST_DEVICE; i++) { > - dev = vdpa_devices[i]; > - if (dev && is_same_vdpa_device(&dev->addr, addr)) > + dev = &vdpa_devices[i]; > + if (dev->ops && is_same_vdpa_device(&dev->addr, addr)) > return -1; > } > > for (i = 0; i < MAX_VHOST_DEVICE; i++) { > - if (vdpa_devices[i] == NULL) > + if (vdpa_devices[i].ops == NULL) > break; > } > > if (i == MAX_VHOST_DEVICE) > return -1; > > - snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i); > - dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device), > - RTE_CACHE_LINE_SIZE); > - if (!dev) > - return -1; > - > + dev = &vdpa_devices[i]; > memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr)); > dev->ops = ops; > - vdpa_devices[i] = dev; > vdpa_device_num++; > > return i; > @@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, > int > rte_vdpa_unregister_device(int did) > { > - if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL) > + if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL) > return -1; > > - rte_free(vdpa_devices[did]); > - vdpa_devices[did] = NULL; > + memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device)); > vdpa_device_num--; > > return did; > @@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr) > return -1; > > for (i = 0; i < MAX_VHOST_DEVICE; ++i) { > - dev = vdpa_devices[i]; > - if (dev && is_same_vdpa_device(&dev->addr, addr)) > + dev = &vdpa_devices[i]; > + if (dev->ops == NULL) > + continue; > + > + if (is_same_vdpa_device(&dev->addr, addr)) > return i; > } > > @@ -117,7 +113,7 @@ rte_vdpa_get_device(int did) > if (did < 0 || did >= MAX_VHOST_DEVICE) > return NULL; > > - return vdpa_devices[did]; > + return &vdpa_devices[did]; > } > > int > @@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m) > free_ind_table(idesc); > return -1; > } > + > +static uint16_t > +vdpa_dev_to_id(const struct rte_vdpa_device *dev) > +{ > + if (dev == NULL) > + return MAX_VHOST_DEVICE; I know it comes from ethdev, this helper was introduced by 214ed1acd12 and changed the comparison type from ptrdiff_t to uin16_t. This is incorrect, ideally it should instead be a ptrdiff_t, then if within acceptable range it should be casted to uin16_t to be used as an index. > + return dev - vdpa_devices; > +} > + > +static int > +vdpa_dev_match(struct rte_vdpa_device *dev, > + const struct rte_device *rte_dev) > +{ > + struct rte_vdpa_dev_addr addr; > + > + /* Only PCI bus supported for now */ > + if (strcmp(rte_dev->bus->name, "pci") != 0) > + return -1; > + > + addr.type = VDPA_ADDR_PCI; > + > + if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0) > + return -1; > + > + if (!is_same_vdpa_device(&dev->addr, &addr)) > + return -1; > + > + return 0; > +} > + > +/* Generic rte_vdpa_dev comparison function. */ > +typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *, > + const struct rte_device *rte_dev); > + > +static struct rte_vdpa_device * > +vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp, > + struct rte_device *rte_dev) > +{ > + struct rte_vdpa_device *dev; > + ptrdiff_t idx; The ptrdiff_t was used in ethdev due to the original start - &rte_eth_devices[0] + 1; The diff was removed by commit 214ed1acd12 . 'idx' should be the proper type to hold a device id, here probably uint16_t. Ethdev class should be cleaned up as well. > + > + /* Avoid Undefined Behaviour */ > + if (start != NULL && > + (start < &vdpa_devices[0] || > + start >= &vdpa_devices[MAX_VHOST_DEVICE])) > + return NULL; Same as before, this guard made sense mostly for the diff following. It is coupled to the "random ptr to static array" comparison happening, it would be better in vpda_dev_to_id() I think. > + > + if (start != NULL) > + idx = vdpa_dev_to_id(start) + 1; > + else > + idx = 0; > + for (; idx < MAX_VHOST_DEVICE; idx++) { > + dev = &vdpa_devices[idx]; > + /* > + * ToDo: Certainly better to introduce a state field, > + * but rely on ops being set for now. > + */ > + if (dev->ops == NULL) > + continue; > + if (cmp(dev, rte_dev) == 0) > + return dev; > + } > + return NULL; > +} > + > +static void * > +vdpa_dev_iterate(const void *start, > + const char *str, > + const struct rte_dev_iterator *it) > +{ > + struct rte_vdpa_device *vdpa_dev = NULL; > + > + RTE_SET_USED(str); > + > + vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device); > + > + return vdpa_dev; > +} > + > +static struct rte_class rte_class_vdpa = { > + .dev_iterate = vdpa_dev_iterate, > +}; > + > +RTE_REGISTER_CLASS(vdpa, rte_class_vdpa); > -- > 2.26.2 > -- Gaëtan