From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AB33DA054F; Mon, 15 Feb 2021 10:31:43 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C47C61606FC; Mon, 15 Feb 2021 10:31:42 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id A913140FDF for ; Mon, 15 Feb 2021 10:31:41 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 1542F7F50C; Mon, 15 Feb 2021 12:31:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 1542F7F50C DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1613381501; bh=N2BRLEyAOw2SKbnM7f2VxI3/eBq8jWNIdnBY7iiVUWQ=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=olv/GA36r1DGfQGF4avJnJV82X7chPg7L3ZynNJKBMxn7iiGoOV/tnEyu/hx/Hh0C I6Kcemx9Acd65n0z1qbyiPQcgHddFNePScdtcqKdLjYX9lvlr8HN7yHXwkunrvBjg3 819SKeU0BPthn2aZx6mUAkZ9hojmYnfFcVfAXDdo= To: Xueming Li Cc: dev@dpdk.org, Viacheslav Ovsiienko , Asaf Penso , Thomas Monjalon , Ferruh Yigit , Ray Kinsella , Neil Horman References: <1613272907-22563-1-git-send-email-xuemingl@nvidia.com> <1613272907-22563-9-git-send-email-xuemingl@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Mon, 15 Feb 2021 12:31:40 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <1613272907-22563-9-git-send-email-xuemingl@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 8/9] ethdev: representor iterator compare complete info X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 2/14/21 6:21 AM, Xueming Li wrote: > The NIC can have multiple PCIe links and can be attached to multiple > hosts, for example the same single NIC can be shared for multiple server > units in the rack. On each PCIe link NIC can provide multiple PFs and > VFs/SFs based on these ones. The full representor identifier consists of > three indices - controller index, PF index, and VF or SF index (if any). > > SR-IOV and SubFunction are created on top of PF. PF index is introduced > because there might be multiple PFs in the bonding configuration and > only bonding device is probed. > > In eth representor comparator callback, ethdev representor ID was > compared with devarg. Since controller index and PF index not compared, > callback returned representor from other PF or controller. > > This patch adds new API to convert representor controller, pf and vf/sf > index to representor ID. Representor comparer callback convert > representor info into ID and compare with device representor ID. > > Signed-off-by: Xueming Li > --- > lib/librte_ethdev/ethdev_driver.h | 32 ++++++++++++ > lib/librte_ethdev/rte_class_eth.c | 38 ++++++++++---- > lib/librte_ethdev/rte_ethdev.c | 83 +++++++++++++++++++++++++++++++ > lib/librte_ethdev/version.map | 1 + > 4 files changed, 145 insertions(+), 9 deletions(-) > > diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h > index abcbc3112d..23342f1be2 100644 > --- a/lib/librte_ethdev/ethdev_driver.h > +++ b/lib/librte_ethdev/ethdev_driver.h > @@ -1243,6 +1243,38 @@ struct rte_eth_devargs { > enum rte_eth_representor_type type; /* type of representor */ > }; > > +/** > + * PMD helper function to convert representor ID from location detail > + * > + * Convert representor ID from controller, pf and (sf or vf). > + * The mapping is retrieved from rte_eth_representor_info_get(). > + * > + * If PMD doesn't return representor range info, simply ignore controller > + * and pf to keep backward compatibility. It does not sound right. If controller and/or pf is specified, it must not be ignored. > + * > + * @param ethdev > + * Handle of ethdev port. > + * @param id May I suggest to name it 'repr_id' to make it less ambguous. > + * Pointer to converted representor ID. I'd prefer do not mix in and out paramters. I suggest to make it the last parameter. > + * @param type > + * Representor type. > + * @param controller > + * Controller ID, -1 if unspecified. > + * @param pf > + * PF port ID, -1 if unspecified. > + * @param representor_port > + * Representor port ID, -1 if unspecified. Not sure that I understand what is it? Is it vf or sf number? > + * > + * @return > + * Negative errno value on error, 0 on success. > + */ > +__rte_internal > +int > +rte_eth_representor_id_convert(const struct rte_eth_dev *ethdev, > + uint16_t *id, > + enum rte_eth_representor_type type, > + int controller, int pf, int representor_port); > + > /** > * PMD helper function to parse ethdev arguments > * > diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c > index 051c892b40..f7b7e659e7 100644 > --- a/lib/librte_ethdev/rte_class_eth.c > +++ b/lib/librte_ethdev/rte_class_eth.c > @@ -65,9 +65,10 @@ eth_representor_cmp(const char *key __rte_unused, > { > int ret; > char *values; > - const struct rte_eth_dev_data *data = opaque; > - struct rte_eth_devargs representors; > - uint16_t index; > + const struct rte_eth_dev *edev = opaque; > + const struct rte_eth_dev_data *data = edev->data; > + struct rte_eth_devargs eth_da; > + uint16_t id, nc, np, nf, i, c, p, f; > > if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0) > return -1; /* not a representor port */ > @@ -76,17 +77,36 @@ eth_representor_cmp(const char *key __rte_unused, > values = strdup(value); > if (values == NULL) > return -1; > - memset(&representors, 0, sizeof(representors)); > - ret = rte_eth_devargs_parse_representor_ports(values, &representors); > + memset(ð_da, 0, sizeof(eth_da)); > + ret = rte_eth_devargs_parse_representor_ports(values, ð_da); > free(values); > if (ret != 0) > return -1; /* invalid devargs value */ > > + if (eth_da.nb_mh_controllers == 0 && eth_da.nb_ports == 0 && > + eth_da.nb_representor_ports == 0) > + return -1; > + nc = eth_da.nb_mh_controllers > 0 ? eth_da.nb_mh_controllers : 1; > + np = eth_da.nb_ports > 0 ? eth_da.nb_ports : 1; > + nf = eth_da.nb_representor_ports > 0 ? eth_da.nb_representor_ports : 1; > + > /* Return 0 if representor id is matching one of the values. */ > - for (index = 0; index < representors.nb_representor_ports; index++) > - if (data->representor_id == > - representors.representor_ports[index]) > + for (i = 0; i < nc * np * nf; ++i) { > + c = i / (np * nf); > + p = (i / nf) % np; > + f = i % nf; > + if (rte_eth_representor_id_convert(edev, > + &id, > + eth_da.type, > + eth_da.nb_mh_controllers == 0 ? -1 : > + eth_da.mh_controllers[c], > + eth_da.nb_ports == 0 ? -1 : eth_da.ports[p], > + eth_da.nb_representor_ports == 0 ? -1 : > + eth_da.representor_ports[f]) < 0) > + continue; > + if (data->representor_id == id) > return 0; > + } > return -1; /* no match */ > } > > @@ -112,7 +132,7 @@ eth_dev_match(const struct rte_eth_dev *edev, > > ret = rte_kvargs_process(kvlist, > eth_params_keys[RTE_ETH_PARAM_REPRESENTOR], > - eth_representor_cmp, edev->data); > + eth_representor_cmp, (void *)(uintptr_t)edev); > if (ret != 0) > return -1; > /* search for representor key */ > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 07c6debb58..da0cf1a920 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -5617,6 +5617,89 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da) > return result; > } > > +int > +rte_eth_representor_id_convert(const struct rte_eth_dev *ethdev, > + uint16_t *id, > + enum rte_eth_representor_type type, > + int controller, int pf, int representor_port) > +{ > + int ret, n, i, count; > + struct rte_eth_representor_info *info = NULL; > + size_t size; > + > + if (type == RTE_ETH_REPRESENTOR_NONE) > + return 0; > + if (id == NULL) > + return -EINVAL; > + > + /* Get PMD representor range info. */ > + ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL); > + if (ret < 0) { > + /* Fallback to direct mapping for compatibility. */ > + *id = representor_port; I think it is a bad behaviour as I stated above. It is only if and only if type is VF, controller and PF are unspecified and representor_port is specified. > + } > + n = ret; > + size = sizeof(*info) + n * sizeof(info->ranges[0]); > + info = calloc(1, size); > + if (info == NULL) > + return -ENOMEM; > + ret = rte_eth_representor_info_get(ethdev->data->port_id, info); > + if (ret < 0) > + goto out; > + > + /* Default controller and pf to caller. */ > + if (controller == -1) > + controller = info->controller; > + if (pf == -1) > + pf = info->pf; > + > + /* Locate representor ID. */ > + for (i = 0; i < n; ++i) { > + if (info->ranges[i].type != type) > + continue; > + /* PMD hit: ignore controller if -1. */ > + if (info->ranges[i].controller != -1 && > + info->ranges[i].controller != (uint16_t)controller) > + continue; I think it is incorrect to ignore controller in range if controller is specified in request. It must match. > + count = info->ranges[i].id_end - info->ranges[i].id_base + 1; > + if (info->ranges[i].type == RTE_ETH_REPRESENTOR_PF) { > + /* PF. */ > + if (pf >= info->ranges[i].pf + count) > + continue; > + *id = info->ranges[i].id_base + > + (pf - info->ranges[i].pf); > + goto out; > + } > + /* VF or SF. */ > + /* PMD hit: ignore pf if -1. */ > + if (info->ranges[i].pf != -1 && > + info->ranges[i].pf != (uint16_t)pf) > + continue; Same for PF. > + if (info->ranges[i].type == RTE_ETH_REPRESENTOR_VF) { Typically switch/case looks a bit a bitter for such code. > + /* VF. */ The comment is useless > + if (representor_port >= info->ranges[i].vf + count) > + continue; > + *id = info->ranges[i].id_base + > + (representor_port - info->ranges[i].vf); > + goto out; > + } else if (info->ranges[i].type == RTE_ETH_REPRESENTOR_SF) { > + /* SF. */ The comment is useless > + if (representor_port >= info->ranges[i].sf + count) > + continue; > + *id = info->ranges[i].id_base + > + (representor_port - info->ranges[i].sf); > + goto out; > + } > + } > + /* Not matching representor ID range. */ > + ret = -ENOENT; > + > +out: > + if (info != NULL) > + free(info); There is no necessity to check against NULL above, free() does it in any case. > + return ret; > +} > + > static int > eth_dev_handle_port_list(const char *cmd __rte_unused, > const char *params __rte_unused, > diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map > index bb6f7436c2..2891f5734e 100644 > --- a/lib/librte_ethdev/version.map > +++ b/lib/librte_ethdev/version.map > @@ -268,6 +268,7 @@ INTERNAL { > rte_eth_hairpin_queue_peer_bind; > rte_eth_hairpin_queue_peer_unbind; > rte_eth_hairpin_queue_peer_update; > + rte_eth_representor_id_convert; > rte_eth_switch_domain_alloc; > rte_eth_switch_domain_free; > }; >