Re: [dpdk-dev] [RFC UNTESTED PATCH] eal_common_cpuflags: Fix %rbx corruption, and simplify the code

["H. Peter Anvin" <hpa@zytor.com>]

I just realized there is yet another oddity in this code:

> @@ -78,8 +69,10 @@ struct cpuid_parameters_t {
>  struct feature_entry {
>  	enum rte_cpu_flag_t feature;            /**< feature name */

The structure contains a field with an enum value...

>  	char name[CPU_FLAG_NAME_MAX_LEN];       /**< String for printing */
> -	struct cpuid_parameters_t params;       /**< cpuid parameters */
> -	uint32_t feature_mask;                  /**< bitmask for feature */
> +	uint32_t leaf;				/**< cpuid leaf */
> +	uint32_t subleaf;			/**< cpuid subleaf */
> +	uint32_t reg;				/**< cpuid register */
> +	uint32_t bit;				/**< cpuid register bit */
>  };
>  
>  
>  /*
> @@ -240,17 +207,20 @@ rte_cpu_get_features(struct cpuid_parameters_t params)
>  int
>  rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
>  {
> -	int value;
> +	const struct feature_entry *feat;
> +	cpu_registers_t regs;
>  
>  	if (feature >= RTE_CPUFLAG_NUMFLAGS)
>  		/* Flag does not match anything in the feature tables */
>  		return -ENOENT;
>  
> -	/* get value of the register containing the desired feature */
> -	value = rte_cpu_get_features(cpu_feature_table[feature].params);
> +	feat = &cpu_feature_table[feature];
> +
> +	/* get the cpuid leaf containing the desired feature */
> +	rte_cpu_get_features(feat->leaf, feat->subleaf, &regs);
>  
>  	/* check if the feature is enabled */
> -	return (cpu_feature_table[feature].feature_mask & value) > 0;
> +	return (regs[feat->reg] >> feat->bit) & 1;
>  }
>  
>  /**

... however, this field is never actually accessed *anywhere* in the
code; the code instead uses the enum value as the table index. There is
absolutely no enforcement that the table contents is aligned with the enum.

If C99-style initializers are permitted in this codebase, I would
strongly recommend using them, and then drop the enum field in struct
feature_entry and use a macro such as:

#define FEAT(name,leaf,subleaf,reg,bit) \
	[RTE_CPUFLAG_##f] = { leaf, subleaf, reg, bit, #f },

(I'd move the string to the end, but that is just a microoptimization.
I'm kind of OCD that way.)

	-hpa