[dpdk-dev] [PATCH v3 00/34] A new net PMD - ice

Lu, Wenzhuo wenzhuo.lu at intel.com
Thu Dec 13 08:10:04 CET 2018


Hi Vipin,


> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, December 13, 2018 2:02 PM
> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3 00/34] A new net PMD - ice
> 
> Hi,
> 
> > -----Original Message-----
> > From: dev <dev-bounces at dpdk.org> On Behalf Of Wenzhuo Lu
> > Sent: Wednesday, December 12, 2018 12:30 PM
> > To: dev at dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>
> > Subject: [dpdk-dev] [PATCH v3 00/34] A new net PMD - ice
> >
> > This patch set adds the support of a new net PMD, Intel® Ethernet
> > Network Adapters E810, also called ice.
> >
> > Besides enabling this new NIC, also some other features supported on this
> NIC.
> 
> Can you mention the other features
Sorry for misleading you. The other features is below features. I'll reword it.

> 
> > Like below,
> >
> > Basic features:
> > 1, Basic device operations: probe, initialization, start/stop, configure, info
> get.
> > 2, RX/TX queue operations: setup/release, start/stop, info get.
> > 3, RX/TX.
> >
> > HW Offload features:
> > 1, CRC Stripping/insertion.
> > 2, L2/L3 checksum strip/insertion.
> > 3, PVID set.
> > 4, TPID change.
> > 5, TSO (LRO/RSC not supported).
> >
> > Stats:
> > 1, statics & xstatics.
> >
> > Switch functions:
> > 1, MAC Filter Add/Delete.
> > 2, VLAN Filter Add/Delete.
> >
> > Power saving:
> > 1, RX interrupt mode.
> >
> > Misc:
> > 1, Interrupt For Link Status.
> > 2, firmware info query.
> > 3, Jumbo Frame Support.
> > 4, ptype check.
> > 5, EEPROM check and set.
> >
> 
> Can you add section to highlight the changes with "---". This is part of
> 'http://doc.dpdk.org/guides/contributing/patches.html' for 'This can be
> added to the cover letter or the annotations'
Will add it.

> 
> > v2:
> >  - Fix shared lib compile issue.
> >  - Add meson build support.
> >  - Update documents.
> >  - Fix more checkpatch issues.
> >
> > v3:
> >  - Removed the support of secondary process.
> >  - Splitted the base code to more patches.
> >  - Pass NULL to rte_zmalloc.
> >  - Changed some magic numbers to macros.
> >  - Fixed the wrong implementation of a specific bitmap.
> 
> Not all comments are addressed or closed from V2. So I have to assume you
> will be doing the same for v4.
> 
> 
> Some of the items
> 
> [PATCH v2 03/20] net/ice: support device and queue ops
> > > > > +
> > > > > +static int
> > > > > +ice_dev_start(struct rte_eth_dev *dev) {
> > > > > +	struct rte_eth_dev_data *data = dev->data;
> > > > > +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> > > > > >dev_private);
> > > > > +	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-
> > > > >dev_private);
> > > > > +	uint16_t nb_rxq = 0;
> > > > > +	uint16_t nb_txq, i;
> > > > > +	int ret;
> > > > > +
> > > > > +	ICE_PROC_SECONDARY_CHECK;
> > > >
> > > > Device start is not supported, but how is this differentiated from
> > > > primary configured device vs secondary configured device.
> > > >
> > > > Ie: primary uses black list '-b BB:DD:F' while secondary uses '-w
> > > > BB:DD:F'. In this case since we are checking process type this
> > > > will return without
> > > start?
> > Two updates with respect to your comment, 1. tool and application like
> > dpdk-procinfo will no longer be able pull data since you are asking to black
> list.
> > 2. If there are functions which needs to shared, like primary using rx-0 and
> tx-0 then secondary rx-1 and tx-1 how to make this work?
> 
> 
> [PATCH v2 01/20] net/ice: add base code
> > Note: In version 1 I enquired about unit or DTS validation for PMD. Is
> > this still holding good?
> Yes, it's planned and on going.
I confirmed validation is going on this device. But not impact this patch set unless we  found bug and need to fix it.

