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 3EDBFA034E; Mon, 14 Feb 2022 17:54:01 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2EB5C41140; Mon, 14 Feb 2022 17:54:01 +0100 (CET) Received: from office2.cesnet.cz (office2.cesnet.cz [195.113.144.244]) by mails.dpdk.org (Postfix) with ESMTP id DB6854067E for ; Mon, 14 Feb 2022 17:53:59 +0100 (CET) Received: from [IPv6:2001:67c:1220:80c:eb:e00e:9e81:ff5f] (unknown [IPv6:2001:67c:1220:80c:eb:e00e:9e81:ff5f]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by office2.cesnet.cz (Postfix) with ESMTPSA id AE775400074; Mon, 14 Feb 2022 17:53:59 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cesnet.cz; s=office2-2020; t=1644857639; bh=GZPzOi5VyypNaEGtb9IkY2bK0gXbPairLeS80TiHErY=; h=Subject:From:To:Date:In-Reply-To:References; b=QoHnKwOoky2xzMDQen2AeHGRJPV+2I7WSiz9iFpH8AK8rgqs4CdlejeHzK0h+AsLz m2Y85XbO3mGVhrqLQo++fX95UC/CEtGfM8werFX/XYIKq1bqCV8p05+Gsnyi8fbMvx 5q4Ut8tmjje7oiKdWJeipW2UpDX8Pf+rWMZNCHoTYyncBSYIxvowX8zVD395RyCP4O M74IIi+afHh5Vd72XQr/iZ8Ldu98li9Q4AGmqZrTjK5HoRKeC9lXDaThJWoG5TYW6R eIJhTaRZyJnoMXQxT35Tbu20ojm/Tx3RqyExc4+yrLv/WUeawojnkAGDSQE+GUKSNa apAlA5grqhR/w== Message-ID: <2aa9023f3d44d286b9d5ba105e3a9bbc456416f6.camel@cesnet.cz> Subject: Re: [PATCH 6/6] drivers/nfb: add support for more MAC addresses From: Martin Spinler To: Ferruh Yigit , dev@dpdk.org Date: Mon, 14 Feb 2022 17:53:59 +0100 In-Reply-To: References: <20220214112541.29782-1-spinler@cesnet.cz> <20220214112541.29782-6-spinler@cesnet.cz> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.4 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit 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 On Mon, 2022-02-14 at 13:37 +0000, Ferruh Yigit wrote: > On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote: > > From: Martin Spinler > > > > Extend the eth_dev_ops by add/remove MAC address functions. > > > > Signed-off-by: Martin Spinler > > --- > > drivers/net/nfb/nfb_ethdev.c | 69 ++++++++++++++++++++++++++++++------ > > 1 file changed, 58 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c > > index 5d503e131a..7dec8e022e 100644 > > --- a/drivers/net/nfb/nfb_ethdev.c > > +++ b/drivers/net/nfb/nfb_ethdev.c > > @@ -214,7 +214,19 @@ static int > > nfb_eth_dev_info(struct rte_eth_dev *dev, > > struct rte_eth_dev_info *dev_info) > > { > > - dev_info->max_mac_addrs = 1; > > + uint16_t i; > > + uint32_t max_mac_addrs; > > + struct pmd_internals *internals = dev->data->dev_private; > > + > > + dev_info->max_mac_addrs = (uint32_t)-1; > > + for (i = 0; i < internals->max_rxmac; i++) { > > + max_mac_addrs = nc_rxmac_mac_address_count(internals->rxmac[i]); > > + dev_info->max_mac_addrs = RTE_MIN(max_mac_addrs, > > + dev_info->max_mac_addrs); > > + } > > + if (internals->max_rxmac == 0) > > + dev_info->max_mac_addrs = 0; > > + > > Not sure if 'max_mac_addrs = 0' is valid value, as far as I can see > driver allocates memory for at least one MAC address, so there is no > case that NIC doesn't support any MAC address. Oh, sorry, I missed that, will be fixed in v2. > > It looks like max_mac_addrs usage is not clear, what is exactly the > 'max_rxmac' & 'rxmac[]' variables are? rxmac is a firmware unit which represents the RX side of one physical Ethernet port (more precisely one logical Eth channel on port) and handles stats+CRC check+MAC filtering. So this goes through all those units and finds the rxmac with minimal memory space for MAC addresses and uses this value. I'll add few comments in the code to v2. > > > dev_info->max_rx_pktlen = (uint32_t)-1; > > dev_info->max_rx_queues = dev->data->nb_rx_queues; > > dev_info->max_tx_queues = dev->data->nb_tx_queues; > > @@ -376,6 +388,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev) > > return 0; > > } > > > > +static uint64_t > > +nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr) > > +{ > > + int i; > > + uint64_t res = 0; > > + for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) { > > + res <<= 8; > > + res |= mac_addr->addr_bytes[i] & 0xFF; > > + } > > + return res; > > +} > > + > > /** > > * DPDK callback to set primary MAC address. > > * > > @@ -392,26 +416,47 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev, > > struct rte_ether_addr *mac_addr) > > { > > unsigned int i; > > - uint64_t mac = 0; > > + uint64_t mac; > > struct rte_eth_dev_data *data = dev->data; > > struct pmd_internals *internals = (struct pmd_internals *) > > data->dev_private; > > > > - if (!rte_is_valid_assigned_ether_addr(mac_addr)) > > - return -EINVAL; > > + mac = nfb_eth_mac_addr_conv(mac_addr); > > + for (i = 0; i < internals->max_rxmac; ++i) > > + nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1); > > This functions is to set default MAC address 'data->mac_addrs[0]', > why same MAC set multiple times in a loop? Unfortunately, currently we don't have firmware support for proper separate port configuration, because DMA queues aren't bound to specific rxmac and traffic can be mixed between them. So I think the best way in this case is to write the same MAC address into all of the rxmacs inside firmware. > > > > > - for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) { > > - mac <<= 8; > > - mac |= mac_addr->addr_bytes[i] & 0xFF; > > - } > > + return 0; > > +} > > > > +static int > > +nfb_eth_mac_addr_add(struct rte_eth_dev *dev, > > + struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused) > > +{ > > + unsigned int i; > > + uint64_t mac = 0; > > + struct rte_eth_dev_data *data = dev->data; > > + struct pmd_internals *internals = (struct pmd_internals *) > > + data->dev_private; > > + > > + mac = nfb_eth_mac_addr_conv(mac_addr); > > for (i = 0; i < internals->max_rxmac; ++i) > > - nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1); > > + nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1); > > > > - rte_ether_addr_copy(mac_addr, data->mac_addrs); > > return 0; > > } > > > > +static void > > +nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index) > > +{ > > + unsigned int i; > > + struct rte_eth_dev_data *data = dev->data; > > + struct pmd_internals *internals = (struct pmd_internals *) > > + data->dev_private; > > + > > + for (i = 0; i < internals->max_rxmac; ++i) > > + nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0); > > +} > > + > > static const struct eth_dev_ops ops = { > > .dev_start = nfb_eth_dev_start, > > .dev_stop = nfb_eth_dev_stop, > > @@ -436,6 +481,8 @@ static const struct eth_dev_ops ops = { > > .stats_get = nfb_eth_stats_get, > > .stats_reset = nfb_eth_stats_reset, > > .mac_addr_set = nfb_eth_mac_addr_set, > > + .mac_addr_add = nfb_eth_mac_addr_add, > > + .mac_addr_remove = nfb_eth_mac_addr_remove, > > }; > > > > /** > > @@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev) > > eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1]; > > eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2]; > > > > - nfb_eth_mac_addr_set(dev, ð_addr_init); > > + rte_eth_dev_default_mac_addr_set(dev->data->port_id, ð_addr_init); > > > > I didn't get this change, why calling the API from the driver, > instead of calling the dev_ops directly as original code did? The API does all the checks and copies the MAC value into internal data struct, so our code was duplicit. I didn't realize that calling the API could be problem. I assume it should be reverted to the prev form? > > > data->promiscuous = nfb_eth_promiscuous_get(dev); > > data->all_multicast = nfb_eth_allmulticast_get(dev); >