[dpdk-dev] [PATCH v10 3/9] eal: add intrinsics support check infrastructure

[David Marchand]

On Tue, Oct 27, 2020 at 4:00 PM Liang Ma <liang.j.ma at 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.burakov at intel.com>
> Signed-off-by: Liang Ma <liang.j.ma at intel.com>
> Acked-by: David Christensen <drc at linux.vnet.ibm.com>
> Acked-by: Jerin Jacob <jerinj at marvell.com>
> Acked-by: Ruifeng Wang <ruifeng.wang at arm.com>
> Acked-by: Ray Kinsella <mdr at 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