[PATCH 1/2] config/arm: adds Arm Neoverse N3 SoC

[PATCH 1/2] config/arm: adds Arm Neoverse N3 SoC

[Wathsala Vithanage]

Add Arm Neoverse N3 part number to build configuration.

Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
Reviewed-by: Dhruv Tripathi <dhruv.tripathi@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@foss.arm.com>

---
 config/arm/meson.build | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/config/arm/meson.build b/config/arm/meson.build
index a45aa9e466..ec7588f055 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -116,6 +116,18 @@ part_number_config_arm = {
             ['RTE_MAX_LCORE', 144],
             ['RTE_MAX_NUMA_NODES', 2]
         ]
+    },
+    '0xd8e': {
+        'march': 'armv8.7-a',
+        'march_features': ['sve2'],
+        'fallback_march': 'armv8.5-a',
+        'flags': [
+            ['RTE_MACHINE', '"neoverse-n3"'],
+            ['RTE_ARM_FEATURE_ATOMICS', true],
+            ['RTE_ARM_FEATURE_WFXT', true],
+            ['RTE_MAX_LCORE', 192],
+            ['RTE_MAX_NUMA_NODES', 2]
+        ]
     }
 }
 implementer_arm = {
@@ -555,6 +567,13 @@ soc_n2 = {
     'numa': false
 }
 
+soc_n3 = {
+    'description': 'Arm Neoverse N3',
+    'implementer': '0x41',
+    'part_number': '0xd8e',
+    'numa': false
+}
+
 soc_odyssey = {
     'description': 'Marvell Odyssey',
     'implementer': '0x41',
@@ -680,6 +699,7 @@ socs = {
     'kunpeng930': soc_kunpeng930,
     'n1sdp': soc_n1sdp,
     'n2': soc_n2,
+    'n3': soc_n3,
     'odyssey' : soc_odyssey,
     'stingray': soc_stingray,
     'thunderx2': soc_thunderx2,
@@ -833,7 +853,7 @@ if update_flags
         if part_number_config.get('force_march', false)
             candidate_march = part_number_config['march']
         else
-            supported_marchs = ['armv9-a', 'armv8.6-a', 'armv8.5-a', 'armv8.4-a', 'armv8.3-a',
+            supported_marchs = ['armv9-a', 'armv8.7-a', 'armv8.6-a', 'armv8.5-a', 'armv8.4-a', 'armv8.3-a',
                                 'armv8.2-a', 'armv8.1-a', 'armv8-a']
             check_compiler_support = false
             foreach supported_march: supported_marchs
-- 
2.34.1

[PATCH 2/2] eal: add Arm WFET in power management intrinsics

[Wathsala Vithanage]

Wait for event with timeout (WFET) puts the CPU in a low power
mode and stays there until an event is signalled (SEV), loss of
an exclusive monitor or a timeout.
WFET is enabled selectively by checking FEAT_WFxT in Linux
auxiliary vector. If FEAT_WFxT is not available power management
will fallback to WFE.
RTE_ARM_USE_WFE macro is not required to enable WFE feature for
PMD power monitoring.

Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
Reviewed-by: Dhruv Tripathi <dhruv.tripathi@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@foss.arm.com>
Reviewed-by: Nick Connolly <nick.connolly@arm.com>
Reviewed-by: Vinod Krishna <vinod.krishna@arm.com>

---
 .mailmap                                   |  4 +-
 app/test/test_cpuflags.c                   |  3 ++
 lib/eal/arm/include/rte_cpuflags_64.h      |  1 +
 lib/eal/arm/include/rte_pause_64.h         | 21 ++++++--
 lib/eal/arm/include/rte_power_intrinsics.h |  6 +++
 lib/eal/arm/rte_cpuflags.c                 |  3 +-
 lib/eal/arm/rte_power_intrinsics.c         | 56 ++++++++++++----------
 7 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/.mailmap b/.mailmap
index 3843868716..eefcce4935 100644
--- a/.mailmap
+++ b/.mailmap
@@ -332,6 +332,7 @@ Dexia Li <dexia.li@jaguarmicro.com>
 Dexuan Cui <decui@microsoft.com>
 Dharmik Thakkar <dharmikjayesh.thakkar@arm.com> <dharmik.thakkar@arm.com>
 Dheemanth Mallikarjun <dheemanthm@vmware.com>
+Dhruv Tripathi <dhruv.tripathi@arm.com>
 Diana Wang <na.wang@corigine.com>
 Didier Pallard <didier.pallard@6wind.com>
 Dilshod Urazov <dilshod.urazov@oktetlabs.ru>
@@ -1027,7 +1028,7 @@ Netanel Belgazal <netanel@amazon.com>
 Netanel Gonen <netanelg@mellanox.com>
 Niall Power <niall.power@intel.com>
 Nicholas Pratte <npratte@iol.unh.edu>
-Nick Connolly <nick.connolly@mayadata.io>
+Nick Connolly <nick.connolly@arm.com> <Nick.x.Connolly@gmail.com>
 Nick Nunley <nicholas.d.nunley@intel.com>
 Niclas Storm <niclas.storm@ericsson.com>
 Nicolas Chautru <nicolas.chautru@intel.com>
@@ -1516,6 +1517,7 @@ Vincent Jardin <vincent.jardin@6wind.com>
 Vincent Li <vincent.mc.li@gmail.com>
 Vincent S. Cojot <vcojot@redhat.com>
 Vinh Tran <vinh.t.tran10@gmail.com>
+Vinod Krishna <vinod.krishna@arm.com>
 Vipin Varghese <vipin.varghese@amd.com> <vipin.varghese@intel.com>
 Vipul Ashri <vipul.ashri@oracle.com>
 Visa Hankala <visa@hankala.org>
diff --git a/app/test/test_cpuflags.c b/app/test/test_cpuflags.c
index a0ff74720c..22ab4dff0a 100644
--- a/app/test/test_cpuflags.c
+++ b/app/test/test_cpuflags.c
@@ -156,6 +156,9 @@ test_cpuflags(void)
 
 	printf("Check for SVEBF16:\t");
 	CHECK_FOR_FLAG(RTE_CPUFLAG_SVEBF16);
+
+	printf("Check for WFXT:\t");
+	CHECK_FOR_FLAG(RTE_CPUFLAG_WFXT);
 #endif
 
 #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
diff --git a/lib/eal/arm/include/rte_cpuflags_64.h b/lib/eal/arm/include/rte_cpuflags_64.h
index afe70209c3..db61539db2 100644
--- a/lib/eal/arm/include/rte_cpuflags_64.h
+++ b/lib/eal/arm/include/rte_cpuflags_64.h
@@ -35,6 +35,7 @@ enum rte_cpu_flag_t {
 	RTE_CPUFLAG_SVEF32MM,
 	RTE_CPUFLAG_SVEF64MM,
 	RTE_CPUFLAG_SVEBF16,
+	RTE_CPUFLAG_WFXT,
 	RTE_CPUFLAG_AARCH64,
 };
 
diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..f732407425 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2017 Cavium, Inc
- * Copyright(c) 2019 Arm Limited
+ * Copyright(c) 2024 Arm Limited
  */
 
 #ifndef _RTE_PAUSE_ARM64_H_
@@ -23,17 +23,28 @@ static inline void rte_pause(void)
 	asm volatile("yield" ::: "memory");
 }
 
-#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
 
-/* Send a local event to quit WFE. */
+/* Send a local event to quit WFE/WFxT. */
 #define __RTE_ARM_SEVL() { asm volatile("sevl" : : : "memory"); }
 
-/* Send a global event to quit WFE for all cores. */
+/* Send a global event to quit WFE/WFxT for all cores. */
 #define __RTE_ARM_SEV() { asm volatile("sev" : : : "memory"); }
 
 /* Put processor into low power WFE(Wait For Event) state. */
 #define __RTE_ARM_WFE() { asm volatile("wfe" : : : "memory"); }
 
+/* Put processor into low power WFET (WFE with Timeout) state. */
+#ifdef RTE_ARM_FEATURE_WFXT
+#define __RTE_ARM_WFET(t) {                              \
+	asm volatile("wfet %x[to]"                        \
+			:                                 \
+			: [to] "r" (t)                    \
+			: "memory");                      \
+	}
+#else
+#define __RTE_ARM_WFET(t) { RTE_SET_USED(t); }
+#endif
+
 /*
  * Atomic exclusive load from addr, it returns the 8-bit content of
  * *addr while making it 'monitored', when it is written by someone
@@ -147,6 +158,8 @@ static inline void rte_pause(void)
 		__RTE_ARM_LOAD_EXC_128(src, dst, memorder) \
 }
 
+#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
+
 static __rte_always_inline void
 rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 		int memorder)
diff --git a/lib/eal/arm/include/rte_power_intrinsics.h b/lib/eal/arm/include/rte_power_intrinsics.h
index 9e498e9ebf..dc80fca60b 100644
--- a/lib/eal/arm/include/rte_power_intrinsics.h
+++ b/lib/eal/arm/include/rte_power_intrinsics.h
@@ -13,6 +13,12 @@ extern "C" {
 
 #include "generic/rte_power_intrinsics.h"
 
+struct rte_power_monitor_info {
+	uint16_t init_done:1,   /* Initialization status bit */
+		wfet_en:1,	/* FEAT_WFET enabled bit */
+		reserved:14;	/* Reserved */
+};
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/eal/arm/rte_cpuflags.c b/lib/eal/arm/rte_cpuflags.c
index 7ba4f8ba97..ad76f2448b 100644
--- a/lib/eal/arm/rte_cpuflags.c
+++ b/lib/eal/arm/rte_cpuflags.c
@@ -115,6 +115,7 @@ const struct feature_entry rte_cpu_feature_table[] = {
 	FEAT_DEF(SVEF32MM,	REG_HWCAP2,   10)
 	FEAT_DEF(SVEF64MM,	REG_HWCAP2,   11)
 	FEAT_DEF(SVEBF16,	REG_HWCAP2,   12)
+	FEAT_DEF(WFXT,		REG_HWCAP2,   31)
 	FEAT_DEF(AARCH64,	REG_PLATFORM,  0)
 };
 #endif /* RTE_ARCH */
@@ -163,7 +164,5 @@ void
 rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
 {
 	memset(intrinsics, 0, sizeof(*intrinsics));
-#ifdef RTE_ARM_USE_WFE
 	intrinsics->power_monitor = 1;
-#endif
 }
diff --git a/lib/eal/arm/rte_power_intrinsics.c b/lib/eal/arm/rte_power_intrinsics.c
index f54cf59e80..4c858192f2 100644
--- a/lib/eal/arm/rte_power_intrinsics.c
+++ b/lib/eal/arm/rte_power_intrinsics.c
@@ -4,20 +4,28 @@
 
 #include <errno.h>
 
+#include <rte_per_lcore.h>
+
+#include "rte_cpuflags.h"
 #include "rte_power_intrinsics.h"
 
 /**
- * This function uses WFE instruction to make lcore suspend
+ * Per core rte_power_monitor_info struct.
+ */
+RTE_DEFINE_PER_LCORE(struct rte_power_monitor_info, pm_info) = {
+	.init_done = 0,
+	.wfet_en = 0,
+};
+
+/**
+ * This function uses WFE/WFET instruction to make lcore suspend
  * execution on ARM.
- * Note that timestamp based timeout is not supported yet.
  */
 int
 rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 		const uint64_t tsc_timestamp)
 {
-	RTE_SET_USED(tsc_timestamp);
-
-#ifdef RTE_ARM_USE_WFE
+	struct rte_power_monitor_info *pminfo;
 	const unsigned int lcore_id = rte_lcore_id();
 	uint64_t cur_value;
 
@@ -31,33 +39,37 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 	if (pmc->fn == NULL)
 		return -EINVAL;
 
+	pminfo = &RTE_PER_LCORE(pm_info);
+
+	if (unlikely(!(pminfo->init_done))) {
+		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
+			pminfo->wfet_en = 1;
+		pminfo->init_done = 1;
+	}
+
 	switch (pmc->size) {
 	case sizeof(uint8_t):
-		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	case sizeof(uint16_t):
-		__RTE_ARM_LOAD_EXC_16(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_16(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	case sizeof(uint32_t):
-		__RTE_ARM_LOAD_EXC_32(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_32(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	case sizeof(uint64_t):
-		__RTE_ARM_LOAD_EXC_64(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_64(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	default:
 		return -EINVAL; /* unexpected size */
 	}
 
-	return 0;
-#else
-	RTE_SET_USED(pmc);
+	if (pminfo->wfet_en)
+		__RTE_ARM_WFET(tsc_timestamp)
+	else
+		__RTE_ARM_WFE()
 
-	return -ENOTSUP;
-#endif
+	return 0;
 }
 
 /**
@@ -80,14 +92,8 @@ int
 rte_power_monitor_wakeup(const unsigned int lcore_id)
 {
 	RTE_SET_USED(lcore_id);
-
-#ifdef RTE_ARM_USE_WFE
-	__RTE_ARM_SEV()
-
+	__RTE_ARM_SEV();
 	return 0;
-#else
-	return -ENOTSUP;
-#endif
 }
 
 int
-- 
2.34.1

Re: [PATCH 2/2] eal: add Arm WFET in power management intrinsics

[Stephen Hemminger]

On Tue,  4 Jun 2024 04:44:01 +0000
Wathsala Vithanage <wathsala.vithanage@arm.com> wrote:

> --- a/lib/eal/arm/include/rte_cpuflags_64.h
> +++ b/lib/eal/arm/include/rte_cpuflags_64.h
> @@ -35,6 +35,7 @@ enum rte_cpu_flag_t {
>  	RTE_CPUFLAG_SVEF32MM,
>  	RTE_CPUFLAG_SVEF64MM,
>  	RTE_CPUFLAG_SVEBF16,
> +	RTE_CPUFLAG_WFXT,
>  	RTE_CPUFLAG_AARCH64,
>  };

Adding new entry in middle of enum will cause ABI to change.

[PATCH v2 1/2] config/arm: adds Arm Neoverse N3 SoC

[Wathsala Vithanage]

Add Arm Neoverse N3 part number to build configuration.

Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
Reviewed-by: Dhruv Tripathi <dhruv.tripathi@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@foss.arm.com>

---
 config/arm/meson.build | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/config/arm/meson.build b/config/arm/meson.build
index a45aa9e466..ec7588f055 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -116,6 +116,18 @@ part_number_config_arm = {
             ['RTE_MAX_LCORE', 144],
             ['RTE_MAX_NUMA_NODES', 2]
         ]
+    },
+    '0xd8e': {
+        'march': 'armv8.7-a',
+        'march_features': ['sve2'],
+        'fallback_march': 'armv8.5-a',
+        'flags': [
+            ['RTE_MACHINE', '"neoverse-n3"'],
+            ['RTE_ARM_FEATURE_ATOMICS', true],
+            ['RTE_ARM_FEATURE_WFXT', true],
+            ['RTE_MAX_LCORE', 192],
+            ['RTE_MAX_NUMA_NODES', 2]
+        ]
     }
 }
 implementer_arm = {
@@ -555,6 +567,13 @@ soc_n2 = {
     'numa': false
 }
 
+soc_n3 = {
+    'description': 'Arm Neoverse N3',
+    'implementer': '0x41',
+    'part_number': '0xd8e',
+    'numa': false
+}
+
 soc_odyssey = {
     'description': 'Marvell Odyssey',
     'implementer': '0x41',
@@ -680,6 +699,7 @@ socs = {
     'kunpeng930': soc_kunpeng930,
     'n1sdp': soc_n1sdp,
     'n2': soc_n2,
+    'n3': soc_n3,
     'odyssey' : soc_odyssey,
     'stingray': soc_stingray,
     'thunderx2': soc_thunderx2,
@@ -833,7 +853,7 @@ if update_flags
         if part_number_config.get('force_march', false)
             candidate_march = part_number_config['march']
         else
-            supported_marchs = ['armv9-a', 'armv8.6-a', 'armv8.5-a', 'armv8.4-a', 'armv8.3-a',
+            supported_marchs = ['armv9-a', 'armv8.7-a', 'armv8.6-a', 'armv8.5-a', 'armv8.4-a', 'armv8.3-a',
                                 'armv8.2-a', 'armv8.1-a', 'armv8-a']
             check_compiler_support = false
             foreach supported_march: supported_marchs
-- 
2.34.1

[PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Wathsala Vithanage]

Wait for event with timeout (WFET) puts the CPU in a low power
mode and stays there until an event is signalled (SEV), loss of
an exclusive monitor or a timeout.
WFET is enabled selectively by checking FEAT_WFxT in Linux
auxiliary vector. If FEAT_WFxT is not available power management
will fallback to WFE.
RTE_ARM_USE_WFE macro is not required to enable WFE feature for
PMD power monitoring.

Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
Reviewed-by: Dhruv Tripathi <dhruv.tripathi@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@foss.arm.com>
Reviewed-by: Nick Connolly <nick.connolly@arm.com>
Reviewed-by: Vinod Krishna <vinod.krishna@arm.com>

---
 .mailmap                              |  2 ++
 app/test/test_cpuflags.c              |  3 ++
 lib/eal/arm/include/rte_cpuflags_64.h |  1 +
 lib/eal/arm/include/rte_pause_64.h    | 21 ++++++++++---
 lib/eal/arm/rte_cpuflags.c            |  3 +-
 lib/eal/arm/rte_power_intrinsics.c    | 45 +++++++++++++++++----------
 6 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/.mailmap b/.mailmap
index 3843868716..31995d492d 100644
--- a/.mailmap
+++ b/.mailmap
@@ -332,6 +332,7 @@ Dexia Li <dexia.li@jaguarmicro.com>
 Dexuan Cui <decui@microsoft.com>
 Dharmik Thakkar <dharmikjayesh.thakkar@arm.com> <dharmik.thakkar@arm.com>
 Dheemanth Mallikarjun <dheemanthm@vmware.com>
+Dhruv Tripathi <dhruv.tripathi@arm.com>
 Diana Wang <na.wang@corigine.com>
 Didier Pallard <didier.pallard@6wind.com>
 Dilshod Urazov <dilshod.urazov@oktetlabs.ru>
@@ -1516,6 +1517,7 @@ Vincent Jardin <vincent.jardin@6wind.com>
 Vincent Li <vincent.mc.li@gmail.com>
 Vincent S. Cojot <vcojot@redhat.com>
 Vinh Tran <vinh.t.tran10@gmail.com>
+Vinod Krishna <vinod.krishna@arm.com>
 Vipin Varghese <vipin.varghese@amd.com> <vipin.varghese@intel.com>
 Vipul Ashri <vipul.ashri@oracle.com>
 Visa Hankala <visa@hankala.org>
diff --git a/app/test/test_cpuflags.c b/app/test/test_cpuflags.c
index a0ff74720c..22ab4dff0a 100644
--- a/app/test/test_cpuflags.c
+++ b/app/test/test_cpuflags.c
@@ -156,6 +156,9 @@ test_cpuflags(void)
 
 	printf("Check for SVEBF16:\t");
 	CHECK_FOR_FLAG(RTE_CPUFLAG_SVEBF16);
+
+	printf("Check for WFXT:\t");
+	CHECK_FOR_FLAG(RTE_CPUFLAG_WFXT);
 #endif
 
 #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
diff --git a/lib/eal/arm/include/rte_cpuflags_64.h b/lib/eal/arm/include/rte_cpuflags_64.h
index afe70209c3..1945a97ca1 100644
--- a/lib/eal/arm/include/rte_cpuflags_64.h
+++ b/lib/eal/arm/include/rte_cpuflags_64.h
@@ -36,6 +36,7 @@ enum rte_cpu_flag_t {
 	RTE_CPUFLAG_SVEF64MM,
 	RTE_CPUFLAG_SVEBF16,
 	RTE_CPUFLAG_AARCH64,
+	RTE_CPUFLAG_WFXT,
 };
 
 #include "generic/rte_cpuflags.h"
diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..f732407425 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2017 Cavium, Inc
- * Copyright(c) 2019 Arm Limited
+ * Copyright(c) 2024 Arm Limited
  */
 
 #ifndef _RTE_PAUSE_ARM64_H_
@@ -23,17 +23,28 @@ static inline void rte_pause(void)
 	asm volatile("yield" ::: "memory");
 }
 
-#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
 
-/* Send a local event to quit WFE. */
+/* Send a local event to quit WFE/WFxT. */
 #define __RTE_ARM_SEVL() { asm volatile("sevl" : : : "memory"); }
 
-/* Send a global event to quit WFE for all cores. */
+/* Send a global event to quit WFE/WFxT for all cores. */
 #define __RTE_ARM_SEV() { asm volatile("sev" : : : "memory"); }
 
 /* Put processor into low power WFE(Wait For Event) state. */
 #define __RTE_ARM_WFE() { asm volatile("wfe" : : : "memory"); }
 
+/* Put processor into low power WFET (WFE with Timeout) state. */
+#ifdef RTE_ARM_FEATURE_WFXT
+#define __RTE_ARM_WFET(t) {                              \
+	asm volatile("wfet %x[to]"                        \
+			:                                 \
+			: [to] "r" (t)                    \
+			: "memory");                      \
+	}
+#else
+#define __RTE_ARM_WFET(t) { RTE_SET_USED(t); }
+#endif
+
 /*
  * Atomic exclusive load from addr, it returns the 8-bit content of
  * *addr while making it 'monitored', when it is written by someone
@@ -147,6 +158,8 @@ static inline void rte_pause(void)
 		__RTE_ARM_LOAD_EXC_128(src, dst, memorder) \
 }
 
+#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
+
 static __rte_always_inline void
 rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 		int memorder)
diff --git a/lib/eal/arm/rte_cpuflags.c b/lib/eal/arm/rte_cpuflags.c
index 7ba4f8ba97..ad76f2448b 100644
--- a/lib/eal/arm/rte_cpuflags.c
+++ b/lib/eal/arm/rte_cpuflags.c
@@ -115,6 +115,7 @@ const struct feature_entry rte_cpu_feature_table[] = {
 	FEAT_DEF(SVEF32MM,	REG_HWCAP2,   10)
 	FEAT_DEF(SVEF64MM,	REG_HWCAP2,   11)
 	FEAT_DEF(SVEBF16,	REG_HWCAP2,   12)
+	FEAT_DEF(WFXT,		REG_HWCAP2,   31)
 	FEAT_DEF(AARCH64,	REG_PLATFORM,  0)
 };
 #endif /* RTE_ARCH */
@@ -163,7 +164,5 @@ void
 rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
 {
 	memset(intrinsics, 0, sizeof(*intrinsics));
-#ifdef RTE_ARM_USE_WFE
 	intrinsics->power_monitor = 1;
-#endif
 }
diff --git a/lib/eal/arm/rte_power_intrinsics.c b/lib/eal/arm/rte_power_intrinsics.c
index f54cf59e80..fc7a0c61f0 100644
--- a/lib/eal/arm/rte_power_intrinsics.c
+++ b/lib/eal/arm/rte_power_intrinsics.c
@@ -4,20 +4,31 @@
 
 #include <errno.h>
 
+#include "rte_cpuflags.h"
 #include "rte_power_intrinsics.h"
 
 /**
- * This function uses WFE instruction to make lcore suspend
+ *  Set wfet_en if WFET is supported
+ */
+uint8_t wfet_en;
+
+RTE_INIT(rte_power_intrinsics_init)
+{
+#ifdef RTE_ARCH_64
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
+		wfet_en = 1;
+#endif
+}
+
+/**
+ * This function uses WFE/WFET instruction to make lcore suspend
  * execution on ARM.
- * Note that timestamp based timeout is not supported yet.
  */
 int
 rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 		const uint64_t tsc_timestamp)
 {
-	RTE_SET_USED(tsc_timestamp);
-
-#ifdef RTE_ARM_USE_WFE
+#ifdef RTE_ARCH_64
 	const unsigned int lcore_id = rte_lcore_id();
 	uint64_t cur_value;
 
@@ -33,28 +44,30 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 
 	switch (pmc->size) {
 	case sizeof(uint8_t):
-		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	case sizeof(uint16_t):
-		__RTE_ARM_LOAD_EXC_16(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_16(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	case sizeof(uint32_t):
-		__RTE_ARM_LOAD_EXC_32(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_32(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	case sizeof(uint64_t):
-		__RTE_ARM_LOAD_EXC_64(pmc->addr, cur_value, rte_memory_order_relaxed)
-		__RTE_ARM_WFE()
+		__RTE_ARM_LOAD_EXC_64(pmc->addr, cur_value, rte_memory_order_relaxed);
 		break;
 	default:
 		return -EINVAL; /* unexpected size */
 	}
 
+	if (wfet_en)
+		__RTE_ARM_WFET(tsc_timestamp)
+	else
+		__RTE_ARM_WFE()
+
 	return 0;
 #else
 	RTE_SET_USED(pmc);
+	RTE_SET_USED(tsc_timestamp);
 
 	return -ENOTSUP;
 #endif
@@ -80,10 +93,8 @@ int
 rte_power_monitor_wakeup(const unsigned int lcore_id)
 {
 	RTE_SET_USED(lcore_id);
-
-#ifdef RTE_ARM_USE_WFE
-	__RTE_ARM_SEV()
-
+#ifdef RTE_ARCH_64
+	__RTE_ARM_SEV();
 	return 0;
 #else
 	return -ENOTSUP;
-- 
2.34.1

Re: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Thomas Monjalon]

19/06/2024 08:45, Wathsala Vithanage:
> --- a/lib/eal/arm/include/rte_cpuflags_64.h
> +++ b/lib/eal/arm/include/rte_cpuflags_64.h
> @@ -36,6 +36,7 @@ enum rte_cpu_flag_t {
>  	RTE_CPUFLAG_SVEF64MM,
>  	RTE_CPUFLAG_SVEBF16,
>  	RTE_CPUFLAG_AARCH64,
> +	RTE_CPUFLAG_WFXT,
>  };

It may be useful to add comments explaining each flag.
May be a separate patch in this series?


> - * Copyright(c) 2019 Arm Limited
> + * Copyright(c) 2024 Arm Limited

No, it's wrong to remove initial date,
and no, you don't need to update dates at all.


> -#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED

Why removing this #ifdef?

> -/* Send a local event to quit WFE. */
> +/* Send a local event to quit WFE/WFxT. */
>  #define __RTE_ARM_SEVL() { asm volatile("sevl" : : : "memory"); }
>  
> -/* Send a global event to quit WFE for all cores. */
> +/* Send a global event to quit WFE/WFxT for all cores. */
>  #define __RTE_ARM_SEV() { asm volatile("sev" : : : "memory"); }
>  
>  /* Put processor into low power WFE(Wait For Event) state. */
>  #define __RTE_ARM_WFE() { asm volatile("wfe" : : : "memory"); }
>  
> +/* Put processor into low power WFET (WFE with Timeout) state. */
> +#ifdef RTE_ARM_FEATURE_WFXT
> +#define __RTE_ARM_WFET(t) {                              \
> +	asm volatile("wfet %x[to]"                        \
> +			:                                 \
> +			: [to] "r" (t)                    \
> +			: "memory");                      \
> +	}

Is there any intrinsic function available?


[...]
> --- a/lib/eal/arm/rte_cpuflags.c
> +++ b/lib/eal/arm/rte_cpuflags.c
> @@ -115,6 +115,7 @@ const struct feature_entry rte_cpu_feature_table[] = {
>  	FEAT_DEF(SVEF32MM,	REG_HWCAP2,   10)
>  	FEAT_DEF(SVEF64MM,	REG_HWCAP2,   11)
>  	FEAT_DEF(SVEBF16,	REG_HWCAP2,   12)
> +	FEAT_DEF(WFXT,		REG_HWCAP2,   31)
>  	FEAT_DEF(AARCH64,	REG_PLATFORM,  0)

Are you sure of alignment? (looks wrong in my email client)


[...]
>  rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
>  {
>  	memset(intrinsics, 0, sizeof(*intrinsics));
> -#ifdef RTE_ARM_USE_WFE
>  	intrinsics->power_monitor = 1;
> -#endif

Why removing this #ifdef?


> +uint8_t wfet_en;

It should be made static probably.
This variable will be unused in some cases, needs #ifdef.

> +
> +RTE_INIT(rte_power_intrinsics_init)
> +{
> +#ifdef RTE_ARCH_64
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
> +		wfet_en = 1;
> +#endif
> +}
> +
> +/**
> + * This function uses WFE/WFET instruction to make lcore suspend
>   * execution on ARM.
> - * Note that timestamp based timeout is not supported yet.
>   */
>  int
>  rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>  		const uint64_t tsc_timestamp)
>  {
> -	RTE_SET_USED(tsc_timestamp);
> -
> -#ifdef RTE_ARM_USE_WFE
> +#ifdef RTE_ARCH_64

It looks wrong.
If RTE_ARM_USE_WFE is disabled, you should not call __RTE_ARM_WFE().

>  	const unsigned int lcore_id = rte_lcore_id();
>  	uint64_t cur_value;
>  
> @@ -33,28 +44,30 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>  
>  	switch (pmc->size) {
>  	case sizeof(uint8_t):
> -		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed)
> -		__RTE_ARM_WFE()
> +		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed);

RE: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Wathsala Wathawana Vithanage]

> 19/06/2024 08:45, Wathsala Vithanage:
> > --- a/lib/eal/arm/include/rte_cpuflags_64.h
> > +++ b/lib/eal/arm/include/rte_cpuflags_64.h
> > @@ -36,6 +36,7 @@ enum rte_cpu_flag_t {
> >  	RTE_CPUFLAG_SVEF64MM,
> >  	RTE_CPUFLAG_SVEBF16,
> >  	RTE_CPUFLAG_AARCH64,
> > +	RTE_CPUFLAG_WFXT,
> >  };
> 
> It may be useful to add comments explaining each flag.
> May be a separate patch in this series?
> 
+1
> 
> > - * Copyright(c) 2019 Arm Limited
> > + * Copyright(c) 2024 Arm Limited
> 
> No, it's wrong to remove initial date,
> and no, you don't need to update dates at all.
> 
> 
> > -#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
> 
> Why removing this #ifdef?

It's not removed, it's moved further down and just above rte_wait_until_equal_X functions.
Use of SEV, and WFE are not limited to  rte_wait_until_equal_X functions, 
PMDs should be able to use them for power management. 

> 
> > -/* Send a local event to quit WFE. */
> > +/* Send a local event to quit WFE/WFxT. */
> >  #define __RTE_ARM_SEVL() { asm volatile("sevl" : : : "memory"); }
> >
> > -/* Send a global event to quit WFE for all cores. */
> > +/* Send a global event to quit WFE/WFxT for all cores. */
> >  #define __RTE_ARM_SEV() { asm volatile("sev" : : : "memory"); }
> >
> >  /* Put processor into low power WFE(Wait For Event) state. */
> > #define __RTE_ARM_WFE() { asm volatile("wfe" : : : "memory"); }
> >
> > +/* Put processor into low power WFET (WFE with Timeout) state. */
> > +#ifdef RTE_ARM_FEATURE_WFXT
> > +#define __RTE_ARM_WFET(t) {                              \
> > +	asm volatile("wfet %x[to]"                        \
> > +			:                                 \
> > +			: [to] "r" (t)                    \
> > +			: "memory");                      \
> > +	}
> 
> Is there any intrinsic function available?
> 

We don't have an intrinsic at the moment.

> 
> [...]
> > --- a/lib/eal/arm/rte_cpuflags.c
> > +++ b/lib/eal/arm/rte_cpuflags.c
> > @@ -115,6 +115,7 @@ const struct feature_entry rte_cpu_feature_table[] =
> {
> >  	FEAT_DEF(SVEF32MM,	REG_HWCAP2,   10)
> >  	FEAT_DEF(SVEF64MM,	REG_HWCAP2,   11)
> >  	FEAT_DEF(SVEBF16,	REG_HWCAP2,   12)
> > +	FEAT_DEF(WFXT,		REG_HWCAP2,   31)
> >  	FEAT_DEF(AARCH64,	REG_PLATFORM,  0)
> 
> Are you sure of alignment? (looks wrong in my email client)

Didn't see this before, I will check.

> 
> 
> [...]
> >  rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
> > {
> >  	memset(intrinsics, 0, sizeof(*intrinsics)); -#ifdef RTE_ARM_USE_WFE
> >  	intrinsics->power_monitor = 1;
> > -#endif
> 
> Why removing this #ifdef?

WFE is available in all the Arm platforms DPDK currently supports. Therefore, an explicit flag is not
required at build time. 

The purpose of RTE_ARM_USE_WFE seems to be controlling the use of WFE in rte_wait_until_equal
functions by defining RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED macro  only when RTE_ARM_USE_WFE
is defined. (eal/arm/include/rte_pause_64.h:15)
From what I'm hearing this was done due to a performance issue rte_wait_until_equal_X functions had
when using WFE.
However, that design is flawed because it made other users of WFE dependent on the choice of
the use of WFE in rte_wait_until_equal functions as __RTE_ARM_WFE was wrapped in an
RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED #ifdef block.
This patch fixes this issue by moving __RTE_ARM_WFE out of RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
block.

Perhaps we should change RTE_ARM_USE_WFE to something like 
RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL ?

> 
> 
> > +uint8_t wfet_en;
> 
> It should be made static probably.
> This variable will be unused in some cases, needs #ifdef.
> 

This variable is used in all cases. It's 1 when WFET is available, 0 when it's not.


> > +
> > +RTE_INIT(rte_power_intrinsics_init)
> > +{
> > +#ifdef RTE_ARCH_64
> > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
> > +		wfet_en = 1;
> > +#endif
> > +}
> > +
> > +/**
> > + * This function uses WFE/WFET instruction to make lcore suspend
> >   * execution on ARM.
> > - * Note that timestamp based timeout is not supported yet.
> >   */
> >  int
> >  rte_power_monitor(const struct rte_power_monitor_cond *pmc,
> >  		const uint64_t tsc_timestamp)
> >  {
> > -	RTE_SET_USED(tsc_timestamp);
> > -
> > -#ifdef RTE_ARM_USE_WFE
> > +#ifdef RTE_ARCH_64
> 
> It looks wrong.
> If RTE_ARM_USE_WFE is disabled, you should not call __RTE_ARM_WFE().
> 

RTE_ARM_USE_WFE should be renamed to reflect its actual use. It's safe to assume that
WFE is available universally in Arm DPDK context.

> >  	const unsigned int lcore_id = rte_lcore_id();
> >  	uint64_t cur_value;
> >
> > @@ -33,28 +44,30 @@ rte_power_monitor(const struct
> > rte_power_monitor_cond *pmc,
> >
> >  	switch (pmc->size) {
> >  	case sizeof(uint8_t):
> > -		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value,
> rte_memory_order_relaxed)
> > -		__RTE_ARM_WFE()
> > +		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value,
> > +rte_memory_order_relaxed);
> 
> 

Re: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Thomas Monjalon]

01/07/2024 23:34, Wathsala Wathawana Vithanage:
> > 19/06/2024 08:45, Wathsala Vithanage:
> > >  rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
> > > {
> > >  	memset(intrinsics, 0, sizeof(*intrinsics)); -#ifdef RTE_ARM_USE_WFE
> > >  	intrinsics->power_monitor = 1;
> > > -#endif
> > 
> > Why removing this #ifdef?
> 
> WFE is available in all the Arm platforms DPDK currently supports. Therefore, an explicit flag is not
> required at build time. 
> 
> The purpose of RTE_ARM_USE_WFE seems to be controlling the use of WFE in rte_wait_until_equal
> functions by defining RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED macro  only when RTE_ARM_USE_WFE
> is defined. (eal/arm/include/rte_pause_64.h:15)
> From what I'm hearing this was done due to a performance issue rte_wait_until_equal_X functions had
> when using WFE.
> However, that design is flawed because it made other users of WFE dependent on the choice of
> the use of WFE in rte_wait_until_equal functions as __RTE_ARM_WFE was wrapped in an
> RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED #ifdef block.
> This patch fixes this issue by moving __RTE_ARM_WFE out of RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
> block.
> 
> Perhaps we should change RTE_ARM_USE_WFE to something like 
> RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL ?

Yes perhaps.
And more importantly, you should do such change in a separate patch.
Don't hide it in WFET patch.

> > > +uint8_t wfet_en;
> > 
> > It should be made static probably.
> > This variable will be unused in some cases, needs #ifdef.
> > 
> 
> This variable is used in all cases. It's 1 when WFET is available, 0 when it's not.

It would be 0 if you make it static yes.

> > > +
> > > +RTE_INIT(rte_power_intrinsics_init)
> > > +{
> > > +#ifdef RTE_ARCH_64
> > > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
> > > +		wfet_en = 1;
> > > +#endif
> > > +}
> > > +
> > > +/**
> > > + * This function uses WFE/WFET instruction to make lcore suspend
> > >   * execution on ARM.
> > > - * Note that timestamp based timeout is not supported yet.
> > >   */
> > >  int
> > >  rte_power_monitor(const struct rte_power_monitor_cond *pmc,
> > >  		const uint64_t tsc_timestamp)
> > >  {
> > > -	RTE_SET_USED(tsc_timestamp);
> > > -
> > > -#ifdef RTE_ARM_USE_WFE
> > > +#ifdef RTE_ARCH_64
> > 
> > It looks wrong.
> > If RTE_ARM_USE_WFE is disabled, you should not call __RTE_ARM_WFE().
> > 
> 
> RTE_ARM_USE_WFE should be renamed to reflect its actual use. It's safe to assume that
> WFE is available universally in Arm DPDK context.

RE: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Wathsala Wathawana Vithanage]

> RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED #ifdef block.
> > This patch fixes this issue by moving __RTE_ARM_WFE out of
> > RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED block.
> >
> > Perhaps we should change RTE_ARM_USE_WFE to something like
> > RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL ?
> 
> Yes perhaps.
RTE_ARM_USE_WFE is already used in drivers/event/cnxk/cn10k_worker.h
therefore RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL is not suitable.
I wouldn't mind keeping RTE_ARM_USE_WFE because "USE_WFE" sounds like an
instruction to use WFE rather than an indication of availability of the WFE instruction. 

Re: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Thomas Monjalon]

03/07/2024 15:27, Wathsala Wathawana Vithanage:
> > RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED #ifdef block.
> > > This patch fixes this issue by moving __RTE_ARM_WFE out of
> > > RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED block.
> > >
> > > Perhaps we should change RTE_ARM_USE_WFE to something like
> > > RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL ?
> > 
> > Yes perhaps.
> RTE_ARM_USE_WFE is already used in drivers/event/cnxk/cn10k_worker.h
> therefore RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL is not suitable.
> I wouldn't mind keeping RTE_ARM_USE_WFE because "USE_WFE" sounds like an
> instruction to use WFE rather than an indication of availability of the WFE instruction. 

The problem is that the definition of this flag is not clear.
What is it doing?
If it's really disabling WFE, keep the #ifdef to not use it.

For now, it is a nack of this patch for all reasons described before.

RE: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Wathsala Wathawana Vithanage]

> > > > +uint8_t wfet_en;
> > >
> > > It should be made static probably.
> > > This variable will be unused in some cases, needs #ifdef.
> > >
> >
> > This variable is used in all cases. It's 1 when WFET is available, 0 when it's not.
> 
> It would be 0 if you make it static yes.

No need to make it static to set it to 0, it will be placed in BSS section as is hence 
will always be initialized to 0.
Static only limits its scope to this file, has nothing to do with initialization to 0.

RE: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Wathsala Wathawana Vithanage]

> 03/07/2024 15:27, Wathsala Wathawana Vithanage:
> > > RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED #ifdef block.
> > > > This patch fixes this issue by moving __RTE_ARM_WFE out of
> > > > RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED block.
> > > >
> > > > Perhaps we should change RTE_ARM_USE_WFE to something like
> > > > RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL ?
> > >
> > > Yes perhaps.
> > RTE_ARM_USE_WFE is already used in drivers/event/cnxk/cn10k_worker.h
> > therefore RTE_ARM_USE_WFE_IN_WAIT_UNTIL_EQUAL is not suitable.
> > I wouldn't mind keeping RTE_ARM_USE_WFE because "USE_WFE" sounds
> like
> > an instruction to use WFE rather than an indication of availability of the WFE
> instruction.
> 
> The problem is that the definition of this flag is not clear.
> What is it doing?
> If it's really disabling WFE, keep the #ifdef to not use it.
> 
> For now, it is a nack of this patch for all reasons described before.
> 

Only other place where this flag is used is drivers/event/cnxk/cn10k_worker.h

b8dbcbe8a57 (Pavan Nikhilesh      2024-02-27 13:41:53 +0530 284) #if defined(RTE_ARM_USE_WFE)

Let’s ask Pavan why this flag is used in cn10k driver. 

From our perspective, WFE is available on all the supported arm platforms in DPDK.
Therefore, RTE_ARM_USE_WFE should be treated as a flag to choose between WFE
and non-WFE code paths due to performance reasons rather than as a flag that indicates
the availability of the instruction on the target CPU.