[v10,1/5] net/bnxt: add support for aarch32

  From: Ruifeng Wang <ruifeng.wang@arm.com>

Expand vector PMD support to aarch32.

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_rxq.h | 2 +-
 drivers/net/bnxt/bnxt_rxr.h | 2 +-
 drivers/net/bnxt/bnxt_txr.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
  

On Wed, Sep 16, 2020 at 4:21 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> From: Ruifeng Wang <ruifeng.wang@arm.com>
>
> Expand vector PMD support to aarch32.
>
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  drivers/net/bnxt/bnxt_rxq.h | 2 +-
>  drivers/net/bnxt/bnxt_rxr.h | 2 +-
>  drivers/net/bnxt/bnxt_txr.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_rxq.h b/drivers/net/bnxt/bnxt_rxq.h
> index d5ce3b6d5..1c4027711 100644
> --- a/drivers/net/bnxt/bnxt_rxq.h
> +++ b/drivers/net/bnxt/bnxt_rxq.h
> @@ -22,7 +22,7 @@ struct bnxt_rx_queue {
>         uint16_t                nb_rx_hold; /* num held free RX desc */
>         uint16_t                rx_free_thresh; /* max free RX desc to hold */
>         uint16_t                queue_id; /* RX queue index */
> -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64) || defined(RTE_ARCH_ARM)
>         uint16_t                rxrearm_nb; /* number of descs to reinit. */
>         uint16_t                rxrearm_start; /* next desc index to reinit. */
>  #endif
> diff --git a/drivers/net/bnxt/bnxt_rxr.h b/drivers/net/bnxt/bnxt_rxr.h
> index 2bf46cd91..e2fba1647 100644
> --- a/drivers/net/bnxt/bnxt_rxr.h
> +++ b/drivers/net/bnxt/bnxt_rxr.h
> @@ -221,7 +221,7 @@ int bnxt_init_one_rx_ring(struct bnxt_rx_queue *rxq);
>  int bnxt_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id);
>  int bnxt_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id);
>
> -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64) || defined(RTE_ARCH_ARM)
>  uint16_t bnxt_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
>                             uint16_t nb_pkts);
>  int bnxt_rxq_vec_setup(struct bnxt_rx_queue *rxq);
> diff --git a/drivers/net/bnxt/bnxt_txr.h b/drivers/net/bnxt/bnxt_txr.h
> index 7715c11b8..38e5ac9df 100644
> --- a/drivers/net/bnxt/bnxt_txr.h
> +++ b/drivers/net/bnxt/bnxt_txr.h
> @@ -59,7 +59,7 @@ uint16_t bnxt_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>                                uint16_t nb_pkts);
>  uint16_t bnxt_dummy_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>                               uint16_t nb_pkts);
> -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64) || defined(RTE_ARCH_ARM)
>  uint16_t bnxt_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
>                             uint16_t nb_pkts);
>  #endif
> --
> 2.20.1
>

Hi Juraj,

I might be missing something, but I don't
believe these changes are sufficient to
enable vector mode for ARM32. For one
thing, bnxt_receive_function() in bnxt_ethdev.c
would need to be changed to enable the
selection of the vector receive function.

Also, meson.build has this condition
for building bnxt_rxtx_vec_neon.c:

if arch_subdir == 'x86'
        sources += files('bnxt_rxtx_vec_sse.c')
elif arch_subdir == 'arm' and host_machine.cpu_family().startswith('aarch64')
        sources += files('bnxt_rxtx_vec_neon.c')
endif

