On Mon, Apr 15, 2013 at 10:50:20AM +0200, Igor Mammedov wrote:
> 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.
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 <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.
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.
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.
>
> > #ifdef TARGET_X86_64
> > dc->lma = (flags >> HF_LMA_SHIFT) & 1;
> > dc->code64 = (flags >> HF_CS64_SHIFT) & 1;
>
--
Eduardo
