On 29-Oct-20 9:27 PM, David Marchand wrote:
On Tue, Oct 27, 2020 at 4:00 PM Liang Ma <liang.j...@intel.com> wrote:
Currently, it is not possible to check support for intrinsics that
are platform-specific, cannot be abstracted in a generic way, or do not
have support on all architectures. The CPUID flags can be used to some
extent, but they are only defined for their platform, while intrinsics
will be available to all code as they are in generic headers.
This patch introduces infrastructure to check support for certain
platform-specific intrinsics, and adds support for checking support for
IA power management-related intrinsics for UMWAIT/UMONITOR and TPAUSE.
Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
Signed-off-by: Liang Ma <liang.j...@intel.com>
Acked-by: David Christensen <d...@linux.vnet.ibm.com>
Acked-by: Jerin Jacob <jer...@marvell.com>
Acked-by: Ruifeng Wang <ruifeng.w...@arm.com>
Acked-by: Ray Kinsella <m...@ashroe.eu>
Coming late to the party, it seems crowded...
diff --git a/lib/librte_eal/include/generic/rte_cpuflags.h
b/lib/librte_eal/include/generic/rte_cpuflags.h
index 872f0ebe3e..28a5aecde8 100644
--- a/lib/librte_eal/include/generic/rte_cpuflags.h
+++ b/lib/librte_eal/include/generic/rte_cpuflags.h
@@ -13,6 +13,32 @@
#include "rte_common.h"
#include <errno.h>
+#include <rte_compat.h>
+
+/**
+ * Structure used to describe platform-specific intrinsics that may or may not
+ * be supported at runtime.
+ */
+struct rte_cpu_intrinsics {
+ uint32_t power_monitor : 1;
+ /**< indicates support for rte_power_monitor function */
+ uint32_t power_pause : 1;
+ /**< indicates support for rte_power_pause function */
+};
- The rte_power library is supposed to be built on top of cpuflags.
Not the other way around.
Those capabilities should have been kept inside the rte_power_ API and
not pollute the cpuflags API.
- All of this should have come as a single patch as the previously
introduced API is unusable before.
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Check CPU support for various intrinsics at runtime.
+ *
+ * @param intrinsics
+ * Pointer to a structure to be filled.
+ */
+__rte_experimental
+void
+rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics);
+
/**
* Enumeration of all CPU features supported
*/
diff --git a/lib/librte_eal/include/generic/rte_power_intrinsics.h
b/lib/librte_eal/include/generic/rte_power_intrinsics.h
index fb897d9060..03a326f076 100644
--- a/lib/librte_eal/include/generic/rte_power_intrinsics.h
+++ b/lib/librte_eal/include/generic/rte_power_intrinsics.h
@@ -32,6 +32,10 @@
* checked against the expected value, and if they match, the entering of
* optimized power state may be aborted.
*
+ * @warning It is responsibility of the user to check if this function is
+ * supported at runtime using `rte_cpu_get_features()` API call. Failing to
do
+ * so may result in an illegal CPU instruction error.
+ *
- Reading this API description... what am I supposed to do in my
application or driver who wants to use the new
rte_power_monitor/rte_power_pause stuff?
I should call rte_cpu_get_features(TOTO) ?
This comment does not give a hint.
I suppose the intent was to refer to the rte_cpu_get_intrinsics_support() thing.
This must be fixed.
- Again, I wonder why we are exposing all this stuff.
This should be hidden in the rte_power API.
We're exposing all of this here because the intrinsics are *not* part of
the power API but rather are generic headers within EAL. Therefore, any
infrastructure checking for their support can *not* reside in the power
library, but instead has to be in EAL.
The intended usage here is to call this function before calling
rte_power_monitor(), such that:
struct rte_cpu_intrinsics intrinsics;
rte_cpu_get_intrinsics_support(&intrinsics);
if (!intrinsics.power_monitor) {
// rte_power_monitor not supported and cannot be used
return;
}
// proceed with rte_power_monitor usage
Failing to do that will result in either -ENOTSUP on non-x86, or illegal
instruction crash on x86 that doesn't have that instruction (because we
encode raw opcode).
I've *not* added this to the previous patches because i wanted to get
this part reviewed specifically, and not mix it with other IA-specific
stuff. It seems that i've succeeded in that goal, as this patch has 4
likes^W acks :)
--
Thanks,
Anatoly
