30/10/2020 11:09, Burakov, Anatoly:
> 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;
> }
This check could be done inside the rte_power API.
> // 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 :)
You did not explain the need for rte_cpu_get_features() call.
