On Thu, 11 Apr 2013 17:07:24 -0300 Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com> > --- > 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. > > 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. > 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 <ehabk...@redhat.com> > --- [...] > 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. > + 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. > #ifdef TARGET_X86_64 > dc->lma = (flags >> HF_LMA_SHIFT) & 1; > dc->code64 = (flags >> HF_CS64_SHIFT) & 1;
