From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [198.137.202.10]) by dpdk.org (Postfix) with ESMTP id AEA9B68C0 for ; Thu, 20 Mar 2014 18:02:30 +0100 (CET) Received: from tazenda.hos.anvin.org ([IPv6:2601:9:7280:8f0:cc79:79ff:fead:f559]) (authenticated bits=0) by mail.zytor.com (8.14.7/8.14.5) with ESMTP id s2KH3xod017185 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 20 Mar 2014 10:04:01 -0700 Message-ID: <532B1F79.1050308@zytor.com> Date: Thu, 20 Mar 2014 10:03:53 -0700 From: "H. Peter Anvin" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: "H. Peter Anvin" , dev@dpdk.org References: <1395333868-2808-1-git-send-email-hpa@linux.intel.com> In-Reply-To: <1395333868-2808-1-git-send-email-hpa@linux.intel.com> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Thu, 20 Mar 2014 17:02:31 -0000 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: #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