diff mbox series

build: add missing arch define for Arm

Message ID 20211217085430.820773-1-ruifeng.wang@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series build: add missing arch define for Arm |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Ruifeng Wang Dec. 17, 2021, 8:54 a.m. UTC 
As per design document, RTE_ARCH is the name of the architecture.
However, the definition was missing on Arm with meson build.
It impacts applications that refers to this string.

Added for Arm builds.

Fixes: b1d48c41189a ("build: support ARM with meson")
Cc: stable@dpdk.org

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 config/arm/meson.build | 3 +++
 1 file changed, 3 insertions(+)

Comments

Thomas Monjalon Jan. 13, 2022, 5:32 p.m. UTC | #1 
17/12/2021 09:54, Ruifeng Wang:
> As per design document, RTE_ARCH is the name of the architecture.
> However, the definition was missing on Arm with meson build.
> It impacts applications that refers to this string.
> 
> Added for Arm builds.
> 
> Fixes: b1d48c41189a ("build: support ARM with meson")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>                  ['RTE_ARCH_ARMv8_AARCH32', true],
> +                ['RTE_ARCH', 'arm64_aarch32'],

Why not armv8_aarch32?

[...]
>          dpdk_conf.set('RTE_ARCH_ARMv7', true)
> +        dpdk_conf.set('RTE_ARCH', 'armv7')
[...]
>      # armv8 build
> +    dpdk_conf.set('RTE_ARCH', 'arm64')

Why not armv8?

What I prefer the most in silicon industry is the naming craziness :)
Ruifeng Wang Jan. 14, 2022, 9:05 a.m. UTC | #2 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 14, 2022 1:33 AM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Cc: dev@dpdk.org; stable@dpdk.org; viktorin@rehivetech.com;
> bruce.richardson@intel.com; stephen@networkplumber.org;
> juraj.linkes@pantheon.tech; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH] build: add missing arch define for Arm
> 
> 17/12/2021 09:54, Ruifeng Wang:
> > As per design document, RTE_ARCH is the name of the architecture.
> > However, the definition was missing on Arm with meson build.
> > It impacts applications that refers to this string.
> >
> > Added for Arm builds.
> >
> > Fixes: b1d48c41189a ("build: support ARM with meson")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >                  ['RTE_ARCH_ARMv8_AARCH32', true],
> > +                ['RTE_ARCH', 'arm64_aarch32'],
> 
> Why not armv8_aarch32?

Thanks for the comments.
Agreed. armv8_aarch32 is consistent with the RTE_ARCH_xx macro above.

> 
> [...]
> >          dpdk_conf.set('RTE_ARCH_ARMv7', true)
> > +        dpdk_conf.set('RTE_ARCH', 'armv7')
> [...]
> >      # armv8 build
> > +    dpdk_conf.set('RTE_ARCH', 'arm64')
> 
> Why not armv8?
> 
> What I prefer the most in silicon industry is the naming craziness :)

While armv8 usually refers to one generation of the Arm architecture, arm64 is more generic for 64-bit architectures.
And what defined for armv8 build is RTE_ARCH_ARM64. So for consistency, arm64 is better?
Thomas Monjalon Jan. 14, 2022, 12:50 p.m. UTC | #3 
14/01/2022 10:05, Ruifeng Wang:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 17/12/2021 09:54, Ruifeng Wang:
> > > As per design document, RTE_ARCH is the name of the architecture.
> > > However, the definition was missing on Arm with meson build.
> > > It impacts applications that refers to this string.
> > >
> > > Added for Arm builds.
> > >
> > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >                  ['RTE_ARCH_ARMv8_AARCH32', true],
> > > +                ['RTE_ARCH', 'arm64_aarch32'],
> > 
> > Why not armv8_aarch32?
> 
> Thanks for the comments.
> Agreed. armv8_aarch32 is consistent with the RTE_ARCH_xx macro above.
> 
> > 
> > [...]
> > >          dpdk_conf.set('RTE_ARCH_ARMv7', true)
> > > +        dpdk_conf.set('RTE_ARCH', 'armv7')
> > [...]
> > >      # armv8 build
> > > +    dpdk_conf.set('RTE_ARCH', 'arm64')
> > 
> > Why not armv8?
> > 
> > What I prefer the most in silicon industry is the naming craziness :)
> 
> While armv8 usually refers to one generation of the Arm architecture, arm64 is more generic for 64-bit architectures.
> And what defined for armv8 build is RTE_ARCH_ARM64. So for consistency, arm64 is better?

