[PATCH v2 1/5] net/ena: hal upgrade

[Brandes, Shai]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit at amd.com>
> Sent: Thursday, October 26, 2023 12:30 AM
> To: Brandes, Shai <shaibran at amazon.com>
> Cc: dev at dpdk.org; Beider, Ron <rbeider at amazon.com>; Atrash, Wajeeh
> <atrwajee at amazon.com>; Bernstein, Amit <amitbern at amazon.com>
> Subject: RE: [EXTERNAL] [PATCH v2 1/5] net/ena: hal upgrade
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On 10/25/2023 2:36 PM, shaibran at amazon.com wrote:
> > From: Shai Brandes <shaibran at amazon.com>
> >
> > ENA maintains a HAL that is shared by all supported host drivers.
> > Main features introduced to the HAL:
> > [1] Reworked the mechanism that queries the performance metrics
> >     from the device.
> > [2] Added support for a new metric that allows monitoring the
> >     available tracked connections.
> > [3] Added support for a new statistic that counts RX drops due
> >     to insufficient buffers provided by host.
> > [4] Added support for Scalable Reliable Datagram (SRD) metrics
> >     from ENA Express.
> > [5] Added support for querying the LLQ entry size recommendation
> >     from the device.
> > [6] Added support for PTP hardware clock (PHC) feature that
> >     provides enhanced accuracy (Not supported by the driver).
> > [7] Added support for new reset reasons for a suspected CPU
> >     starvation and for completion descriptor inconsistency.
> > [8] Aligned all return error code to a common notation.
> > [9] Removed an obsolete queue tail pointer update API.
> >
> > Signed-off-by: Shai Brandes <shaibran at amazon.com>
> > Reviewed-by: Amit Bernstein <amitbern at amazon.com>
> > ---
> >  doc/guides/rel_notes/release_23_11.rst        |   4 +
> >  drivers/net/ena/base/ena_com.c                | 499 +++++++++++++++---
> >  drivers/net/ena/base/ena_com.h                | 197 ++++++-
> >  .../net/ena/base/ena_defs/ena_admin_defs.h    | 198 ++++++-
> >  .../net/ena/base/ena_defs/ena_eth_io_defs.h   |  18 +-
> >  drivers/net/ena/base/ena_defs/ena_gen_info.h  |   4 +-
> >  drivers/net/ena/base/ena_defs/ena_regs_defs.h |  12 +
> >  drivers/net/ena/base/ena_eth_com.c            |  45 +-
> >  drivers/net/ena/base/ena_eth_com.h            |  30 +-
> >  drivers/net/ena/base/ena_plat.h               |   8 +-
> >  drivers/net/ena/base/ena_plat_dpdk.h          |  49 +-
> >  drivers/net/ena/ena_ethdev.c                  |  16 +-
> >  12 files changed, 915 insertions(+), 165 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_23_11.rst
> > b/doc/guides/rel_notes/release_23_11.rst
> > index 0a6fc76a9d..e3b0ba58c9 100644
> > --- a/doc/guides/rel_notes/release_23_11.rst
> > +++ b/doc/guides/rel_notes/release_23_11.rst
> > @@ -144,6 +144,10 @@ New Features
> >
> >    * Added support for Network Service Header (NSH) flow matching.
> >
> > +* **Updated Amazon Elastic Network Adapter ena net driver.**
> > +
> >
> 
> What do you think clarify as:
> 
> Updated Amazon ena (Elastic Network Adapter) net driver.
[Brandes, Shai] agree
> 
> 
> > +  * Upgraded ENA HAL to latest version.
> > +
> >
> 
> Updates should be ordered in vendor in the driver group, so can you please
> sort it based on 'Amazon'? With current release notes, it makes before Intel
> driver updates.
[Brandes, Shai] fixed
> 
> 
> 
> >  * **Updated Solarflare net driver.**
> >
> >    * Added support for transfer flow action ``INDIRECT`` with subtype
> ``VXLAN_ENCAP``.
> > diff --git a/drivers/net/ena/base/ena_com.c
> > b/drivers/net/ena/base/ena_com.c index 5ca36ab6d9..880d047956 100644
> > --- a/drivers/net/ena/base/ena_com.c
> > +++ b/drivers/net/ena/base/ena_com.c
> > @@ -38,6 +38,12 @@
> >
> >  #define ENA_MAX_ADMIN_POLL_US 5000
> >
> > +/* PHC definitions */
> > +#define ENA_PHC_DEFAULT_EXPIRE_TIMEOUT_USEC 20 #define
> > +ENA_PHC_DEFAULT_BLOCK_TIMEOUT_USEC 1000 #define
> > +ENA_PHC_TIMESTAMP_ERROR 0xFFFFFFFFFFFFFFFF #define
> > +ENA_PHC_REQ_ID_OFFSET 0xDEAD
> > +
> >
> >
> /**********************************************************
> ***********
> > ********/
> >
> /**********************************************************
> ***********
> > ********/
> >
> /**********************************************************
> ***********
> > ********/ @@ -70,7 +76,7 @@ static int ena_com_mem_addr_set(struct
> > ena_com_dev *ena_dev,
> >                                      dma_addr_t addr)  {
> >       if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> > -             ena_trc_err(ena_dev, "DMA address has more bits than the device
> supports\n");
> > +             ena_trc_err(ena_dev, "DMA address has more bits that the
> > + device supports\n");
> >
> 
> Original wording looks better to me, 'than' instead of 'that', but we can get
> another opinion from a native English speaker.
> 
> <...>
> 
[Brandes, Shai] agree, fixed 

> > diff --git a/drivers/net/ena/base/ena_plat.h
> > b/drivers/net/ena/base/ena_plat.h index 2583823080..a3649e0cb6 100644
> > --- a/drivers/net/ena/base/ena_plat.h
> > +++ b/drivers/net/ena/base/ena_plat.h
> > @@ -14,14 +14,16 @@
> >  #else
> >  #include <ena_plat_dpdk.h>
> >  #endif
> > +#elif defined(_WIN32)
> > +#include <ena_plat_windows.h>
> >  #elif defined(__FreeBSD__)
> > -#if defined(_KERNEL)
> > +#if defined(__KERNEL__)
> >  #include <ena_plat_fbsd.h>
> >  #else
> >  #include <ena_plat_dpdk.h>
> >  #endif
> > -#elif defined(_WIN32)
> > -#include <ena_plat_windows.h>
> > +#elif defined(__APPLE__)
> > +#include <ena_plat_macos.h>
> >  #else
> >  #error "Invalid platform"
> >  #endif
> >
> 
> As far as I can see only ena_plat_dpdk.h exists, other ena_plat_*.h files not
> exists at all in dpdk driver, would it be possible to strip those lines for the
> dpdk version of the ena_plat.h to not confuse about the support.
> 
> If this creates additional maintanence burden, OK to continue as it is.
> 
[Brandes, Shai] removed all, no problem

> 
> <...>
> 
> > @@ -107,6 +110,7 @@ extern int ena_logtype_com;  #define
> > BITS_PER_LONG_LONG (__SIZEOF_LONG_LONG__ * 8)  #define U64_C(x)
> x ##
> > ULL
> >  #define BIT(nr)         (1UL << (nr))
> > +#define BIT64(nr)    BIT(nr)
> >
> 
> Can use existing RTE_BIT64 / RTE_BIT32 macros
> 
> <...>
[Brandes, Shai] changed, thanks for the notice
> 
> > diff --git a/drivers/net/ena/ena_ethdev.c
> > b/drivers/net/ena/ena_ethdev.c index 7345e480f8..b764442dbb 100644
> > --- a/drivers/net/ena/ena_ethdev.c
> > +++ b/drivers/net/ena/ena_ethdev.c
> > @@ -1171,7 +1171,6 @@ static int ena_start(struct rte_eth_dev *dev)
> >       struct ena_adapter *adapter = dev->data->dev_private;
> >       uint64_t ticks;
> >       int rc = 0;
> > -     uint16_t i;
> >
> >       /* Cannot allocate memory in secondary process */
> >       if (rte_eal_process_type() != RTE_PROC_PRIMARY) { @@ -1209,11
> > +1208,6 @@ static int ena_start(struct rte_eth_dev *dev)
> >       ++adapter->dev_stats.dev_start;
> >       adapter->state = ENA_ADAPTER_STATE_RUNNING;
> >
> > -     for (i = 0; i < dev->data->nb_rx_queues; i++)
> > -             dev->data->rx_queue_state[i] =
> RTE_ETH_QUEUE_STATE_STARTED;
> > -     for (i = 0; i < dev->data->nb_tx_queues; i++)
> > -             dev->data->tx_queue_state[i] =
> RTE_ETH_QUEUE_STATE_STARTED;
> > -
> >       return 0;
> >
> >  err_rss_init:
> > @@ -1229,7 +1223,6 @@ static int ena_stop(struct rte_eth_dev *dev)
> >       struct ena_com_dev *ena_dev = &adapter->ena_dev;
> >       struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> >       struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
> > -     uint16_t i;
> >       int rc;
> >
> >       /* Cannot free memory in secondary process */ @@ -1261,11
> > +1254,6 @@ static int ena_stop(struct rte_eth_dev *dev)
> >       adapter->state = ENA_ADAPTER_STATE_STOPPED;
> >       dev->data->dev_started = 0;
> >
> > -     for (i = 0; i < dev->data->nb_rx_queues; i++)
> > -             dev->data->rx_queue_state[i] =
> RTE_ETH_QUEUE_STATE_STOPPED;
> > -     for (i = 0; i < dev->data->nb_tx_queues; i++)
> > -             dev->data->tx_queue_state[i] =
> RTE_ETH_QUEUE_STATE_STOPPED;
> > -
> >       return 0;
> >  }
> >
> 
> Above queue state related changes are coming from other patch in this
> release, I guess removing them is a mistake, can you please double check?
[Brandes, Shai] good catch, indeed was removed accidently