Re: [Qemu-devel] [PATCH qom-cpu-next 2/2] target-i386: Replace cpuid_*features fields with a feature word array

[Eduardo Habkost]

On Mon, Apr 15, 2013 at 10:50:20AM +0200, Igor Mammedov wrote:
> On Thu, 11 Apr 2013 17:07:24 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > ---
> > 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 <address@hidden>
> > ---
> [...]
> > 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