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

[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.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.
> 
> 

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