[PATCH 0/2] Introduce x86 specific identification API

[PATCH 0/2] Introduce x86 specific identification API

[David Marchand]

Rather than have every driver implement their own set of cpuid() ugly
stuff, provide an abstracted (gcc/clang vs MSVC) API for querying about
x86 processor details.

Note:
- coming up with a cross arch API seems a difficult task, hence a arch
  specific API has been preferred. If other arches have ideas how to
  abstract I am open to suggestions though I won't have the time to
  implement it for this release,
- usage of this arch specific API must not become the common practice
  and maintainers will have to be wary that drivers are still working
  fine with generic/common processor/arch features,


-- 
David Marchand

David Marchand (2):
  eal: introduce x86 processor identification
  common/mlx5: use EAL x86 processor identification

 MAINTAINERS                       |   1 +
 app/test/meson.build              |   1 +
 app/test/test_cpu.c               |  37 ++++++++
 drivers/common/mlx5/mlx5_common.c |  81 +++++------------
 lib/eal/common/eal_common_cpu.c   | 141 ++++++++++++++++++++++++++++++
 lib/eal/common/eal_cpu.h          |  77 ++++++++++++++++
 lib/eal/common/meson.build        |   1 +
 lib/eal/version.map               |   6 ++
 8 files changed, 285 insertions(+), 60 deletions(-)
 create mode 100644 app/test/test_cpu.c
 create mode 100644 lib/eal/common/eal_common_cpu.c
 create mode 100644 lib/eal/common/eal_cpu.h

-- 
2.41.0

[PATCH 1/2] eal: introduce x86 processor identification

[David Marchand]

In some really specific cases, it may be needed to get a detailed
information on the processor running a DPDK application for drivers to
achieve better performance, or for matters that concern only them.

Those information are highly arch-specific and require a specific API.

Introduce a set of functions to get brand, family and model of a x86
processor.
Those functions do not make sense on other arches and a
driver must first check rte_cpu_is_x86() before anything else.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 MAINTAINERS                     |   1 +
 app/test/meson.build            |   1 +
 app/test/test_cpu.c             |  37 +++++++++
 lib/eal/common/eal_common_cpu.c | 141 ++++++++++++++++++++++++++++++++
 lib/eal/common/eal_cpu.h        |  77 +++++++++++++++++
 lib/eal/common/meson.build      |   1 +
 lib/eal/version.map             |   6 ++
 7 files changed, 264 insertions(+)
 create mode 100644 app/test/test_cpu.c
 create mode 100644 lib/eal/common/eal_common_cpu.c
 create mode 100644 lib/eal/common/eal_cpu.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 698608cdb2..b87d47a1e4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -158,6 +158,7 @@ F: app/test/test_barrier.c
 F: app/test/test_bitcount.c
 F: app/test/test_byteorder.c
 F: app/test/test_common.c
+F: app/test/test_cpu.c
 F: app/test/test_cpuflags.c
 F: app/test/test_cycles.c
 F: app/test/test_debug.c
