From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from na01-bn1-obe.outbound.protection.outlook.com (mail-bn1bon0111.outbound.protection.outlook.com [157.56.111.111]) by dpdk.org (Postfix) with ESMTP id 7E21AB62 for ; Fri, 27 Mar 2015 08:59:55 +0100 (CET) Received: from BN3PR03MB1333.namprd03.prod.outlook.com (25.163.34.144) by BN3PR03MB1333.namprd03.prod.outlook.com (25.163.34.144) with Microsoft SMTP Server (TLS) id 15.1.125.19; Fri, 27 Mar 2015 07:59:53 +0000 Received: from BN3PR03MB1333.namprd03.prod.outlook.com ([25.163.34.144]) by BN3PR03MB1333.namprd03.prod.outlook.com ([25.163.34.144]) with mapi id 15.01.0125.002; Fri, 27 Mar 2015 07:59:53 +0000 From: Xuelin Shi To: "Ananyev, Konstantin" Thread-Topic: [dpdk-dev] [PATCH v2] ixgbe: fix ixgbe access endian issue on bigendian arch Thread-Index: AQHQaCn78cgXG+Yux0qTLu+oDNIJVJ0v8r5Q Date: Fri, 27 Mar 2015 07:59:52 +0000 Message-ID: References: <1427178825-4172-1-git-send-email-xuelin.shi@freescale.com> <2601191342CEEE43887BDE71AB97725821407D74@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725821407D74@irsmsx105.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [199.59.231.64] authentication-results: intel.com; dkim=none (message not signed) header.d=none; x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN3PR03MB1333; x-forefront-antispam-report: BMV:1; SFV:NSPM; SFS:(10019020)(6009001)(51704005)(377454003)(164054003)(13464003)(76176999)(106116001)(76576001)(19580405001)(54356999)(74316001)(110136001)(19580395003)(86362001)(50986999)(99286002)(46102003)(2950100001)(2900100001)(40100003)(33656002)(62966003)(77156002)(66066001)(92566002)(87936001)(2656002)(102836002); DIR:OUT; SFP:1102; SCL:1; SRVR:BN3PR03MB1333; H:BN3PR03MB1333.namprd03.prod.outlook.com; FPR:; SPF:None; MLV:sfv; LANG:en; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(601004)(5002010)(5005006); SRVR:BN3PR03MB1333; BCL:0; PCL:0; RULEID:; SRVR:BN3PR03MB1333; x-forefront-prvs: 0528942FD8 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Mar 2015 07:59:52.8044 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR03MB1333 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: fix ixgbe access endian issue on bigendian arch X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Mar 2015 07:59:56 -0000 Hi, Please see my comments inline. Thanks, Xuelin > -----Original Message----- > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] > Sent: Friday, March 27, 2015 09:04 > To: Shi Xuelin-B29237; thomas.monjalon@6wind.com > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v2] ixgbe: fix ixgbe access endian issue > on bigendian arch >=20 > Hi, >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of > > xuelin.shi@freescale.com > > Sent: Tuesday, March 24, 2015 6:34 AM > > To: thomas.monjalon@6wind.com > > Cc: dev@dpdk.org; Xuelin Shi > > Subject: [dpdk-dev] [PATCH v2] ixgbe: fix ixgbe access endian issue on > > bigendian arch > > > > From: Xuelin Shi > > > > enforce rules of the cpu and ixgbe exchange data. > > 1. cpu use data owned by ixgbe must use rte_le_to_cpu_xx(...) 2. cpu > > fill data to ixgbe must use rte_cpu_to_le_xx(...) > > > > Signed-off-by: Xuelin Shi > > --- > > changes for v2: > > rebased on latest head. > > fix some style issue detected by checkpatch.pl > > > > lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 104 > > ++++++++++++++++++++++++-------------- > > 1 file changed, 66 insertions(+), 38 deletions(-) > > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > index 9da2c7e..961ac08 100644 > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > @@ -128,7 +128,7 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq) > > int i; > > > > /* check DD bit on threshold descriptor */ > > - status =3D txq->tx_ring[txq->tx_next_dd].wb.status; > > + status =3D rte_le_to_cpu_32(txq->tx_ring[txq->tx_next_dd].wb.status); > > if (! (status & IXGBE_ADVTXD_STAT_DD)) > > return 0; > > > > @@ -174,11 +174,14 @@ tx4(volatile union ixgbe_adv_tx_desc *txdp, > struct rte_mbuf **pkts) > > pkt_len =3D (*pkts)->data_len; > > > > /* write data to descriptor */ > > - txdp->read.buffer_addr =3D buf_dma_addr; > > + txdp->read.buffer_addr =3D rte_cpu_to_le_64(buf_dma_addr); > > + > > txdp->read.cmd_type_len =3D > > - ((uint32_t)DCMD_DTYP_FLAGS | pkt_len); > > + rte_cpu_to_le_32((uint32_t)DCMD_DTYP_FLAGS | pkt_len); > > + > > txdp->read.olinfo_status =3D > > - (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT); > > + rte_cpu_to_le_32(pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT); > > + > > rte_prefetch0(&(*pkts)->pool); > > } > > } > > @@ -194,11 +197,14 @@ tx1(volatile union ixgbe_adv_tx_desc *txdp, > struct rte_mbuf **pkts) > > pkt_len =3D (*pkts)->data_len; > > > > /* write data to descriptor */ > > - txdp->read.buffer_addr =3D buf_dma_addr; > > + txdp->read.buffer_addr =3D rte_cpu_to_le_64(buf_dma_addr); > > + > > txdp->read.cmd_type_len =3D > > - ((uint32_t)DCMD_DTYP_FLAGS | pkt_len); > > + rte_cpu_to_le_32((uint32_t)DCMD_DTYP_FLAGS | pkt_len); > > + > > txdp->read.olinfo_status =3D > > - (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT); > > + rte_cpu_to_le_32(pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT); > > + > > rte_prefetch0(&(*pkts)->pool); > > } > > > > @@ -285,7 +291,7 @@ tx_xmit_pkts(void *tx_queue, struct rte_mbuf > **tx_pkts, > > * a divisor of the ring size > > */ > > tx_r[txq->tx_next_rs].read.cmd_type_len |=3D > > - rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS); > > + rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS); > > txq->tx_next_rs =3D (uint16_t)(txq->tx_rs_thresh - 1); > > > > txq->tx_tail =3D 0; > > @@ -304,7 +310,7 @@ tx_xmit_pkts(void *tx_queue, struct rte_mbuf > **tx_pkts, > > */ > > if (txq->tx_tail > txq->tx_next_rs) { > > tx_r[txq->tx_next_rs].read.cmd_type_len |=3D > > - rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS); > > + rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS); > > txq->tx_next_rs =3D (uint16_t)(txq->tx_next_rs + > > txq->tx_rs_thresh); > > if (txq->tx_next_rs >=3D txq->nb_tx_desc) @@ -505,6 +511,7 @@ > > ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq) > > uint16_t nb_tx_desc =3D txq->nb_tx_desc; > > uint16_t desc_to_clean_to; > > uint16_t nb_tx_to_clean; > > + uint32_t stat; > > > > /* Determine the last descriptor needing to be cleaned */ > > desc_to_clean_to =3D (uint16_t)(last_desc_cleaned + > > txq->tx_rs_thresh); @@ -513,7 +520,9 @@ ixgbe_xmit_cleanup(struct > > ixgbe_tx_queue *txq) > > > > /* Check to make sure the last descriptor to clean is done */ > > desc_to_clean_to =3D sw_ring[desc_to_clean_to].last_id; > > - if (! (txr[desc_to_clean_to].wb.status & IXGBE_TXD_STAT_DD)) > > + > > + stat =3D rte_le_to_cpu_32(txr[desc_to_clean_to].wb.status); > > + if (!(stat & IXGBE_TXD_STAT_DD)) > > { > > PMD_TX_FREE_LOG(DEBUG, > > "TX descriptor %4u is not done" > > @@ -544,7 +553,7 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq) > > * up to the last descriptor with the RS bit set > > * are done. Only reset the threshold descriptor. > > */ > > - txr[desc_to_clean_to].wb.status =3D 0; > > + txr[desc_to_clean_to].wb.status =3D rte_cpu_to_le_32(0); >=20 > Don't think you need swap bytes here. Yes, it's to accentuate the endian rule. I'm expecting compiler will optimi= zing it. >=20 > > > > /* Update the txq to reflect the last descriptor that was cleaned > */ > > txq->last_desc_cleaned =3D desc_to_clean_to; @@ -801,12 +810,14 @@ > > ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, > > */ > > slen =3D m_seg->data_len; > > buf_dma_addr =3D RTE_MBUF_DATA_DMA_ADDR(m_seg); > > + > > txd->read.buffer_addr =3D > > - rte_cpu_to_le_64(buf_dma_addr); > > + rte_cpu_to_le_64(buf_dma_addr); > > txd->read.cmd_type_len =3D > > - rte_cpu_to_le_32(cmd_type_len | slen); > > + rte_cpu_to_le_32(cmd_type_len | slen); > > txd->read.olinfo_status =3D > > - rte_cpu_to_le_32(olinfo_status); > > + rte_cpu_to_le_32(olinfo_status); > > + > > txe->last_id =3D tx_last; > > tx_id =3D txe->next_id; > > txe =3D txn; > > @@ -946,14 +957,16 @@ ixgbe_rx_scan_hw_ring(struct ixgbe_rx_queue *rxq) > > uint64_t pkt_flags; > > int s[LOOK_AHEAD], nb_dd; > > int i, j, nb_rx =3D 0; > > + uint32_t stat; > > > > > > /* get references to current descriptor and S/W ring entry */ > > rxdp =3D &rxq->rx_ring[rxq->rx_tail]; > > rxep =3D &rxq->sw_ring[rxq->rx_tail]; > > > > + stat =3D rte_le_to_cpu_32(rxdp->wb.upper.status_error); > > /* check to make sure there is at least 1 packet to receive */ > > - if (! (rxdp->wb.upper.status_error & IXGBE_RXDADV_STAT_DD)) > > + if (!(stat & IXGBE_RXDADV_STAT_DD)) > > return 0; > > > > /* > > @@ -965,7 +978,7 @@ ixgbe_rx_scan_hw_ring(struct ixgbe_rx_queue *rxq) > > { > > /* Read desc statuses backwards to avoid race condition */ > > for (j =3D LOOK_AHEAD-1; j >=3D 0; --j) > > - s[j] =3D rxdp[j].wb.upper.status_error; > > + s[j] =3D rte_le_to_cpu_32(rxdp[j].wb.upper.status_error); > > > > /* Compute how many status bits were set */ > > nb_dd =3D 0; > > @@ -976,28 +989,36 @@ ixgbe_rx_scan_hw_ring(struct ixgbe_rx_queue > > *rxq) > > > > /* Translate descriptor info to mbuf format */ > > for (j =3D 0; j < nb_dd; ++j) { > > + uint16_t tmp16; > > + uint32_t tmp32; > > + > > mb =3D rxep[j].mbuf; > > - pkt_len =3D (uint16_t)(rxdp[j].wb.upper.length - rxq- > >crc_len); > > + tmp16 =3D rte_le_to_cpu_16(rxdp[j].wb.upper.length); > > + pkt_len =3D tmp16 - rxq->crc_len; >=20 > Here and in other places, wonder why do you introduce temporary variables= ? > Why not just: > pkt_len =3D rte_le_to_cpu_16( (rxdp[j].wb.upper.length) - rxq->crc_len; ? Comply with 80 char width limit style >=20 > > mb->data_len =3D pkt_len; > > mb->pkt_len =3D pkt_len; > > - mb->vlan_tci =3D rxdp[j].wb.upper.vlan; > > mb->vlan_tci =3D rte_le_to_cpu_16(rxdp[j].wb.upper.vlan); > > > > /* convert descriptor fields to rte mbuf flags */ > > - pkt_flags =3D rx_desc_hlen_type_rss_to_pkt_flags( > > + tmp32 =3D rte_le_to_cpu_32( > > rxdp[j].wb.lower.lo_dword.data); >=20 >=20 > Same here, why just not: > pkt_flags =3D > rx_desc_hlen_type_rss_to_pkt_flags(rte_le_to_cpu_32(rxdp[j].wb.lower.lo_d > word.data)); > ? >=20 > > + pkt_flags =3D rx_desc_hlen_type_rss_to_pkt_flags(tmp32); > > + > > /* reuse status field from scan list */ > > pkt_flags |=3D rx_desc_status_to_pkt_flags(s[j]); > > pkt_flags |=3D rx_desc_error_to_pkt_flags(s[j]); > > mb->ol_flags =3D pkt_flags; > > > > if (likely(pkt_flags & PKT_RX_RSS_HASH)) > > - mb->hash.rss =3D rxdp[j].wb.lower.hi_dword.rss; > > + mb->hash.rss =3D rte_le_to_cpu_32( > > + rxdp[j].wb.lower.hi_dword.rss); > > else if (pkt_flags & PKT_RX_FDIR) { > > - mb->hash.fdir.hash =3D > > - > (uint16_t)((rxdp[j].wb.lower.hi_dword.csum_ip.csum) > > - & IXGBE_ATR_HASH_MASK); > > - mb->hash.fdir.id =3D > rxdp[j].wb.lower.hi_dword.csum_ip.ip_id; > > + tmp16 =3D rxdp[j].wb.lower.hi_dword.csum_ip.csum; > > + mb->hash.fdir.hash =3D rte_le_to_cpu_16( > > + tmp16 & IXGBE_ATR_HASH_MASK); >=20 > Shouldn't it be: > rte_le_to_cpu_16(tmp16) & IXGBE_ATR_HASH_MASK; ? Because the value of IXGBE_ATR_HASH_MASK is assumed as little endian.=20 >=20 > > + > > + tmp16 =3D rxdp[j].wb.lower.hi_dword.csum_ip.ip_id; > > + mb->hash.fdir.id =3D rte_le_to_cpu_16(tmp16); > > } > > } > > > > @@ -1051,8 +1072,8 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq) > > > > /* populate the descriptors */ > > dma_addr =3D > rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb)); > > - rxdp[i].read.hdr_addr =3D dma_addr; > > - rxdp[i].read.pkt_addr =3D dma_addr; > > + rxdp[i].read.hdr_addr =3D rte_cpu_to_le_64(dma_addr); > > + rxdp[i].read.pkt_addr =3D rte_cpu_to_le_64(dma_addr); >=20 > No need these 2 swaps here. > dma_addr already converted to LE, just a line above. OK, these two should be removed. >=20 > > } > > > > /* update tail pointer */ > > @@ -1220,8 +1241,8 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > > * using invalid descriptor fields when read from rxd. > > */ > > rxdp =3D &rx_ring[rx_id]; > > - staterr =3D rxdp->wb.upper.status_error; > > - if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) > > + staterr =3D rte_le_to_cpu_32(rxdp->wb.upper.status_error); > > + if (!(staterr & IXGBE_RXDADV_STAT_DD)) >=20 > Why do you need that change here? > Original code seems also ok to me. I think it is more clear. We read some value from PCIe and change it to cpu= format. I accentuate the rules of read/write between cpu and PCIe. >=20 >=20 > > break; > > rxd =3D *rxdp; > > > > @@ -1325,12 +1346,17 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > > rxm->ol_flags =3D pkt_flags; > > > > if (likely(pkt_flags & PKT_RX_RSS_HASH)) > > - rxm->hash.rss =3D rxd.wb.lower.hi_dword.rss; > > + rxm->hash.rss =3D rte_le_to_cpu_32( > > + rxd.wb.lower.hi_dword.rss); > > else if (pkt_flags & PKT_RX_FDIR) { > > - rxm->hash.fdir.hash =3D > > - (uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum) > > - & IXGBE_ATR_HASH_MASK); > > - rxm->hash.fdir.id =3D rxd.wb.lower.hi_dword.csum_ip.ip_id; > > + uint16_t tmp16; > > + > > + tmp16 =3D rxd.wb.lower.hi_dword.csum_ip.csum; > > + rxm->hash.fdir.hash =3D rte_le_to_cpu_16( > > + tmp16 & IXGBE_ATR_HASH_MASK); >=20 >=20 > Same thing here, seems should be: rte_le_to_cpu_16(tmp16) & > IXGBE_ATR_HASH_MASK; >=20 > > + > > + rxm->hash.fdir.id =3D rte_le_to_cpu_16( > > + rxd.wb.lower.hi_dword.csum_ip.ip_id); > > } > > /* > > * Store the mbuf address into the next entry of the array @@ > > -1412,8 +1438,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct > rte_mbuf **rx_pkts, > > * using invalid descriptor fields when read from rxd. > > */ > > rxdp =3D &rx_ring[rx_id]; > > - staterr =3D rxdp->wb.upper.status_error; > > - if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) > > + staterr =3D rte_cpu_to_le_32(rxdp->wb.upper.status_error); > > + if (!(staterr & IXGBE_RXDADV_STAT_DD)) >=20 >=20 > Again, seems unnecessary. >=20 > > break; > > rxd =3D *rxdp; > > > > @@ -1742,7 +1768,7 @@ ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq) > > prev =3D (uint16_t) (txq->nb_tx_desc - 1); > > for (i =3D 0; i < txq->nb_tx_desc; i++) { > > volatile union ixgbe_adv_tx_desc *txd =3D &txq->tx_ring[i]; > > - txd->wb.status =3D IXGBE_TXD_STAT_DD; > > + txd->wb.status =3D rte_cpu_to_le_32(IXGBE_TXD_STAT_DD); > > txe[i].mbuf =3D NULL; > > txe[i].last_id =3D i; > > txe[prev].next_id =3D i; > > @@ -2293,7 +2319,8 @@ ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev, > uint16_t rx_queue_id) > > rxdp =3D &(rxq->rx_ring[rxq->rx_tail]); > > > > while ((desc < rxq->nb_rx_desc) && > > - (rxdp->wb.upper.status_error & IXGBE_RXDADV_STAT_DD)) { > > + (rte_le_to_cpu_32(rxdp->wb.upper.status_error) & > > + IXGBE_RXDADV_STAT_DD)) { > > desc +=3D IXGBE_RXQ_SCAN_INTERVAL; > > rxdp +=3D IXGBE_RXQ_SCAN_INTERVAL; > > if (rxq->rx_tail + desc >=3D rxq->nb_rx_desc) @@ -2318,7 > +2345,8 @@ > > ixgbe_dev_rx_descriptor_done(void *rx_queue, uint16_t offset) > > desc -=3D rxq->nb_rx_desc; > > > > rxdp =3D &rxq->rx_ring[desc]; > > - return !!(rxdp->wb.upper.status_error & IXGBE_RXDADV_STAT_DD); > > + return !!(rte_le_to_cpu_32(rxdp->wb.upper.status_error) & > > + IXGBE_RXDADV_STAT_DD); > > } > > > > void > > -- > > 1.9.1