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 16E66A054F; Tue, 2 Mar 2021 15:10:05 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9F28A1CC4DE; Tue, 2 Mar 2021 15:10:04 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3FA8C40142 for ; Tue, 2 Mar 2021 15:10:03 +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 ADCBD7F572; Tue, 2 Mar 2021 17:10:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru ADCBD7F572 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1614694202; bh=krpZLfp7e2nKyXfvnQrU4GhrZnw89cr7xa7HIAng/MA=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=cxUQ1EXlye6onbNomGj0GwlpReEhPIpmPbjiXfdKo7uiisyStCyV4QA7IBQtxaJbS bpXmAFqiCZaseqz2GSp1x6SB2G3/4kSp/ZTFortxezC2ZEZpn4MrKdc303Zo5V4DoM +iEKhxcQdMOZVaRaPLLdlmZAjbxYvCUNMtG5Z4Nk= To: Xueming Li Cc: dev@dpdk.org, Viacheslav Ovsiienko , Asaf Penso , Thomas Monjalon , Ferruh Yigit , Ray Kinsella , Neil Horman References: <1608303356-13089-2-git-send-email-xuemingl@nvidia.com> <20210302114346.1463-1-xuemingl@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <7986bd6d-fd38-abb3-ea06-1a20fe27d18b@oktetlabs.ru> Date: Tue, 2 Mar 2021 17:10:02 +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: <20210302114346.1463-1-xuemingl@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v7 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 3/2/21 2:43 PM, 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 | 91 +++++++++++++++++++++++++++++++ > lib/librte_ethdev/version.map | 1 + > 4 files changed, 153 insertions(+), 9 deletions(-) > > diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h > index 7b0f392e34..3ff1b90b2f 100644 > --- a/lib/librte_ethdev/ethdev_driver.h > +++ b/lib/librte_ethdev/ethdev_driver.h > @@ -1244,6 +1244,38 @@ struct rte_eth_devargs { > enum rte_eth_representor_type type; /* type of representor */ > }; > > +/** > + * PMD helper function to convert representor ID from location detail Full stop is missing above. Also I'm not sure in term "convert" here. It sounds a bit misleading to me. May be just "get". I.e PMD helper function to get representor ID by location detail. and rte_eth_representor_id_get() What do you think? > + * > + * Convert representor ID from controller, pf and (sf or vf). > + * The mapping is retrieved from rte_eth_representor_info_get(). > + * > + * For backward compatibility, if no representor info, direct > + * map legacy VF (no controller and pf). > + * > + * @param ethdev > + * Handle of ethdev port. > + * @param repr_id > + * Pointer to converted representor ID. > + * @param type > + * Representor type. > + * @param controller > + * Controller ID, -1 if unspecified. > + * @param pf > + * PF port ID, -1 if unspecified. > + * @param representor_port > + * VF or SF representor port number, -1 if unspecified. Mixing input and output parameters looks bad to me. May I suggest to put repr_id the last? I.e. to have all input parameters first. (May be I've already mentioned it, if I missed your reply, please, repeat it once again). > + * > + * @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 *repr_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 c88e345e7d..78cdef11be 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -5623,6 +5623,97 @@ 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 *repr_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 (repr_id == NULL) > + return -EINVAL; > + > + /* Get PMD representor range info. */ > + ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL); > + if (ret < 0) { > + if (type == RTE_ETH_REPRESENTOR_VF && controller == -1 && > + pf == -1) { > + /* Direct mapping for legacy VF representor. */ > + *repr_id = representor_port; > + return 0; > + } else { > + return ret; > + } > + } > + 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. */ > + ret = -ENOENT; > + 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) First of all I don't understand why 'controller' is cast to uint16_t here. Both 'controller' and range->controller have 'int' type. Second, I'm sorry, but I still don't understand the ignore logic. Why information retrieval may return -1 for controller and/or PF? What does it mean? Above fallback to the device controller and pf if unspecified by the caller look good and make sense. > + continue; > + count = info->ranges[i].id_end - info->ranges[i].id_base + 1; > + switch (info->ranges[i].type) { > + case RTE_ETH_REPRESENTOR_PF: > + if (pf >= info->ranges[i].pf + count) > + continue; Condition must be stricter. We must ensure that requested port within both boundaries of the range. I.e. representor_port should not be smaller than info->ranges[i].pf. It is required for below subtraction. > + *repr_id = info->ranges[i].id_base + > + (pf - info->ranges[i].pf); > + ret = 0; > + goto out; > + case RTE_ETH_REPRESENTOR_VF: > + /* PMD hit: ignore pf if -1. */ > + if (info->ranges[i].pf != -1 && > + info->ranges[i].pf != (uint16_t)pf) Same as above. Cast seems to be not required. > + continue; > + if (representor_port >= info->ranges[i].vf + count) Same as above. > + continue; > + *repr_id = info->ranges[i].id_base + > + (representor_port - info->ranges[i].vf); > + ret = 0; > + goto out; > + case RTE_ETH_REPRESENTOR_SF: > + /* PMD hit: ignore pf if -1. */ > + if (info->ranges[i].pf != -1 && > + info->ranges[i].pf != (uint16_t)pf) Same as above. Cast seems to be not required. > + continue; > + if (representor_port >= info->ranges[i].sf + count) > + continue; Same as above. > + *repr_id = info->ranges[i].id_base + > + (representor_port - info->ranges[i].sf); > + ret = 0; > + goto out; > + default: > + break; > + } > + } > +out: > + free(info); > + 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; > }; >