From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58184) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URkaT-00005R-Rl for qemu-devel@nongnu.org; Mon, 15 Apr 2013 10:40:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1URkaO-0000pv-EC for qemu-devel@nongnu.org; Mon, 15 Apr 2013 10:40:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2985) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URkaO-0000pp-4L for qemu-devel@nongnu.org; Mon, 15 Apr 2013 10:40:48 -0400 Date: Mon, 15 Apr 2013 16:40:42 +0200 From: Igor Mammedov Message-ID: <20130415164042.71bd6ec4@nial.usersys.redhat.com> In-Reply-To: <20130415134036.GA2719@otherpad.lan.raisama.net> References: <1365710844-30453-1-git-send-email-ehabkost@redhat.com> <1365710844-30453-3-git-send-email-ehabkost@redhat.com> <20130415105020.13df4df6@nial.usersys.redhat.com> <20130415134036.GA2719@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qom-cpu-next 2/2] target-i386: Replace cpuid_*features fields with a feature word array List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Andreas =?ISO-8859-1?B?RuRyYmVy?= On Mon, 15 Apr 2013 10:40:36 -0300 Eduardo Habkost wrote: > On Mon, Apr 15, 2013 at 10:50:20AM +0200, Igor Mammedov wrote: > > On Thu, 11 Apr 2013 17:07:24 -0300 > > Eduardo Habkost wrote: > > > > > This replaces the feature-bit fields on both X86CPU and x86_def_t > > > structs with an array. > > > > > > With this, we will be able to simplify code that simply does the same > > > operation on all feature words (e.g. kvm_check_features_against_host(), > > > filter_features_for_kvm(), add_flagname_to_bitmaps(), CPU feature-bit > > > property lookup/registration, and the proposed "feature-words" property) > > > > > > This should also help avoid bugs like the ones introduced when we added > > > cpuid_7_0_ebx_features. Today, adding a new feature word to the code > > > requires chaning 5 or 6 different places in the code, and it's very easy > > > to miss a problem when we forget to update one of those parts. See, for > > > example: > > > > > > * The bug solved by commit ffa8c11f0bbf47e1b7a3a62f97bc1da591c6734a; > > > (CPUID 7 leaf was not being filtered based on host capabilities) > > > * The bug solved by commit 07ca59450c9a0c5df65665ce46aa8487af59a1dd > > > (check/enforce flags were not checking all feature flags) > > > > > > Signed-off-by: Eduardo Habkost > > > --- > > > This patch was created solely using a sed script and no manual changes, > > > to try to avoid mistakes while converting the code, and make it easier > > > to rebase if necessary. The sed script can be seen at: > > > https://gist.github.com/4271991 > > It doesn't apply anymore. > > I'll redo and resend. Thanks. > > > > > > > Changes v7: > > > - Rebase on top qom-cpu-next > > > (commit 3755f0a9d48da07258f4a0ef5e883272799e47b9) > > commit IDs are often useless on staging tree, since they are changing, > > pls use commit's subj. > > I'll do it next time. > > > > > > > Changes v6: > > > - Rebase on top of Andreas' qom-cpu tree (commit > > > 9260944307077b93a66bf861a467107af986fe47) > > > - Break lines on kvm_check_features_against_host() > > > - Break the lines on builtin_x86_defs just after the "=". > > > This way the feature lists stay on separate lines, this patch gets > > > easier to review, and future patches that touches the code around > > > builtin_x86_defs will be even easier to review (as they won't need > > > to touch the lines containing the fature lists again) > > > > > > Signed-off-by: Eduardo Habkost > > > --- > > [...] > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index c2e02fe..e506d12 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -348,21 +348,14 @@ static void add_flagname_to_bitmaps(const char > > > *flagname, > > > typedef struct x86_def_t { > > > const char *name; > > > - uint32_t level; > > > + uint32_t level, xlevel, xlevel2; > > it's not cpuid_*features that patch re-factors, pls move to separate patch > > if this movement necessary at all. I don't see any gain in touch it. > > Before applying this patch, xlevel was close to cpuid_ext[23]_features > and xlevel2 was close to cpuid_ext4_features. Leaving them at completely > random places in the struct after removing the cpuid_*_features field > looks confusing to me. then do it ask separate clean up patch. and it would be nicer with one field per line, instead of ^^^. > > At the same time, moving them without removing/moving the > cpuid_*_features at the same time wouldn't make sense to me either. So I > decided to move them in the same patch. > > > > > > + FeatureWordArray features; > > > /* vendor is zero-terminated, 12 character ASCII string */ > > > char vendor[CPUID_VENDOR_SZ + 1]; > > [...] > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > > index 2b4e319..299a793 100644 > > > --- a/target-i386/cpu.h > > > +++ b/target-i386/cpu.h > > > @@ -834,23 +834,14 @@ typedef struct CPUX86State { > > > uint64_t pat; > > > > > > /* processor features (e.g. for CPUID insn) */ > > > - uint32_t cpuid_level; > > > + uint32_t cpuid_level, cpuid_xlevel, cpuid_xlevel2;ditto > > ditto > > > > > + FeatureWordArray features; > > > uint32_t cpuid_vendor1; > > > uint32_t cpuid_vendor2; > > > uint32_t cpuid_vendor3; > > > uint32_t cpuid_version; > > > - uint32_t cpuid_features; > > > - uint32_t cpuid_ext_features; > > > - uint32_t cpuid_xlevel; > > > uint32_t cpuid_model[12]; > > > - uint32_t cpuid_ext2_features; > > > - uint32_t cpuid_ext3_features; > > > uint32_t cpuid_apic_id; > > > - /* Store the results of Centaur's CPUID instructions */ > > > - uint32_t cpuid_xlevel2; > > > - uint32_t cpuid_ext4_features; > > > - /* Flags from CPUID[EAX=7,ECX=0].EBX */ > > > - uint32_t cpuid_7_0_ebx_features; > > > > > > /* MTRRs */ > > > uint64_t mtrr_fixed[11]; > > > @@ -864,8 +855,6 @@ typedef struct CPUX86State { > > > uint8_t soft_interrupt; > > > uint8_t has_error_code; > > > uint32_t sipi_vector; > > > - uint32_t cpuid_kvm_features; > > > - uint32_t cpuid_svm_features; > > > bool tsc_valid; > > > int tsc_khz; > > > void *kvm_xsave_buf; > > [...] > > > diff --git a/target-i386/translate.c b/target-i386/translate.c > > > index 7596a90..d340aab 100644 > > > --- a/target-i386/translate.c > > > +++ b/target-i386/translate.c > > > @@ -8279,11 +8279,11 @@ static inline void > > > gen_intermediate_code_internal(CPUX86State *env, if (flags & > > > HF_SOFTMMU_MASK) { dc->mem_index = (cpu_mmu_index(env) + 1) << 2; > > > } > > > - dc->cpuid_features = env->cpuid_features; > > > - dc->cpuid_ext_features = env->cpuid_ext_features; > > > - dc->cpuid_ext2_features = env->cpuid_ext2_features; > > > - dc->cpuid_ext3_features = env->cpuid_ext3_features; > > > - dc->cpuid_7_0_ebx_features = env->cpuid_7_0_ebx_features; > > > + dc->cpuid_features = env->features[FEAT_1_EDX]; > > > + dc->cpuid_ext_features = env->features[FEAT_1_ECX]; > > > + dc->cpuid_ext2_features = env->features[FEAT_8000_0001_EDX]; > > > + dc->cpuid_ext3_features = env->features[FEAT_8000_0001_ECX]; > > > + dc->cpuid_7_0_ebx_features = env->features[FEAT_7_0_EBX]; > > why leaving cpuid_*features here, it's not much extra code on the first > > glance, so a consistent re-factoring would be justified. > > struct DisasContext doesn't have all feature words (SVM, KVM, and > C000_0001_EDX are not present), so it doesn't need the full feature > array. I also don't know if making the struct larger would impact TCG > performance, so I prefer to not touch that part of the TCG code myself. agreed > > > > > > #ifdef TARGET_X86_64 > > > dc->lma = (flags >> HF_LMA_SHIFT) & 1; > > > dc->code64 = (flags >> HF_CS64_SHIFT) & 1; > > >