I don't really care as long as we can have fun of this naming :)
Juraj Linkeš Jan. 17, 2022, 1:12 p.m. UTC | #4 
> -----Original Message-----
> From: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Sent: Friday, January 14, 2022 10:05 AM
> To: thomas@monjalon.net
> Cc: dev@dpdk.org; stable@dpdk.org; viktorin@rehivetech.com;
> bruce.richardson@intel.com; stephen@networkplumber.org; Juraj Linkeš
> <juraj.linkes@pantheon.tech>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH] build: add missing arch define for Arm
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Friday, January 14, 2022 1:33 AM
> > To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> > Cc: dev@dpdk.org; stable@dpdk.org; viktorin@rehivetech.com;
> > bruce.richardson@intel.com; stephen@networkplumber.org;
> > juraj.linkes@pantheon.tech; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> > Subject: Re: [PATCH] build: add missing arch define for Arm
> >
> > 17/12/2021 09:54, Ruifeng Wang:
> > > As per design document, RTE_ARCH is the name of the architecture.
> > > However, the definition was missing on Arm with meson build.
> > > It impacts applications that refers to this string.
> > >
> > > Added for Arm builds.
> > >
> > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >                  ['RTE_ARCH_ARMv8_AARCH32', true],
> > > +                ['RTE_ARCH', 'arm64_aarch32'],
> >
> > Why not armv8_aarch32?
> 
> Thanks for the comments.
> Agreed. armv8_aarch32 is consistent with the RTE_ARCH_xx macro above.
> 
> >
> > [...]
> > >          dpdk_conf.set('RTE_ARCH_ARMv7', true)
> > > +        dpdk_conf.set('RTE_ARCH', 'armv7')
> > [...]
> > >      # armv8 build
> > > +    dpdk_conf.set('RTE_ARCH', 'arm64')
> >
> > Why not armv8?
> >
> > What I prefer the most in silicon industry is the naming craziness :)
> 
> While armv8 usually refers to one generation of the Arm architecture, arm64 is
> more generic for 64-bit architectures.
> And what defined for armv8 build is RTE_ARCH_ARM64. So for consistency,
> arm64 is better?
> 