diff --git a/app/test/meson.build b/app/test/meson.build
index 05bae9216d..4b37ad02fa 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -44,6 +44,7 @@ source_file_deps = {
     'test_cmdline_string.c': [],
     'test_common.c': [],
     'test_compressdev.c': ['compressdev'],
+    'test_cpu.c': [],
     'test_cpuflags.c': [],
     'test_crc.c': ['net'],
     'test_cryptodev.c': test_cryptodev_deps,
diff --git a/app/test/test_cpu.c b/app/test/test_cpu.c
new file mode 100644
index 0000000000..40d8bd94eb
--- /dev/null
+++ b/app/test/test_cpu.c
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Red Hat, Inc.
+ */
+
+#include <stdio.h>
+#include <inttypes.h>
+
+#include "eal_cpu.h"
+
+#include "test.h"
+
+static int
+test_cpu(void)
+{
+#ifndef RTE_ARCH_X86
+	RTE_TEST_ASSERT(!rte_cpu_is_x86(), "rte_cpu_is_x86() returned true on " RTE_STR(RTE_ARCH));
+#else
+	const char *vendor;
+
+	RTE_TEST_ASSERT(rte_cpu_is_x86(), "rte_cpu_is_x86() returned false");
+
+	if (rte_cpu_x86_is_amd())
+		vendor = "AMD";
+	else if (rte_cpu_x86_is_intel())
+		vendor = "Intel";
+	else
+		vendor = "unknown";
+
+	printf("The processor running this program is a x86 %s processor, brand=0x%"
+		PRIx8", family=0x%"PRIx8", model=0x%"PRIx8"\n", vendor, rte_cpu_x86_brand(),
+		rte_cpu_x86_family(), rte_cpu_x86_model());
+#endif
+
+	return TEST_SUCCESS;
+}
+
+REGISTER_FAST_TEST(cpu_autotest, true, true, test_cpu);
diff --git a/lib/eal/common/eal_common_cpu.c b/lib/eal/common/eal_common_cpu.c
new file mode 100644
index 0000000000..18cdb27f75
--- /dev/null
+++ b/lib/eal/common/eal_common_cpu.c
@@ -0,0 +1,141 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Red Hat, Inc.
+ */
+
+#include <rte_debug.h>
+
+#include "eal_cpu.h"
+
+#ifdef RTE_ARCH_X86
+#ifndef RTE_TOOLCHAIN_MSVC
+#include <cpuid.h>
+#endif
+
+static void
+x86_cpuid(uint32_t leaf, uint32_t subleaf, uint32_t *eax, uint32_t *ebx,
+	uint32_t *ecx, uint32_t *edx)
+{
+	uint32_t regs[4] = { 0 };
+
+#ifdef RTE_TOOLCHAIN_MSVC
+	__cpuidex(regs, leaf, subleaf);
+#else
+	__cpuid_count(leaf, subleaf, regs[0], regs[1], regs[2], regs[3]);
+#endif
+
+	*eax = regs[0];
+	*ebx = regs[1];
+	*ecx = regs[2];
+	*edx = regs[3];
+}
+#endif /* RTE_ARCH_X86 */
+
+bool
+rte_cpu_is_x86(void)
+{
+#ifndef RTE_ARCH_X86
+	return false;
+#else
+	return true;
+#endif
+}
+
+bool
+rte_cpu_x86_is_amd(void)
+{
+#ifndef RTE_ARCH_X86
+	rte_panic("Calling %s does not make sense on %s architecture.\n",
+		__func__, RTE_STR(RTE_ARCH));
+#else
+	uint32_t eax, ebx, ecx, edx;
+
+	x86_cpuid(0x0, 0x0, &eax, &ebx, &ecx, &edx);
+	/* ascii_to_little_endian("Auth enti cAMD") */
+	return ebx == 0x68747541 && ecx == 0x444d4163 && edx == 0x69746e65;
+#endif
+}
+
+bool
+rte_cpu_x86_is_intel(void)
+{
+#ifndef RTE_ARCH_X86
+	rte_panic("Calling %s does not make sense on %s architecture.\n",
+		__func__, RTE_STR(RTE_ARCH));
+#else
+	uint32_t eax, ebx, ecx, edx;
+
+	x86_cpuid(0x0, 0x0, &eax, &ebx, &ecx, &edx);
+	/* ascii_to_little_endian("Genu ineI ntel") */
+	return ebx == 0x756e6547 && ecx == 0x6c65746e && edx == 0x49656e69;
+#endif
+}
+
+uint8_t
+rte_cpu_x86_brand(void)
+{
+#ifndef RTE_ARCH_X86
+	rte_panic("Calling %s does not make sense on %s architecture.\n",
+		__func__, RTE_STR(RTE_ARCH));
+#else
+	uint32_t eax, ebx, ecx, edx;
+	uint8_t brand = 0;
+
+	x86_cpuid(0x0, 0x0, &eax, &ebx, &ecx, &edx);
+	if (eax >= 1) {
+		x86_cpuid(0x1, 0x0, &eax, &ebx, &ecx, &edx);
+		brand = ebx & 0xff;
+	}
+
+	return brand;
+#endif
+}
+
+uint8_t
+rte_cpu_x86_family(void)
+{
+#ifndef RTE_ARCH_X86
+	rte_panic("Calling %s does not make sense on %s architecture.\n",
+		__func__, RTE_STR(RTE_ARCH));
+#else
+	uint32_t eax, ebx, ecx, edx;
+	uint8_t family = 0;
+
+	x86_cpuid(0x0, 0x0, &eax, &ebx, &ecx, &edx);
+	if (eax >= 1) {
+		uint8_t family_id;
+
+		x86_cpuid(0x1, 0x0, &eax, &ebx, &ecx, &edx);
+		family_id = (eax >> 8) & 0x0f;
+		family = family_id;
+		if (family_id == 0xf)
+			family += (eax >> 20) & 0xff;
+	}
+
+	return family;
+#endif
+}
+
+uint8_t
+rte_cpu_x86_model(void)
+{
+#ifndef RTE_ARCH_X86
+	rte_panic("Calling %s does not make sense on %s architecture.\n",
+		__func__, RTE_STR(RTE_ARCH));
+#else
+	uint32_t eax, ebx, ecx, edx;
+	uint8_t model = 0;
+
+	x86_cpuid(0x0, 0x0, &eax, &ebx, &ecx, &edx);
+	if (eax >= 1) {
+		uint8_t family_id;
+
+		x86_cpuid(0x1, 0x0, &eax, &ebx, &ecx, &edx);
+		family_id = (eax >> 8) & 0x0f;
+		model = (eax >> 4) & 0x0f;
+		if (family_id == 0x6 || family_id == 0xf)
+			model += (eax >> 12) & 0xf0;
+	}
+
+	return model;
+#endif
+}
diff --git a/lib/eal/common/eal_cpu.h b/lib/eal/common/eal_cpu.h
new file mode 100644
index 0000000000..26d8e06bf0
--- /dev/null
+++ b/lib/eal/common/eal_cpu.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Red Hat, Inc.
+ */
+
+#ifndef EAL_CPU_H
+#define EAL_CPU_H
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include <rte_compat.h>
+
+/**
+ * Returns whether the processor running this program is a x86 one.
+ *
+ * @return
+ *      true or false
+ */
+__rte_internal
+bool rte_cpu_is_x86(void);
+
+/**
+ * Returns whether the processor running this program is a AMD x86 one.
+ *
+ * Note: calling this function only makes sense if rte_cpu_is_x86() == true.
+ *
+ * @return
+ *      true or false
+ */
+__rte_internal
+bool rte_cpu_x86_is_amd(void);
+
+/**
+ * Returns whether the processor running this program is a Intel x86 one.
+ *
+ * Note: calling this function only makes sense if rte_cpu_is_x86() == true.
+ *
+ * @return
+ *      true or false
+ */
+__rte_internal
+bool rte_cpu_x86_is_intel(void);
+
+/**
+ * Returns the processor brand (as returned by CPUID).
+ *
+ * Note: calling this function only makes sense if rte_cpu_is_x86() == true.
+ *
+ * @return
+ *      x86 processor brand
+ */
+__rte_internal
+uint8_t rte_cpu_x86_brand(void);
+
+/**
+ * Returns the processor family (as returned by CPUID).
+ *
+ * Note: calling this function only makes sense if rte_cpu_is_x86() == true.
+ *
+ * @return
+ *      x86 processor family
+ */
+__rte_internal
+uint8_t rte_cpu_x86_family(void);
+
+/**
+ * Returns the processor model (as returned by CPUID).
+ *
+ * Note: calling this function only makes sense if rte_cpu_is_x86() == true.
+ *
+ * @return
+ *      x86 processor model
+ */
+__rte_internal
+uint8_t rte_cpu_x86_model(void);
+
+#endif /* EAL_CPU_H */
diff --git a/lib/eal/common/meson.build b/lib/eal/common/meson.build
index 22a626ba6f..bef5b2575b 100644
--- a/lib/eal/common/meson.build
+++ b/lib/eal/common/meson.build
@@ -9,6 +9,7 @@ sources += files(
         'eal_common_bus.c',
         'eal_common_class.c',
         'eal_common_config.c',
+        'eal_common_cpu.c',
         'eal_common_debug.c',
         'eal_common_dev.c',
         'eal_common_devargs.c',
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7940431e5a..62632202c5 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -424,6 +424,12 @@ INTERNAL {
 
 	rte_bus_register;
 	rte_bus_unregister;
+	rte_cpu_is_x86;
+	rte_cpu_x86_brand;
+	rte_cpu_x86_family;
+	rte_cpu_x86_is_amd;
+	rte_cpu_x86_is_intel;
+	rte_cpu_x86_model;
 	rte_eal_get_baseaddr;
 	rte_eal_parse_coremask;
 	rte_firmware_read;
-- 
2.41.0

Re: [PATCH 1/2] eal: introduce x86 processor identification

[Bruce Richardson]

On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> In some really specific cases, it may be needed to get a detailed
> information on the processor running a DPDK application for drivers to
> achieve better performance, or for matters that concern only them.
> 
> Those information are highly arch-specific and require a specific API.
> 
> Introduce a set of functions to get brand, family and model of a x86
> processor.
> Those functions do not make sense on other arches and a
> driver must first check rte_cpu_is_x86() before anything else.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  MAINTAINERS                     |   1 +
>  app/test/meson.build            |   1 +
>  app/test/test_cpu.c             |  37 +++++++++
>  lib/eal/common/eal_common_cpu.c | 141 ++++++++++++++++++++++++++++++++
>  lib/eal/common/eal_cpu.h        |  77 +++++++++++++++++
>  lib/eal/common/meson.build      |   1 +
>  lib/eal/version.map             |   6 ++
>  7 files changed, 264 insertions(+)
>  create mode 100644 app/test/test_cpu.c
>  create mode 100644 lib/eal/common/eal_common_cpu.c
>  create mode 100644 lib/eal/common/eal_cpu.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 698608cdb2..b87d47a1e4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -158,6 +158,7 @@ F: app/test/test_barrier.c
>  F: app/test/test_bitcount.c
>  F: app/test/test_byteorder.c
>  F: app/test/test_common.c
> +F: app/test/test_cpu.c
>  F: app/test/test_cpuflags.c
>  F: app/test/test_cycles.c
>  F: app/test/test_debug.c
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 05bae9216d..4b37ad02fa 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -44,6 +44,7 @@ source_file_deps = {
>      'test_cmdline_string.c': [],
>      'test_common.c': [],
>      'test_compressdev.c': ['compressdev'],
> +    'test_cpu.c': [],
>      'test_cpuflags.c': [],
>      'test_crc.c': ['net'],
>      'test_cryptodev.c': test_cryptodev_deps,
> diff --git a/app/test/test_cpu.c b/app/test/test_cpu.c
> new file mode 100644
> index 0000000000..40d8bd94eb
> --- /dev/null
> +++ b/app/test/test_cpu.c
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Red Hat, Inc.
> + */
> +
> +#include <stdio.h>
> +#include <inttypes.h>
> +
> +#include "eal_cpu.h"
> +
> +#include "test.h"
> +
> +static int
> +test_cpu(void)
> +{
> +#ifndef RTE_ARCH_X86
> +	RTE_TEST_ASSERT(!rte_cpu_is_x86(), "rte_cpu_is_x86() returned true on " RTE_STR(RTE_ARCH));
> +#else
> +	const char *vendor;
> +
> +	RTE_TEST_ASSERT(rte_cpu_is_x86(), "rte_cpu_is_x86() returned false");
> +
> +	if (rte_cpu_x86_is_amd())
> +		vendor = "AMD";
> +	else if (rte_cpu_x86_is_intel())
> +		vendor = "Intel";
> +	else
> +		vendor = "unknown";
> +
> +	printf("The processor running this program is a x86 %s processor, brand=0x%"
> +		PRIx8", family=0x%"PRIx8", model=0x%"PRIx8"\n", vendor, rte_cpu_x86_brand(),
> +		rte_cpu_x86_family(), rte_cpu_x86_model());
> +#endif
> +
> +	return TEST_SUCCESS;
> +}
> +
> +REGISTER_FAST_TEST(cpu_autotest, true, true, test_cpu);
> diff --git a/lib/eal/common/eal_common_cpu.c b/lib/eal/common/eal_common_cpu.c
> new file mode 100644
> index 0000000000..18cdb27f75
> --- /dev/null
> +++ b/lib/eal/common/eal_common_cpu.c
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Red Hat, Inc.
> + */
> +
> +#include <rte_debug.h>
> +
> +#include "eal_cpu.h"
> +
> +#ifdef RTE_ARCH_X86
> +#ifndef RTE_TOOLCHAIN_MSVC
> +#include <cpuid.h>
> +#endif
> +
> +static void
> +x86_cpuid(uint32_t leaf, uint32_t subleaf, uint32_t *eax, uint32_t *ebx,
> +	uint32_t *ecx, uint32_t *edx)
> +{
> +	uint32_t regs[4] = { 0 };
> +
> +#ifdef RTE_TOOLCHAIN_MSVC
> +	__cpuidex(regs, leaf, subleaf);
> +#else
> +	__cpuid_count(leaf, subleaf, regs[0], regs[1], regs[2], regs[3]);
> +#endif
> +
> +	*eax = regs[0];
> +	*ebx = regs[1];
> +	*ecx = regs[2];
> +	*edx = regs[3];
> +}
> +#endif /* RTE_ARCH_X86 */

From a readability perspective, I think it would be better to expand the
scope of this ifdef and have two copies of each function in the file,
rather than a single copy of each with #ifdefs. WDYT?

/Bruce

Re: [PATCH 1/2] eal: introduce x86 processor identification

[David Marchand]

On Fri, Sep 22, 2023 at 11:47 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> > In some really specific cases, it may be needed to get a detailed
> > information on the processor running a DPDK application for drivers to
> > achieve better performance, or for matters that concern only them.
> >
> > Those information are highly arch-specific and require a specific API.
> >
> > Introduce a set of functions to get brand, family and model of a x86
> > processor.
> > Those functions do not make sense on other arches and a
> > driver must first check rte_cpu_is_x86() before anything else.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  MAINTAINERS                     |   1 +
> >  app/test/meson.build            |   1 +
> >  app/test/test_cpu.c             |  37 +++++++++
> >  lib/eal/common/eal_common_cpu.c | 141 ++++++++++++++++++++++++++++++++
> >  lib/eal/common/eal_cpu.h        |  77 +++++++++++++++++
> >  lib/eal/common/meson.build      |   1 +
> >  lib/eal/version.map             |   6 ++
> >  7 files changed, 264 insertions(+)
> >  create mode 100644 app/test/test_cpu.c
> >  create mode 100644 lib/eal/common/eal_common_cpu.c
> >  create mode 100644 lib/eal/common/eal_cpu.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 698608cdb2..b87d47a1e4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -158,6 +158,7 @@ F: app/test/test_barrier.c
> >  F: app/test/test_bitcount.c
> >  F: app/test/test_byteorder.c
> >  F: app/test/test_common.c
> > +F: app/test/test_cpu.c
> >  F: app/test/test_cpuflags.c
> >  F: app/test/test_cycles.c
> >  F: app/test/test_debug.c
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index 05bae9216d..4b37ad02fa 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -44,6 +44,7 @@ source_file_deps = {
> >      'test_cmdline_string.c': [],
> >      'test_common.c': [],
> >      'test_compressdev.c': ['compressdev'],
> > +    'test_cpu.c': [],
> >      'test_cpuflags.c': [],
> >      'test_crc.c': ['net'],
> >      'test_cryptodev.c': test_cryptodev_deps,
> > diff --git a/app/test/test_cpu.c b/app/test/test_cpu.c
> > new file mode 100644
> > index 0000000000..40d8bd94eb
> > --- /dev/null
> > +++ b/app/test/test_cpu.c
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2023 Red Hat, Inc.
> > + */
> > +
> > +#include <stdio.h>
> > +#include <inttypes.h>
> > +
> > +#include "eal_cpu.h"
> > +
> > +#include "test.h"
> > +
> > +static int
> > +test_cpu(void)
> > +{
> > +#ifndef RTE_ARCH_X86
> > +     RTE_TEST_ASSERT(!rte_cpu_is_x86(), "rte_cpu_is_x86() returned true on " RTE_STR(RTE_ARCH));
> > +#else
> > +     const char *vendor;
> > +
> > +     RTE_TEST_ASSERT(rte_cpu_is_x86(), "rte_cpu_is_x86() returned false");
> > +
> > +     if (rte_cpu_x86_is_amd())
> > +             vendor = "AMD";
> > +     else if (rte_cpu_x86_is_intel())
> > +             vendor = "Intel";
> > +     else
> > +             vendor = "unknown";
> > +
> > +     printf("The processor running this program is a x86 %s processor, brand=0x%"
> > +             PRIx8", family=0x%"PRIx8", model=0x%"PRIx8"\n", vendor, rte_cpu_x86_brand(),
> > +             rte_cpu_x86_family(), rte_cpu_x86_model());
> > +#endif
> > +
> > +     return TEST_SUCCESS;
> > +}
> > +
> > +REGISTER_FAST_TEST(cpu_autotest, true, true, test_cpu);
> > diff --git a/lib/eal/common/eal_common_cpu.c b/lib/eal/common/eal_common_cpu.c
> > new file mode 100644
> > index 0000000000..18cdb27f75
> > --- /dev/null
> > +++ b/lib/eal/common/eal_common_cpu.c
> > @@ -0,0 +1,141 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2023 Red Hat, Inc.
> > + */
> > +
> > +#include <rte_debug.h>
> > +
> > +#include "eal_cpu.h"
> > +
> > +#ifdef RTE_ARCH_X86
> > +#ifndef RTE_TOOLCHAIN_MSVC
> > +#include <cpuid.h>
> > +#endif
> > +
> > +static void
> > +x86_cpuid(uint32_t leaf, uint32_t subleaf, uint32_t *eax, uint32_t *ebx,
> > +     uint32_t *ecx, uint32_t *edx)
> > +{
> > +     uint32_t regs[4] = { 0 };
> > +
> > +#ifdef RTE_TOOLCHAIN_MSVC
> > +     __cpuidex(regs, leaf, subleaf);
> > +#else
> > +     __cpuid_count(leaf, subleaf, regs[0], regs[1], regs[2], regs[3]);
> > +#endif
> > +
> > +     *eax = regs[0];
> > +     *ebx = regs[1];
> > +     *ecx = regs[2];
> > +     *edx = regs[3];
> > +}
> > +#endif /* RTE_ARCH_X86 */
>
> From a readability perspective, I think it would be better to expand the
> scope of this ifdef and have two copies of each function in the file,
> rather than a single copy of each with #ifdefs. WDYT?

I can have a try later, but if you want to take over the series, I
don't mind :-).


-- 
David Marchand

Re: [PATCH 1/2] eal: introduce x86 processor identification

[Bruce Richardson]

On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> In some really specific cases, it may be needed to get a detailed
> information on the processor running a DPDK application for drivers to
> achieve better performance, or for matters that concern only them.
> 
> Those information are highly arch-specific and require a specific API.
> 
> Introduce a set of functions to get brand, family and model of a x86
> processor.
> Those functions do not make sense on other arches and a
> driver must first check rte_cpu_is_x86() before anything else.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Couple of thoughts, having had a few minutes to process this.

* Rather than rte_cpu_is_x86() API, we could go a general API called
  rte_cpu_arch() which returns either a string, or an enum value. Within
  that, rather than #ifdefs, the actual return value could just be a define
  placed by meson in the rte_build_config.h file. The list of families
  according to meson are [1] - we'd just need to merge the 32 and 64-bit
  variants into one in the meson file.

* Similarly rather than having is_intel or is_amd functions, we could
  generalize to a "manufacturer" API, which could be applicable for other
  architectures too.

/Bruce

[1] https://mesonbuild.com/Reference-tables.html#cpu-families

Re: [PATCH 1/2] eal: introduce x86 processor identification

[Bruce Richardson]

On Fri, Sep 22, 2023 at 11:38:38AM +0100, Bruce Richardson wrote:
> On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> > In some really specific cases, it may be needed to get a detailed
> > information on the processor running a DPDK application for drivers to
> > achieve better performance, or for matters that concern only them.
> > 
> > Those information are highly arch-specific and require a specific API.
> > 
> > Introduce a set of functions to get brand, family and model of a x86
> > processor.
> > Those functions do not make sense on other arches and a
> > driver must first check rte_cpu_is_x86() before anything else.
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> 
> Couple of thoughts, having had a few minutes to process this.
> 
> * Rather than rte_cpu_is_x86() API, we could go a general API called
>   rte_cpu_arch() which returns either a string, or an enum value. Within
>   that, rather than #ifdefs, the actual return value could just be a define
>   placed by meson in the rte_build_config.h file. The list of families
>   according to meson are [1] - we'd just need to merge the 32 and 64-bit
>   variants into one in the meson file.
> 
We already have the architecture family in meson computed as the
"arch_subdir". Exposing that to the C code and wrapping as EAL function
might look like below (adding to eal.h for convenience).

Thoughts?
/Bruce

diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
index 53c4a5519e..39f65d0e0c 100644
--- a/lib/eal/include/rte_eal.h
+++ b/lib/eal/include/rte_eal.h
@@ -517,6 +517,24 @@ __rte_internal
 int
 rte_eal_parse_coremask(const char *coremask, int *cores);
 
+enum rte_arch_family {
+       rte_cpu_arch_arm,
+       rte_cpu_arch_loongarch,
+       rte_cpu_arch_ppc,
+       rte_cpu_arch_riscv,
+       rte_cpu_arch_x86,
+};
+
+/**
+ * Return the architecture family of the current CPU
+ */
+static inline enum rte_arch_family
+rte_eal_get_arch_family(void)
+{
+       /* take value from build config set by meson */
+       return RTE_ARCH_FAMILY;
+}
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/meson.build b/meson.build
index 2e6e546d20..45e22daeb1 100644
--- a/meson.build
+++ b/meson.build
@@ -63,6 +63,7 @@ elif host_machine.cpu_family().startswith('ppc')
 elif host_machine.cpu_family().startswith('riscv')
     arch_subdir = 'riscv'
 endif
+dpdk_conf.set('RTE_ARCH_FAMILY', 'rte_cpu_arch_' + arch_subdir)
 
 # configure the build, and make sure configs here and in config folder are
 # able to be included in any file. We also store a global array of include dirs

Re: [PATCH 1/2] eal: introduce x86 processor identification

[David Marchand]

On Fri, Sep 22, 2023 at 12:40 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> > In some really specific cases, it may be needed to get a detailed
> > information on the processor running a DPDK application for drivers to
> > achieve better performance, or for matters that concern only them.
> >
> > Those information are highly arch-specific and require a specific API.
> >
> > Introduce a set of functions to get brand, family and model of a x86
> > processor.
> > Those functions do not make sense on other arches and a
> > driver must first check rte_cpu_is_x86() before anything else.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> Couple of thoughts, having had a few minutes to process this.
>
> * Rather than rte_cpu_is_x86() API, we could go a general API called
>   rte_cpu_arch() which returns either a string, or an enum value. Within
>   that, rather than #ifdefs, the actual return value could just be a define
>   placed by meson in the rte_build_config.h file. The list of families
>   according to meson are [1] - we'd just need to merge the 32 and 64-bit
>   variants into one in the meson file.

Your proposal (in next mail) lgtm.


>
> * Similarly rather than having is_intel or is_amd functions, we could
>   generalize to a "manufacturer" API, which could be applicable for other
>   architectures too.

Like a rte_cpu_x86_manufacturer() ? which returns an enum too I suppose.



-- 
David Marchand

Re: [PATCH 1/2] eal: introduce x86 processor identification

[Bruce Richardson]

On Mon, Sep 25, 2023 at 11:46:00AM +0200, David Marchand wrote:
> On Fri, Sep 22, 2023 at 12:40 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> > > In some really specific cases, it may be needed to get a detailed
> > > information on the processor running a DPDK application for drivers to
> > > achieve better performance, or for matters that concern only them.
> > >
> > > Those information are highly arch-specific and require a specific API.
> > >
> > > Introduce a set of functions to get brand, family and model of a x86
> > > processor.
> > > Those functions do not make sense on other arches and a
> > > driver must first check rte_cpu_is_x86() before anything else.
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> >
> > Couple of thoughts, having had a few minutes to process this.
> >
> > * Rather than rte_cpu_is_x86() API, we could go a general API called
> >   rte_cpu_arch() which returns either a string, or an enum value. Within
> >   that, rather than #ifdefs, the actual return value could just be a define
> >   placed by meson in the rte_build_config.h file. The list of families
> >   according to meson are [1] - we'd just need to merge the 32 and 64-bit
> >   variants into one in the meson file.
> 
> Your proposal (in next mail) lgtm.
> 
> 
> >
> > * Similarly rather than having is_intel or is_amd functions, we could
> >   generalize to a "manufacturer" API, which could be applicable for other
> >   architectures too.
> 
> Like a rte_cpu_x86_manufacturer() ? which returns an enum too I suppose.
> 
I was actually thinking a more general "rte_cpu_manufacturer()" which
returns string, and therefore could be implemented by all architectures.
Could default to NULL or string "unknown" if not implemented.

/Bruce

RE: [PATCH 1/2] eal: introduce x86 processor identification

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 25 September 2023 12.16
> 
> On Mon, Sep 25, 2023 at 11:46:00AM +0200, David Marchand wrote:
> > On Fri, Sep 22, 2023 at 12:40 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Fri, Sep 22, 2023 at 11:37:20AM +0200, David Marchand wrote:
> > > > In some really specific cases, it may be needed to get a detailed
> > > > information on the processor running a DPDK application for drivers to
> > > > achieve better performance, or for matters that concern only them.
> > > >
> > > > Those information are highly arch-specific and require a specific API.
> > > >
> > > > Introduce a set of functions to get brand, family and model of a x86
> > > > processor.
> > > > Those functions do not make sense on other arches and a
> > > > driver must first check rte_cpu_is_x86() before anything else.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > >
> > > Couple of thoughts, having had a few minutes to process this.
> > >
> > > * Rather than rte_cpu_is_x86() API, we could go a general API called
> > >   rte_cpu_arch() which returns either a string, or an enum value. Within
> > >   that, rather than #ifdefs, the actual return value could just be a
> define
> > >   placed by meson in the rte_build_config.h file. The list of families
> > >   according to meson are [1] - we'd just need to merge the 32 and 64-bit
> > >   variants into one in the meson file.
> >
> > Your proposal (in next mail) lgtm.
> >
> >
> > >
> > > * Similarly rather than having is_intel or is_amd functions, we could
> > >   generalize to a "manufacturer" API, which could be applicable for other
> > >   architectures too.
> >
> > Like a rte_cpu_x86_manufacturer() ? which returns an enum too I suppose.
> >
> I was actually thinking a more general "rte_cpu_manufacturer()" which
> returns string, and therefore could be implemented by all architectures.
> Could default to NULL or string "unknown" if not implemented.
> 
> /Bruce

Please stop to think about this!

Nobody cares who manufactured the CPU. E.g. is it Cavium or Marvell... some vendors might have multiple series of CPUs with different features, e.g. from companies they have bought up.

What you are looking for are some sort of "capabilities" APIs, so you can query what the CPU offers. In this context, the CPU is just a device, like any other device. E.g. the ethdev has a bunch of "capabilities" APIs, where you can query for specific features, maximum number of queues, descriptors, and a lot more.

[PATCH 2/2] common/mlx5: use EAL x86 processor identification

[David Marchand]

Rather than use an ugly asm thing, use newly introduced EAL x86 API.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/common/mlx5/mlx5_common.c | 81 ++++++++-----------------------
 1 file changed, 21 insertions(+), 60 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_common.c b/drivers/common/mlx5/mlx5_common.c
index 0ad14a48c7..99adcd960e 100644
--- a/drivers/common/mlx5/mlx5_common.c
+++ b/drivers/common/mlx5/mlx5_common.c
@@ -6,6 +6,7 @@
 #include <string.h>
 #include <stdio.h>
 
+#include <eal_cpu.h>
 #include <rte_errno.h>
 #include <rte_mempool.h>
 #include <rte_class.h>
@@ -52,29 +53,6 @@ uint8_t haswell_broadwell_cpu;
  */
 #define MLX5_SQ_DB_NC "sq_db_nc"
 
-/* In case this is an x86_64 intel processor to check if
- * we should use relaxed ordering.
- */
-#ifdef RTE_ARCH_X86_64
-/**
- * This function returns processor identification and feature information
- * into the registers.
- *
- * @param eax, ebx, ecx, edx
- *		Pointers to the registers that will hold cpu information.
- * @param level
- *		The main category of information returned.
- */
-static inline void mlx5_cpu_id(unsigned int level,
-				unsigned int *eax, unsigned int *ebx,
-				unsigned int *ecx, unsigned int *edx)
-{
-	__asm__("cpuid\n\t"
-		: "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
-		: "0" (level));
-}
-#endif
-
 RTE_LOG_REGISTER_DEFAULT(mlx5_common_logtype, NOTICE)
 
 /* Head of list of drivers. */
@@ -1246,46 +1224,29 @@ mlx5_common_init(void)
 RTE_INIT_PRIO(mlx5_is_haswell_broadwell_cpu, LOG)
 {
 #ifdef RTE_ARCH_X86_64
-	unsigned int broadwell_models[4] = {0x3d, 0x47, 0x4F, 0x56};
-	unsigned int haswell_models[4] = {0x3c, 0x3f, 0x45, 0x46};
-	unsigned int i, model, family, brand_id, vendor;
-	unsigned int signature_intel_ebx = 0x756e6547;
-	unsigned int extended_model;
-	unsigned int eax = 0;
-	unsigned int ebx = 0;
-	unsigned int ecx = 0;
-	unsigned int edx = 0;
-	int max_level;
-
-	mlx5_cpu_id(0, &eax, &ebx, &ecx, &edx);
-	vendor = ebx;
-	max_level = eax;
-	if (max_level < 1) {
-		haswell_broadwell_cpu = 0;
+	uint8_t broadwell_models[] = {0x3d, 0x47, 0x4f, 0x56};
+	uint8_t haswell_models[] = {0x3c, 0x3f, 0x45, 0x46};
+	unsigned int i;
+	uint8_t model;
+
+	if (!rte_cpu_is_x86() || !rte_cpu_x86_is_intel() || rte_cpu_x86_brand() != 0x0 ||
+			rte_cpu_x86_family() != 0x6)
+		goto out;
+
+	model = rte_cpu_x86_model();
+	for (i = 0; i < RTE_DIM(broadwell_models); i++) {
+		if (model != broadwell_models[i])
+			continue;
+		haswell_broadwell_cpu = 1;
 		return;
 	}
-	mlx5_cpu_id(1, &eax, &ebx, &ecx, &edx);
-	model = (eax >> 4) & 0x0f;
-	family = (eax >> 8) & 0x0f;
-	brand_id = ebx & 0xff;
-	extended_model = (eax >> 12) & 0xf0;
-	/* Check if the processor is Haswell or Broadwell */
-	if (vendor == signature_intel_ebx) {
-		if (family == 0x06)
-			model += extended_model;
-		if (brand_id == 0 && family == 0x6) {
-			for (i = 0; i < RTE_DIM(broadwell_models); i++)
-				if (model == broadwell_models[i]) {
-					haswell_broadwell_cpu = 1;
-					return;
-				}
-			for (i = 0; i < RTE_DIM(haswell_models); i++)
-				if (model == haswell_models[i]) {
-					haswell_broadwell_cpu = 1;
-					return;
-				}
-		}
+	for (i = 0; i < RTE_DIM(haswell_models); i++) {
+		if (model != haswell_models[i])
+			continue;
+		haswell_broadwell_cpu = 1;
+		return;
 	}
+out:
 #endif
 	haswell_broadwell_cpu = 0;
 }
-- 
2.41.0