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.
--
David Marchand
