On Mon, Jan 08, 2018 at 05:05:47PM +0000, Bruce Richardson wrote:

On Sat, Dec 30, 2017 at 10:07:54PM +0530, Pavan Nikhilesh wrote:

quoted

Added support for detecting march and mcpu by reading midr_el1 register.
The implementer, primary part number values read can be used to figure
out the underlying arm cpu.

Signed-off-by: Pavan Nikhilesh <redacted>
---

 The current method used for reading MIDR_EL1 form userspace might not be
 reliable and can be easily modified by updating config/arm/machine.py.

 More info on midr_el1 can be found at
 http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500g/BABFEABI.html

 This patch depends on http://dpdk.org/dev/patchwork/patch/32410/

I had intended that patch to just be a prototype to start adding ARM
support - have you considered taking that and rolling in these changes
into that as a part of a set?

Agreed, I will merge the patches as a patchset in v2.

quoted

 config/arm/machine.py  | 18 ++++++++++++++++++
 config/arm/meson.build | 20 ++++++++++++++++++++
 config/meson.build     |  3 ++-
 drivers/meson.build    |  2 +-
 examples/meson.build   |  2 +-
 lib/meson.build        |  2 +-
 meson.build            |  2 +-
 7 files changed, 44 insertions(+), 5 deletions(-)
 create mode 100755 config/arm/machine.py

diff --git a/config/arm/machine.py b/config/arm/machine.py
new file mode 100755
index 000000000..3c6e7b6a7
--- /dev/null
+++ b/config/arm/machine.py

@@ -0,0 +1,18 @@
+#!/usr/bin/python
+import pprint
+pp = pprint
+
+ident = []
+fname = '/sys/devices/system/cpu/cpu0/regs/identification/midr_el1'
+with open(fname) as f:
+    content = f.read()
+
+midr_el1 = (int(content.rstrip('\n'), 16))
+
+ident.append(hex((midr_el1 >> 24) & 0xFF))  # Implementer
+ident.append(hex((midr_el1 >> 20) & 0xF))   # Variant
+ident.append(hex((midr_el1 >> 16) & 0XF))   # Architecture
+ident.append(hex((midr_el1 >> 4) & 0xFFF))  # Primary Part number
+ident.append(hex(midr_el1 & 0xF))           # Revision
+
+print(' '.join(ident))

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 250958415..f6ae69c21 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build

@@ -41,3 +41,23 @@ else
 endif
 dpdk_conf.set('RTE_CACHE_LINE_SIZE', 128)
 dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
+
+detect_vendor = find_program(join_paths(meson.current_source_dir(),
+			'machine.py'))
+cmd = run_command(detect_vendor.path())
+if cmd.returncode() != 0
+	message('Unable to read midr_el1')
+else
+	cmd_output = cmd.stdout().strip().split(' ')
+	message('midr_el1 output: \n' + 'Implementor ' + cmd_output[0] +
+			' Variant ' + cmd_output[1] + ' Architecture ' +
+			cmd_output[2] + ' Primary Part number ' + cmd_output[3]
+			+ ' Revision ' + cmd_output[4])
+	if cmd_output[0] == '0x43'
+		message('Implementor : Cavium')
+		dpdk_conf.set('RTE_MACHINE', 'thunderx')
+		machine_arg = []
+	        machine_arg += '-march=' + 'armv8-a+crc+crypto'
+		machine_arg += '-mcpu=' + 'thunderx'
+	endif
+endif

Should the call to the script and return code not be dependent on the
current value of machine i.e. only if it's "native"? If the user has
specified a "machine" type as part of the meson configuration
parameters, you should not override it. Similarly in the cross-build
case, the machine type is taken from the cross-build file itself.

Yes, only in native build scenario the script and result should be used  I will
modify that in v2.

quoted

diff --git a/config/meson.build b/config/meson.build
index 86e978fb1..fe8104676 100644
--- a/config/meson.build
+++ b/config/meson.build

@@ -8,7 +8,8 @@ else
 	machine = get_option('machine')
 endif
 dpdk_conf.set('RTE_MACHINE', machine)
-machine_arg = '-march=' + machine
+machine_arg = []
+machine_arg += '-march=' + machine

I was confused initially as to why this change, but now I realise it's
due to the fact that for the thunderx build you need both an -march and
an -mcpu flag. I think it might be better to make this change as a
separate patch and rename the variable from "machine_arg" to
"machine_args" to make it clearer it's an array.

I was thinking of having a dpdk_machine_args in base meson.build parallel to
dpdk_extra_ldflags and friends.

Alternatively, we can have separate variables for march flag and mcpu
flag. [Does cpu-type need to be a configuration parameter for ARM
platforms, in the non-cross-build case?]

Initially while experimenting with meson I did try out splitting the march and
mcpu flags but that resulted in extra overhead i.e. filtering out mcpu when
it's unused in case of x86 builds.

In case of x86 the mcpu flag is deprecated where as in arm it provides extra
hint to the compiler to do silicon specific optimizations as the implementation
of core is vendor specific. In case of gcc7[1] the mcpu can be furthur tuned to
be core specific by basing it around the primary part number
(thunderxt81(0xA2), thunderxt83(0xA3), thunderxt88(0xA1)) detected via reading
midr_el1.

[1] https://gcc.gnu.org/gcc-7/changes.html

/Bruce

Thanks,
Pavan.