Were you able to build with these changes
and confirm that the vector mode functions
are selected and used? (There is a log at INFO
level to indicate which function is selected,
as should "show rxq info ..." from the
testpmd CLI.

Thanks,

   Lance
  

> -----Original Message-----
> From: Lance Richardson <lance.richardson@broadcom.com>
> Sent: Thursday, November 5, 2020 2:30 AM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: thomas@monjalon.net; Bruce Richardson <bruce.richardson@intel.com>;
> aconole@redhat.com; maicolgabriel@hotmail.com; dev@dpdk.org; Ruifeng
> Wang <Ruifeng.Wang@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v10 1/5] net/bnxt: add support for aarch32
> 
> On Wed, Sep 16, 2020 at 4:21 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
> wrote:
> >
> > From: Ruifeng Wang <ruifeng.wang@arm.com>
> >
> > Expand vector PMD support to aarch32.
> >
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > ---
> >  drivers/net/bnxt/bnxt_rxq.h | 2 +-
> >  drivers/net/bnxt/bnxt_rxr.h | 2 +-
> >  drivers/net/bnxt/bnxt_txr.h | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_rxq.h b/drivers/net/bnxt/bnxt_rxq.h
> > index d5ce3b6d5..1c4027711 100644
> > --- a/drivers/net/bnxt/bnxt_rxq.h
> > +++ b/drivers/net/bnxt/bnxt_rxq.h
> > @@ -22,7 +22,7 @@ struct bnxt_rx_queue {
> >         uint16_t                nb_rx_hold; /* num held free RX desc */
> >         uint16_t                rx_free_thresh; /* max free RX desc to hold */
> >         uint16_t                queue_id; /* RX queue index */
> > -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> > +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64) ||
> defined(RTE_ARCH_ARM)
> >         uint16_t                rxrearm_nb; /* number of descs to reinit. */
> >         uint16_t                rxrearm_start; /* next desc index to reinit. */
> >  #endif
> > diff --git a/drivers/net/bnxt/bnxt_rxr.h b/drivers/net/bnxt/bnxt_rxr.h
> > index 2bf46cd91..e2fba1647 100644
> > --- a/drivers/net/bnxt/bnxt_rxr.h
> > +++ b/drivers/net/bnxt/bnxt_rxr.h
> > @@ -221,7 +221,7 @@ int bnxt_init_one_rx_ring(struct bnxt_rx_queue
> *rxq);
> >  int bnxt_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id);
> >  int bnxt_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id);
> >
> > -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> > +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64) ||
> defined(RTE_ARCH_ARM)
> >  uint16_t bnxt_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
> >                             uint16_t nb_pkts);
> >  int bnxt_rxq_vec_setup(struct bnxt_rx_queue *rxq);
> > diff --git a/drivers/net/bnxt/bnxt_txr.h b/drivers/net/bnxt/bnxt_txr.h
> > index 7715c11b8..38e5ac9df 100644
> > --- a/drivers/net/bnxt/bnxt_txr.h
> > +++ b/drivers/net/bnxt/bnxt_txr.h
> > @@ -59,7 +59,7 @@ uint16_t bnxt_xmit_pkts(void *tx_queue, struct
> rte_mbuf **tx_pkts,
> >                                uint16_t nb_pkts);
> >  uint16_t bnxt_dummy_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts,
> >                               uint16_t nb_pkts);
> > -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> > +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64) ||
> defined(RTE_ARCH_ARM)
> >  uint16_t bnxt_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> >                             uint16_t nb_pkts);
> >  #endif
> > --
> > 2.20.1
> >
> 
> Hi Juraj,
> 
> I might be missing something, but I don't
> believe these changes are sufficient to
> enable vector mode for ARM32. For one
> thing, bnxt_receive_function() in bnxt_ethdev.c
> would need to be changed to enable the
> selection of the vector receive function.

Hi Lance,

This patch set aimed to enable aarch32 compilation and run some basic unit tests.
So changes in this patch fixed some 'symbol not found' issues when building for aarch32.
However, it do need a respin because a patch that changed Arm flags has been merged. 

> 
> Also, meson.build has this condition
> for building bnxt_rxtx_vec_neon.c:
> 
> if arch_subdir == 'x86'
>         sources += files('bnxt_rxtx_vec_sse.c')
> elif arch_subdir == 'arm' and host_machine.cpu_family().startswith('aarch64')
>         sources += files('bnxt_rxtx_vec_neon.c')
> endif
> 
> Were you able to build with these changes

Yes. I was able to build with these changes.
As you can find in 3/5 of this patch set, aarch32 uses (cpu_family = 'aarch64').
So condition in meson.build is not a problem.