Using armv8_aarch32 along with arm64 doesn't seem right. We should unite these and I think armv8 makes sense. As you mentioned arvm8 is an arm64 architecture and using the more precise identification is better in my opinion (as that gives more information). As for the consistency with RTE_ARCH_ARM64, I think the problem is that we don't have the RTE_ARCH_ARMv8 flag (which would provide the consistency, but won't be used):
The current code is, accurately, written for 64bit arm architectures (all of them).
There is currently no need to differentiate between 64bit arm architectures which is why RTE_ARCH_ARMv8 doesn't exist.
However, armv8 exists and we know how to identify it which is why I think setting RTE_ARCH to armv8 is the way to go.

So my thinking is RTE_ARCH should be set to armv8, which implies RTE_ARCH_ARMv8 which in turn implies RTE_ARCH_ARM64. We're just missing the middle part since there's no use for it now.

And to be fully consistent, we could add RTE_ARCH_ARM32 to armv7 (as a superset of RTE_ARCH_ARMv7, but that likely won't be of much use).
Morten Brørup Jan. 17, 2022, 1:50 p.m. UTC | #5 
> From: Juraj Linkeš [mailto:juraj.linkes@pantheon.tech]
> Sent: Monday, 17 January 2022 14.12
> 
> > From: Ruifeng Wang <Ruifeng.Wang@arm.com>
> > Sent: Friday, January 14, 2022 10:05 AM
> >
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Friday, January 14, 2022 1:33 AM
> > >
> > > 17/12/2021 09:54, Ruifeng Wang:
> > > > As per design document, RTE_ARCH is the name of the architecture.
> > > > However, the definition was missing on Arm with meson build.
> > > > It impacts applications that refers to this string.
> > > >
> > > > Added for Arm builds.
> > > >
> > > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > ---
> > > >                  ['RTE_ARCH_ARMv8_AARCH32', true],
> > > > +                ['RTE_ARCH', 'arm64_aarch32'],
> > >
> > > Why not armv8_aarch32?
> >
> > Thanks for the comments.
> > Agreed. armv8_aarch32 is consistent with the RTE_ARCH_xx macro above.
> >
> > >
> > > [...]
> > > >          dpdk_conf.set('RTE_ARCH_ARMv7', true)
> > > > +        dpdk_conf.set('RTE_ARCH', 'armv7')
> > > [...]
> > > >      # armv8 build
> > > > +    dpdk_conf.set('RTE_ARCH', 'arm64')
> > >
> > > Why not armv8?
> > >
> > > What I prefer the most in silicon industry is the naming craziness
> :)
> >
> > While armv8 usually refers to one generation of the Arm architecture,
> arm64 is
> > more generic for 64-bit architectures.
> > And what defined for armv8 build is RTE_ARCH_ARM64. So for
> consistency,
> > arm64 is better?
> >
> 
> Using armv8_aarch32 along with arm64 doesn't seem right. We should
> unite these and I think armv8 makes sense. As you mentioned arvm8 is an
> arm64 architecture and using the more precise identification is better
> in my opinion (as that gives more information). As for the consistency
> with RTE_ARCH_ARM64, I think the problem is that we don't have the
> RTE_ARCH_ARMv8 flag (which would provide the consistency, but won't be
> used):
> The current code is, accurately, written for 64bit arm architectures
> (all of them).
> There is currently no need to differentiate between 64bit arm
> architectures which is why RTE_ARCH_ARMv8 doesn't exist.
> However, armv8 exists and we know how to identify it which is why I
> think setting RTE_ARCH to armv8 is the way to go.
> 
> So my thinking is RTE_ARCH should be set to armv8, which implies
> RTE_ARCH_ARMv8 which in turn implies RTE_ARCH_ARM64. We're just missing
> the middle part since there's no use for it now.
> 
> And to be fully consistent, we could add RTE_ARCH_ARM32 to armv7 (as a
> superset of RTE_ARCH_ARMv7, but that likely won't be of much use).
> 

DPDK already has the cross-platform RTE_ARCH_64 and RTE_ARCH_32 to indicate 64/32 bit word size.

You don't need ARM-specific versions of these!
Ruifeng Wang Jan. 19, 2022, 9:44 a.m. UTC | #6 
> -----Original Message-----
> From: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Sent: Monday, January 17, 2022 9:12 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; stable@dpdk.org; viktorin@rehivetech.com;
> bruce.richardson@intel.com; stephen@networkplumber.org; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd
> <nd@arm.com>
> Subject: RE: [PATCH] build: add missing arch define for Arm
> 
> 
> 
> > -----Original Message-----
> > From: Ruifeng Wang <Ruifeng.Wang@arm.com>
> > Sent: Friday, January 14, 2022 10:05 AM
> > To: thomas@monjalon.net
> > Cc: dev@dpdk.org; stable@dpdk.org; viktorin@rehivetech.com;
> > bruce.richardson@intel.com; stephen@networkplumber.org; Juraj Linkeš
> > <juraj.linkes@pantheon.tech>; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> > Subject: RE: [PATCH] build: add missing arch define for Arm
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Friday, January 14, 2022 1:33 AM
> > > To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> > > Cc: dev@dpdk.org; stable@dpdk.org; viktorin@rehivetech.com;
> > > bruce.richardson@intel.com; stephen@networkplumber.org;
> > > juraj.linkes@pantheon.tech; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> > > Subject: Re: [PATCH] build: add missing arch define for Arm
> > >
> > > 17/12/2021 09:54, Ruifeng Wang:
> > > > As per design document, RTE_ARCH is the name of the architecture.
> > > > However, the definition was missing on Arm with meson build.
> > > > It impacts applications that refers to this string.
> > > >
> > > > Added for Arm builds.
> > > >
> > > > Fixes: b1d48c41189a ("build: support ARM with meson")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > ---
> > > >                  ['RTE_ARCH_ARMv8_AARCH32', true],
> > > > +                ['RTE_ARCH', 'arm64_aarch32'],
> > >
> > > Why not armv8_aarch32?
> >
> > Thanks for the comments.
> > Agreed. armv8_aarch32 is consistent with the RTE_ARCH_xx macro above.
> >
> > >
> > > [...]
> > > >          dpdk_conf.set('RTE_ARCH_ARMv7', true)
> > > > +        dpdk_conf.set('RTE_ARCH', 'armv7')
> > > [...]
> > > >      # armv8 build
> > > > +    dpdk_conf.set('RTE_ARCH', 'arm64')
> > >
> > > Why not armv8?
> > >
> > > What I prefer the most in silicon industry is the naming craziness
> > > :)
> >
> > While armv8 usually refers to one generation of the Arm architecture,
> > arm64 is more generic for 64-bit architectures.
> > And what defined for armv8 build is RTE_ARCH_ARM64. So for
> > consistency,
> > arm64 is better?
> >
> 
> Using armv8_aarch32 along with arm64 doesn't seem right. We should unite

Thanks for providing your thoughts.
I have no strong opinion on this. armv8 indeed is better aligned with armv8_aarch32.
I will change in next version.

> these and I think armv8 makes sense. As you mentioned arvm8 is an arm64
> architecture and using the more precise identification is better in my opinion
> (as that gives more information). As for the consistency with
> RTE_ARCH_ARM64, I think the problem is that we don't have the
> RTE_ARCH_ARMv8 flag (which would provide the consistency, but won't be
> used):
> The current code is, accurately, written for 64bit arm architectures (all of
> them).
> There is currently no need to differentiate between 64bit arm architectures
> which is why RTE_ARCH_ARMv8 doesn't exist.
> However, armv8 exists and we know how to identify it which is why I think
> setting RTE_ARCH to armv8 is the way to go.
> 
> So my thinking is RTE_ARCH should be set to armv8, which implies
> RTE_ARCH_ARMv8 which in turn implies RTE_ARCH_ARM64. We're just
> missing the middle part since there's no use for it now.
> 
> And to be fully consistent, we could add RTE_ARCH_ARM32 to armv7 (as a
> superset of RTE_ARCH_ARMv7, but that likely won't be of much use).
diff mbox series

Patch

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 213324d262..3e1d7836df 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -49,6 +49,7 @@  implementer_generic = {
                 ['RTE_ARCH_ARM_NEON_MEMCPY', false],
                 ['RTE_ARCH_STRICT_ALIGN', true],
                 ['RTE_ARCH_ARMv8_AARCH32', true],
+                ['RTE_ARCH', 'arm64_aarch32'],
                 ['RTE_CACHE_LINE_SIZE', 64]
             ]
         }
@@ -432,11 +433,13 @@  if dpdk_conf.get('RTE_ARCH_32')
     else
         # armv7 build
         dpdk_conf.set('RTE_ARCH_ARMv7', true)
+        dpdk_conf.set('RTE_ARCH', 'armv7')
         # the minimum architecture supported, armv7-a, needs the following,
         machine_args += '-mfpu=neon'
     endif
 else
     # armv8 build
+    dpdk_conf.set('RTE_ARCH', 'arm64')
     update_flags = true
     soc_config = {}
     if not meson.is_cross_build()