From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 8CA94159 for ; Mon, 24 Mar 2014 17:05:41 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1WS7Oi-0001dM-4p; Mon, 24 Mar 2014 12:07:06 -0400 Date: Mon, 24 Mar 2014 12:06:47 -0400 From: Neil Horman To: "H. Peter Anvin" Message-ID: <20140324160647.GF19368@hmsreliant.think-freely.org> References: <1395333868-2808-1-git-send-email-hpa@linux.intel.com> <532B1F79.1050308@zytor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <532B1F79.1050308@zytor.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org, "H. Peter Anvin" Subject: Re: [dpdk-dev] [RFC UNTESTED PATCH] eal_common_cpuflags: Fix %rbx corruption, and simplify the code X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 Mar 2014 16:05:41 -0000 On Thu, Mar 20, 2014 at 10:03:53AM -0700, H. Peter Anvin wrote: > 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, ®s); > > > > /* 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: > Actually, its a bit simpler than that, the enum parameter is actually completely unused, and so can be removed entirely. The FEAT_DEF macro does what you suggest below already, but only for the feature and name fields. I'll remove the enum and its definition, and augment the macro to cover the rest of the fields. Neil > #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 > >