> and confirm that the vector mode functions
> are selected and used? (There is a log at INFO
> level to indicate which function is selected,
> as should "show rxq info ..." from the
> testpmd CLI.
> 
> Thanks,
Thanks for your review.
> 
>    Lance
  

> > Hi Juraj,
> >
> > I might be missing something, but I don't
> > believe these changes are sufficient to
> > enable vector mode for ARM32. For one
> > thing, bnxt_receive_function() in bnxt_ethdev.c
> > would need to be changed to enable the
> > selection of the vector receive function.
>
> Hi Lance,
>
> This patch set aimed to enable aarch32 compilation and run some basic unit tests.
> So changes in this patch fixed some 'symbol not found' issues when building for aarch32.
> However, it do need a respin because a patch that changed Arm flags has been merged.
>
> >
> > Also, meson.build has this condition
> > for building bnxt_rxtx_vec_neon.c:
> >
> > if arch_subdir == 'x86'
> >         sources += files('bnxt_rxtx_vec_sse.c')
> > elif arch_subdir == 'arm' and host_machine.cpu_family().startswith('aarch64')
> >         sources += files('bnxt_rxtx_vec_neon.c')
> > endif
> >
> > Were you able to build with these changes
>
> Yes. I was able to build with these changes.
> As you can find in 3/5 of this patch set, aarch32 uses (cpu_family = 'aarch64').
> So condition in meson.build is not a problem.
>
> > and confirm that the vector mode functions
> > are selected and used? (There is a log at INFO
> > level to indicate which function is selected,
> > as should "show rxq info ..." from the
> > testpmd CLI.
> >
> > Thanks,
> Thanks for your review.
> >
> >    Lance

Hi Juraj,

Have you tried building with this patch set lately? Changes
have been made to bnxt_rxtx_vec_neon.c for 20.11 that
require 64-bit (for example, the intrinsic vzip1q_u32() is
used, this intrinsic is only available for 64-bit targets), as
I was able to confirm by applying your patch set and
attempting to build.

I think the only change that is actually needed is this
in drivers/net/bnxt/meson.build:

-elif arch_subdir == 'arm' and host_machine.cpu_family().startswith('aarch64')
+elif arch_subdir == 'arm' and
host_machine.cpu_family().startswith('aarch64') and
dpdk_conf.get('RTE_ARCH_64')

BTW, there are compilation failures in sfc_efx due to lack
of support for __int128 for Arm 32-bit targets.

Thanks,
    Lance
  

> -----Original Message-----
> From: Lance Richardson <lance.richardson@broadcom.com>
> Sent: Thursday, November 5, 2020 9:42 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Cc: Juraj Linkeš <juraj.linkes@pantheon.tech>; thomas@monjalon.net;
> Bruce Richardson <bruce.richardson@intel.com>; aconole@redhat.com;
> maicolgabriel@hotmail.com; dev@dpdk.org; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v10 1/5] net/bnxt: add support for aarch32
> 
> > > Hi Juraj,
> > >
> > > I might be missing something, but I don't
> > > believe these changes are sufficient to
> > > enable vector mode for ARM32. For one
> > > thing, bnxt_receive_function() in bnxt_ethdev.c
> > > would need to be changed to enable the
> > > selection of the vector receive function.
> >
> > Hi Lance,
> >
> > This patch set aimed to enable aarch32 compilation and run some basic unit
> tests.
> > So changes in this patch fixed some 'symbol not found' issues when
> building for aarch32.
> > However, it do need a respin because a patch that changed Arm flags has
> been merged.
> >
> > >
> > > Also, meson.build has this condition
> > > for building bnxt_rxtx_vec_neon.c:
> > >
> > > if arch_subdir == 'x86'
> > >         sources += files('bnxt_rxtx_vec_sse.c')
> > > elif arch_subdir == 'arm' and
> host_machine.cpu_family().startswith('aarch64')
> > >         sources += files('bnxt_rxtx_vec_neon.c')
> > > endif
> > >
> > > Were you able to build with these changes
> >
> > Yes. I was able to build with these changes.
> > As you can find in 3/5 of this patch set, aarch32 uses (cpu_family =
> 'aarch64').
> > So condition in meson.build is not a problem.
> >
> > > and confirm that the vector mode functions
> > > are selected and used? (There is a log at INFO
> > > level to indicate which function is selected,
> > > as should "show rxq info ..." from the
> > > testpmd CLI.
> > >
> > > Thanks,
> > Thanks for your review.
> > >
> > >    Lance
> 
> Hi Juraj,
> 
> Have you tried building with this patch set lately? Changes
> have been made to bnxt_rxtx_vec_neon.c for 20.11 that
> require 64-bit (for example, the intrinsic vzip1q_u32() is
> used, this intrinsic is only available for 64-bit targets), as
> I was able to confirm by applying your patch set and
> attempting to build.

When the patch set was posted, it did compile. There was a Travis job added, and it passed.
But now the patch set is stale because it has been in Patchwork for over a month and
many other changes has been made during that time.
Definitely it needs rework.
Will respin the patch set and send out new version.

> 
> I think the only change that is actually needed is this
> in drivers/net/bnxt/meson.build:
> 
> -elif arch_subdir == 'arm' and
> host_machine.cpu_family().startswith('aarch64')
> +elif arch_subdir == 'arm' and
> host_machine.cpu_family().startswith('aarch64') and
> dpdk_conf.get('RTE_ARCH_64')

Will look into this.

> 
> BTW, there are compilation failures in sfc_efx due to lack
> of support for __int128 for Arm 32-bit targets.

Thanks for reminding.
Will look into this.

> 
> Thanks,
>     Lance