> 
> [PATCH v2 02/20] net/ice: support device initialization
> > +# Compile burst-oriented ICE PMD driver #
> CONFIG_RTE_LIBRTE_ICE_PMD=y
> 
> Based on ' https://patches.dpdk.org/patch/48488/' it is suggested to
> configure. But here is it already set to 'y'. Is this correct? If yes can you
> update ' https://patches.dpdk.org/patch/48488/'
We've discussed the setting. I don’t know anything left. If there's, please let me know.

> 
>  [PATCH v2 20/20] net/ice: support meson build
> > > > > Should not meson build option be add start. That is in patch
> > > > > 1/20 so compile options does not fail?
> > > > It will not fail. Enabling the compile earlier only means the code
> > > > can be
> > > compiled.
> > > > But, to use this device we do need the whole patch set. From this
> > > > point of view, compiling it at the end maybe better.
> > > Thanks for update, so will 'meson-build' success if apply 3 patches?
> > Sure, meson build will not be broken by any one of these patches. Only
> > until this patch, what built by meson can support ice.
> Thanks for confirmation that you have tried './devtools/test-meson-
> builds.sh' and the intermediate build for ICE_DSI PMD does not fail.
I said meson build is working. But don't know what's ICE_DSI PMD.

> 
>  [PATCH v2 16/20] net/ice: support basic RX/TX
> 
>  [PATCH v2 14/20] net/ice: support statistics
>  > > > +	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
> > > > +			     ns->eth.tx_broadcast) * ETHER_CRC_LEN;
> > >
> > > In earlier patch for 'mtu set check' we added VSI SWITCH VLAN.
> > > Should we add VSI VLAN here?
> > Don't need. They're different functions. We add crc length here
> > because of HW counts the packet length before crc is added.
> So you are not fetching stats from HW registers from switch is this correct?
> How will you get stats for actually transmitted in xstats? As I understand
> xstats is for switch HW stats right?
No. The stats is got from HW. We just correct it as HW doesn’t have the chance to add CRC length. But I don't understand why switch is mentioned here.

> 
>  [PATCH v2 04/20] net/ice: support getting	device	information
>  > > Does this mean per queue offload capability is not supported? If
> > > yes, can you mention this in release notes under 'support or limitation'
> > No, it's not supported. We have a document, ice.ini, to list all the
> > features supported. All the others are not supported.
> > BTW, I don't think anything not supported is limitation.
> If I understand correctly,  ICE_DSI_PMD is advertising it has not offload for
> RX and TX. But you are stating in ice.ini you are listing offload supports. So
> let me rephrase the question 'if you support port level offload capability, it
> will reflect for all queues rx and tx. But if you reflect queue level offload as 0
> for rx and tx, then APIs rte_eth_rx_queue_setup and
> rte_eth_tx_queue_setup if queue offload enabled should fail. Is this correct
> understanding?'
Sorry, I don’t know what's ICE_DSI_PMD.

> 
> > > If device switch is not configured (default value from NVM) should
> > > we highlight the switch can support speed 10, 100, 1000, 1000 and son
> on?
> > No, this's the capability getting from HW.
> If HW is supported or configured for 10, 100, 25G then those should be
> returned correctly this I agree. But when the device is queried for capability
> it should highlight all supported speeds of switch. Am I right?
No. Here shows the result not all the speeds supported. Like the speed after auto negotiation.

> 
> > > If speed is not true as stated above, can you please add this to
> > > release notes and documentation.
> > Here listed all the case we can get from HW.
> Please add to ice_dsi documentation also.
Sorry, no idea about ice_dsi.

> 
> snipped



More information about the dev mailing list